> On 12 Aug 2016, at 14:11 PM, 李强 <liqiang...@360.cn> wrote:
>
> Hi Dmitry
>
>>
>>> On 12 Aug 2016, at 04:21 AM, 李强 <liqiang...@360.cn> 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: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
>>>>>>
>>>>>
>>>
>
