[Qemu-devel] 答复: [PATCH] net: vmxnet: check fragments count at pkt initialisation

[李强]

Hello Dmitry,

I don't see the assert for 'max_frags' in vmxnet device emulation. Could you 
please point it out?

In my PoC, I set it to '0x20000000',  and in vmxnet_tx_pkt_init() the 'p->raw' 
will be NULL because of an integer overflow(in x86). And this will bypass all 
the assert, and in 
vmxnet_tx_pkt_add_raw_fragment(), will cause an NULL pointer reference.

void vmxnet_tx_pkt_init(struct VmxnetTxPkt **pkt, uint32_t max_frags,
    bool has_virt_hdr)
{
    struct VmxnetTxPkt *p = g_malloc0(sizeof *p);

    p->vec = g_malloc((sizeof *p->vec) *
        (max_frags + VMXNET_TX_PKT_PL_START_FRAG));

    p->raw = g_malloc((sizeof *p->raw) * max_frags);

    *pkt = p;
}

IIUC, we should do assert in the device layer, in vmxnet3_activate_device() in 
vmxnet?

> -----邮件原件-----
> 发件人: Dmitry Fleytman [mailto:dmi...@daynix.com]
> 发送时间: 2016年8月11日 16:16
> 收件人: P J P
> 抄送: Qemu developers; 李强; Jason Wang
> 主题: Re: [PATCH] net: vmxnet: check fragments count at pkt initialisation
> 
> 
> > On 11 Aug 2016, at 11:08 AM, Dmitry Fleytman <dmi...@daynix.com> wrote:
> >
> >
> > Acked-by: Dmitry Fleytman <dmi...@daynix.com>
> 
> Oops, please ignore this ACK, I replied to the wrong e-mail.
> 
> As far as I see max_frags for VMXNET3 is a size of device’s TX ring so this 
> will
> always assert.
> 
> I don’t think we need this limitation in the device code. Maximum number of
> fragments is an internal knowledge of network backend.
> 
> ~Dmitry
> 
> >
> >> On 10 Aug 2016, at 23:38 PM, P J P <ppan...@redhat.com> wrote:
> >>
> >> From: Li Qiang <liqiang...@360.cn>
> >>
> >> When net transport abstraction layer initialises the pkt, the maximum
> >> fragmentation count is not checked. This could lead to an integer
> >> overflow causing a NULL pointer dereference.
> >> Add check to avoid it.
> >>
> >> Reported-by: Li Qiang <liqiang...@360.cn>
> >> Signed-off-by: Prasad J Pandit <p...@fedoraproject.org>
> >> ---
> >> hw/net/net_tx_pkt.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c index
> >> 53dfaa2..7ea3c17 100644
> >> --- a/hw/net/net_tx_pkt.c
> >> +++ b/hw/net/net_tx_pkt.c
> >> @@ -58,9 +58,12 @@ struct NetTxPkt {
> >>    bool is_loopback;
> >> };
> >>
> >> +#define NET_PKT_MAX_FRAGS    16  /* ref: MAX_SKB_FRAGS in kernel
> driver */
> >> +
> >> void net_tx_pkt_init(struct NetTxPkt **pkt, PCIDevice *pci_dev,
> >>    uint32_t max_frags, bool has_virt_hdr) {
> >> +    assert(max_frags <= NET_PKT_MAX_FRAGS);
> >>    struct NetTxPkt *p = g_malloc0(sizeof *p);
> >>
> >>    p->pci_dev = pci_dev;
> >> --
> >> 2.5.5
> >>
> >