| Message ID | 1470861494-17834-1-git-send-email-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Acked-by: Dmitry Fleytman <dmitry@daynix.com> > On 10 Aug 2016, at 23:38 PM, P J P <ppandit@redhat.com> wrote: > > From: Li Qiang <liqiang6-s@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 <liqiang6-s@360.cn> > Signed-off-by: Prasad J Pandit <pjp@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 >
> On 11 Aug 2016, at 11:08 AM, Dmitry Fleytman <dmitry@daynix.com> wrote: > > > Acked-by: Dmitry Fleytman <dmitry@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 <ppandit@redhat.com> wrote: >> >> From: Li Qiang <liqiang6-s@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 <liqiang6-s@360.cn> >> Signed-off-by: Prasad J Pandit <pjp@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 >> >
Hello Dmitry,
I don't see the assert for 'max_frags' in vmxnet device emulation. Could you please point it out?
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.
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?
> -----邮件原件-----
> 发件人: Dmitry Fleytman [mailto:dmitry@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 <dmitry@daynix.com> wrote:
> >
> >
> > Acked-by: Dmitry Fleytman <dmitry@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 <ppandit@redhat.com> wrote:
> >>
> >> From: Li Qiang <liqiang6-s@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 <liqiang6-s@360.cn>
> >> Signed-off-by: Prasad J Pandit <pjp@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
> >>
> >
> On 12 Aug 2016, at 04:21 AM, 李强 <liqiang6-s@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. > > 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:dmitry@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 <dmitry@daynix.com> wrote: >>> >>> >>> Acked-by: Dmitry Fleytman <dmitry@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 <ppandit@redhat.com> wrote: >>>> >>>> From: Li Qiang <liqiang6-s@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 <liqiang6-s@360.cn> >>>> Signed-off-by: Prasad J Pandit <pjp@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 >>>> >>> >
Hi Dmitry > > > On 12 Aug 2016, at 04:21 AM, 李强 <liqiang6-s@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:dmitry@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 <dmitry@daynix.com> > wrote: > >>> > >>> > >>> Acked-by: Dmitry Fleytman <dmitry@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 <ppandit@redhat.com> wrote: > >>>> > >>>> From: Li Qiang <liqiang6-s@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 <liqiang6-s@360.cn> > >>>> Signed-off-by: Prasad J Pandit <pjp@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 > >>>> > >>> > >
> On 12 Aug 2016, at 14:11 PM, 李强 <liqiang6-s@360.cn> wrote: > > Hi Dmitry > >> >>> On 12 Aug 2016, at 04:21 AM, 李强 <liqiang6-s@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:dmitry@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 <dmitry@daynix.com> >> wrote: >>>>> >>>>> >>>>> Acked-by: Dmitry Fleytman <dmitry@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 <ppandit@redhat.com> wrote: >>>>>> >>>>>> From: Li Qiang <liqiang6-s@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 <liqiang6-s@360.cn> >>>>>> Signed-off-by: Prasad J Pandit <pjp@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 >>>>>> >>>>> >>> >
On 12 August 2016 at 02:21, 李强 <liqiang6-s@360.cn> wrote: > Hello Dmitry, > > I don't see the assert for 'max_frags' in vmxnet device emulation. Could you please point it out? > > 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. > > 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; > } If the problem you are trying to guard against is that the multiply in the argument to g_malloc() overflows, then the best way to fix that I think is to use g_new() instead. thanks -- PMM
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;