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

[李强]

Hi Dmitry

> 
> > On 12 Aug 2016, at 04:21 AM, 李强 <address@hidden> wrote:
> >
> > Hello Dmitry,
> >
> > I don't see the assert for 'max_frags' in vmxnet device emulation. Could you
> please point it out?
> 
> 
>  Hi,
> 
> I mean that max_frags for vmxnet3 device is a size of TX ring so assert
> introduced by this patch will fire all the time.
> 

Got it.

> >
> > 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.
> 
> Yes, it is null because of memory allocation failure. However in real life
> scenarios it can not be that big unless TX ring is that big, and in that case 
> you’ll
> get memory allocation problem earlier (during TX ring allocation).
> 
> Additionally, one should not limit max_frags by maximum number of skb
> fragments because not all network backends produce skb’s.
> 

It is null not because of memory allocation failure but integer overflow. In 
x86, 0x20000000 * sizeof *p->raw) will be rolled to 0, g_malloc(0) will return 
NULL. This is not a failure.
We can set vmxnet3 ' s->max_tx_frags' to any value from the guest kernel.

In vmxnet3_activate_device(), we can see the 'size' is read from the input of 
guest. We can set 'conf.txRingSize' by insmod a kernel module in guest. 
Actually, we can reset guest driver shared area and control all the data. 

       size = VMXNET3_READ_TX_QUEUE_DESCR32(qdescr_pa, conf.txRingSize);

        vmxnet3_ring_init(&s->txq_descr[i].tx_ring, pa, size,
                          sizeof(struct Vmxnet3_TxDesc), false);
        VMXNET3_RING_DUMP(VMW_CFPRN, "TX", i, &s->txq_descr[i].tx_ring);

        s->max_tx_frags += size;

> >
> > 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?
> 
> Not sure such an assert is needed at all. In general, QEMU code does not check
> memory allocation results.
> 

I think this is not related to memory allocation results but is related to 
integer overflow.

Thanks.


--
Li Qiang / Cloud Security Team, Qihoo 360 Inc

> >
> >> -----邮件原件-----
> >> 发件人: Dmitry Fleytman [mailto:address@hidden
> >> 发送时间: 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 <address@hidden>
> wrote:
> >>>
> >>>
> >>> Acked-by: Dmitry Fleytman <address@hidden>
> >>
> >> 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 <address@hidden> wrote:
> >>>>
> >>>> From: Li Qiang <address@hidden>
> >>>>
> >>>> 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 <address@hidden>
> >>>> Signed-off-by: Prasad J Pandit <address@hidden>
> >>>> ---
> >>>> 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
> >>>>
> >>>
> >