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

[Dmitry Fleytman]

> On 12 Aug 2016, at 14:11 PM, 李强 <address@hidden> wrote:
> 
> 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;

I see.
In this case I suggest to either check with assert that p->raw is not null 
after allocation or that max_tx_frags <  0x20000000 before allocation.

> 
>>> 
>>> 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
>>>>>> 
>>>>> 
>>> 
>