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

[Dmitry Fleytman]

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

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

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

Best Regards,
Dmitry

> 
>> -----邮件原件-----
>> 发件人: 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
>>>> 
>>> 
>