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