| Message ID | 20181203100608.28538-2-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix possible OOB during queuing packets | expand |
On 12/3/18 4:06 AM, Jason Wang wrote: > We try to detect and drop too large packet (>INT_MAX) in 1592a9947036 > ("net: ignore packet size greater than INT_MAX") during packet > delivering. Unfortunately, this is not sufficient as we may hit > another integer overflow when trying to queue such large packet in > qemu_net_queue_append_iov(): > > - size of the allocation may overflow on 32bit > - packet->size is integer which may overflow even on 64bit > > Fixing this by move the check to qemu_sendv_packet_async() which is s/move/moving/ > the entrance of all networking codes and reduce the limit to > NET_BUFSIZE to be more conservative. Please mention commit 1592a994 in the commit message (since you are effectively reverting that with this as its replacement), and if this is (as I suspect) an additional patch required for the complete fix to CVE-2018-10839, also mention that. > > Cc: qemu-stable@nongnu.org > Cc: Li Qiang <liq3ea@163.com> > Reported-by: Li Qiang <liq3ea@gmail.com> > Reviewed-by: Li Qiang <liq3ea@gmail.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > net/net.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/net/net.c b/net/net.c > index 07c194a8f6..affe1877cf 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -712,15 +712,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, > void *opaque) > { > NetClientState *nc = opaque; > - size_t size = iov_size(iov, iovcnt); > int ret; > > - if (size > INT_MAX) { > - return size; > - } > > if (nc->link_down) { > - return size; > + return iov_size(iov, iovcnt); Reverts 1592a994, and... > } > > if (nc->receive_disabled) { > @@ -745,10 +741,15 @@ ssize_t qemu_sendv_packet_async(NetClientState *sender, > NetPacketSent *sent_cb) > { > NetQueue *queue; > + size_t size = iov_size(iov, iovcnt); > int ret; > > + if (size > NET_BUFSIZE) { > + return size; > + } ...returns early for packets larger than 68k (a much smaller limit than INT_MAX, which makes analysis for int overflow a lot easier) at a saner point in the code. Returning a large value is weird, but auditing all callers: hw/net/virtio-net.c: ret = qemu_sendv_packet_async(qemu_get_subqueue(n->nic, queue_index), - only checks if ret is 0 (returns -EBUSY) or not (assumes packet was sent) net/netmap.c: iovsize = qemu_sendv_packet_async(&s->nc, s->iov, iovcnt, - only checks if ret is 0 (starts polling) or not (assumes packet was sent) net/net.c: return qemu_sendv_packet_async(nc, iov, iovcnt, NULL); - implementation of qemu_sendv_packet() - so we have to audit those callers as well: hw/net/net_tx_pkt.c: qemu_sendv_packet(nc, iov, iov_cnt); hw/net/rocker/rocker_fp.c: qemu_sendv_packet(nc, iov, iovcnt); hw/net/rtl8139.c: qemu_sendv_packet(qemu_get_queue(s->nic), iov, 3); net/hub.c: qemu_sendv_packet(&port->nc, iov, iovcnt); - all four of these do not check the return status So, it looks like none of the callers cares if the return value is overlarge (no further math on the values), just that it is non-zero (where the callers then presumably assume the packet was sent). However, I am not familiar enough with the code to know if skipping the packet by returning a non-zero value is going to have knock-on effects - that is, my audit shows what the callers do, but not whether it was sane. > + > if (sender->link_down || !sender->peer) { > - return iov_size(iov, iovcnt); > + return size; > } If this is indeed CVE fixing, then we want it in -rc4, but I don't know if the patch is correctly secure yet without answers to my questions. Especially on a CVE fix for -rc4, you want to make the reviewer's life as easy as possible by providing a commit message that includes enough details to make analysis easy.
On 2018-12-03 11:06, Jason Wang wrote: > We try to detect and drop too large packet (>INT_MAX) in 1592a9947036 > ("net: ignore packet size greater than INT_MAX") during packet > delivering. Unfortunately, this is not sufficient as we may hit > another integer overflow when trying to queue such large packet in > qemu_net_queue_append_iov(): > > - size of the allocation may overflow on 32bit > - packet->size is integer which may overflow even on 64bit > > Fixing this by move the check to qemu_sendv_packet_async() which is > the entrance of all networking codes and reduce the limit to > NET_BUFSIZE to be more conservative. > > Cc: qemu-stable@nongnu.org > Cc: Li Qiang <liq3ea@163.com> > Reported-by: Li Qiang <liq3ea@gmail.com> > Reviewed-by: Li Qiang <liq3ea@gmail.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > net/net.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) Since this is a critical patch for rc4, here's a verbose review... > diff --git a/net/net.c b/net/net.c > index 07c194a8f6..affe1877cf 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -712,15 +712,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, > void *opaque) > { > NetClientState *nc = opaque; > - size_t size = iov_size(iov, iovcnt); > int ret; > > - if (size > INT_MAX) { > - return size; > - } > > if (nc->link_down) { > - return size; > + return iov_size(iov, iovcnt); > } In case you respin this patch again, please make qemu_deliver_packet_iov() "static", so that it is clear that it can not be called directly from the outside anymore. And in case there is no need to respin, please consider to send a separate patch for 4.0 instead. Ok, thinking now load about the call chain: Anyway, qemu_deliver_packet_iov is not directly called from any other file currently, so this is ok ... So let's see how it is used in net.c ... it's only used as paramter here: qemu_new_net_queue(qemu_deliver_packet_iov, nc) ... qemu_new_net_queue() assigns it to NetQueue->deliver which is only used within queue.c. Functions using that ->deliver function pointer are the static functions qemu_net_queue_deliver() and qemu_net_queue_deliver_iov(). First one only uses one iov, so I don't think we can overflow the size here. Second one is used in turn are used by the public function qemu_net_queue_send_iov(). This has two callers, one in net.c in qemu_sendv_packet_async() which you guard below ==> OK. The other caller is in qemu_netfilter_pass_to_next() in filter.c - and this function is called from many more other places ... but as far as I can see, these don't call it in a way where the size could overflow. ==> Removing the check in qemu_deliver_packet_iov() sounds ok to me, if it is checked in qemu_sendv_packet_async() instead. > if (nc->receive_disabled) { > @@ -745,10 +741,15 @@ ssize_t qemu_sendv_packet_async(NetClientState *sender, > NetPacketSent *sent_cb) > { > NetQueue *queue; > + size_t size = iov_size(iov, iovcnt); > int ret; > > + if (size > NET_BUFSIZE) { > + return size; > + } It's a little bit unfortunate that the unsigned size will be cast to ssize_t, so a very large size could suddenly change sign. But as Eric already wrote in his mail, it seems like the callers are either ignoring the return value, or just checking for != 0, so it should be ok for now. To be more consistent, maybe it would be better to always return an negative error code here instead? > if (sender->link_down || !sender->peer) { > - return iov_size(iov, iovcnt); > + return size; > } > > /* Let filters handle the packet first */ > I think I'm fine if this patch is applied in its current shape for rc4, so: Reviewed-by: Thomas Huth <thuth@redhat.com> ... but please consider some follow-up patches to make qemu_deliver_packet_iov() static, and maybe to return an error code in qemu_sendv_packet_async() instead.
On 2018/12/4 上午12:18, Eric Blake wrote: > On 12/3/18 4:06 AM, Jason Wang wrote: >> We try to detect and drop too large packet (>INT_MAX) in 1592a9947036 >> ("net: ignore packet size greater than INT_MAX") during packet >> delivering. Unfortunately, this is not sufficient as we may hit >> another integer overflow when trying to queue such large packet in >> qemu_net_queue_append_iov(): >> >> - size of the allocation may overflow on 32bit >> - packet->size is integer which may overflow even on 64bit >> >> Fixing this by move the check to qemu_sendv_packet_async() which is > > s/move/moving/ Ok. > >> the entrance of all networking codes and reduce the limit to >> NET_BUFSIZE to be more conservative. > > Please mention commit 1592a994 in the commit message (since you are > effectively reverting that with this as its replacement), I think I did it? > and if this is (as I suspect) an additional patch required for the > complete fix to CVE-2018-10839, also mention that. Ok. > >> >> Cc: qemu-stable@nongnu.org >> Cc: Li Qiang <liq3ea@163.com> >> Reported-by: Li Qiang <liq3ea@gmail.com> >> Reviewed-by: Li Qiang <liq3ea@gmail.com> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> net/net.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/net/net.c b/net/net.c >> index 07c194a8f6..affe1877cf 100644 >> --- a/net/net.c >> +++ b/net/net.c >> @@ -712,15 +712,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState >> *sender, >> void *opaque) >> { >> NetClientState *nc = opaque; >> - size_t size = iov_size(iov, iovcnt); >> int ret; >> - if (size > INT_MAX) { >> - return size; >> - } >> if (nc->link_down) { >> - return size; >> + return iov_size(iov, iovcnt); > > Reverts 1592a994, and... > >> } >> if (nc->receive_disabled) { >> @@ -745,10 +741,15 @@ ssize_t qemu_sendv_packet_async(NetClientState >> *sender, >> NetPacketSent *sent_cb) >> { >> NetQueue *queue; >> + size_t size = iov_size(iov, iovcnt); >> int ret; >> + if (size > NET_BUFSIZE) { >> + return size; >> + } > > ...returns early for packets larger than 68k (a much smaller limit > than INT_MAX, which makes analysis for int overflow a lot easier) at a > saner point in the code. Returning a large value is weird, Might be, but we did this for years, see the following return value when link is down. > but auditing all callers: > > hw/net/virtio-net.c: ret = > qemu_sendv_packet_async(qemu_get_subqueue(n->nic, queue_index), > - only checks if ret is 0 (returns -EBUSY) or not (assumes packet was > sent) > net/netmap.c: iovsize = qemu_sendv_packet_async(&s->nc, s->iov, > iovcnt, > - only checks if ret is 0 (starts polling) or not (assumes packet was > sent) > net/net.c: return qemu_sendv_packet_async(nc, iov, iovcnt, NULL); > - implementation of qemu_sendv_packet() - so we have to audit those > callers as well: > > hw/net/net_tx_pkt.c: qemu_sendv_packet(nc, iov, iov_cnt); > hw/net/rocker/rocker_fp.c: qemu_sendv_packet(nc, iov, iovcnt); > hw/net/rtl8139.c: qemu_sendv_packet(qemu_get_queue(s->nic), iov, 3); > net/hub.c: qemu_sendv_packet(&port->nc, iov, iovcnt); > - all four of these do not check the return status > > So, it looks like none of the callers cares if the return value is > overlarge (no further math on the values), just that it is non-zero > (where the callers then presumably assume the packet was sent). Yes, the caller may only care if it returns zero. > However, I am not familiar enough with the code to know if skipping > the packet by returning a non-zero value is going to have knock-on > effects - that is, my audit shows what the callers do, but not whether > it was sane. > The difference between qemu_sendv_packet() and qemu_sendv_packet_async() is that the latter can trigger a callback (sent_cb) when peer can accept more packets. This could be used by some high speed networking implementation to prevent the source from producing more packets and wasting cpu cycles in dropping packets. After peer can accept more, sent_cb usually enable the source to produce packets. Those who use qemu_sendv_packet() will just waste some cpu in dropping the packets. Consider we are emulating ethernet and packet will be copied if queued, it's safe to assume that the packet was sent. >> + >> if (sender->link_down || !sender->peer) { >> - return iov_size(iov, iovcnt); >> + return size; >> } > > If this is indeed CVE fixing, then we want it in -rc4, but I don't > know if the patch is correctly secure yet without answers to my > questions. Especially on a CVE fix for -rc4, you want to make the > reviewer's life as easy as possible by providing a commit message that > includes enough details to make analysis easy. Hope my answer help. If it is, I will add them to the commit log. Thanks for the reviewing.
On 2018/12/4 上午2:13, Thomas Huth wrote: > On 2018-12-03 11:06, Jason Wang wrote: >> We try to detect and drop too large packet (>INT_MAX) in 1592a9947036 >> ("net: ignore packet size greater than INT_MAX") during packet >> delivering. Unfortunately, this is not sufficient as we may hit >> another integer overflow when trying to queue such large packet in >> qemu_net_queue_append_iov(): >> >> - size of the allocation may overflow on 32bit >> - packet->size is integer which may overflow even on 64bit >> >> Fixing this by move the check to qemu_sendv_packet_async() which is >> the entrance of all networking codes and reduce the limit to >> NET_BUFSIZE to be more conservative. >> >> Cc: qemu-stable@nongnu.org >> Cc: Li Qiang <liq3ea@163.com> >> Reported-by: Li Qiang <liq3ea@gmail.com> >> Reviewed-by: Li Qiang <liq3ea@gmail.com> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> net/net.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) > Since this is a critical patch for rc4, here's a verbose review... > >> diff --git a/net/net.c b/net/net.c >> index 07c194a8f6..affe1877cf 100644 >> --- a/net/net.c >> +++ b/net/net.c >> @@ -712,15 +712,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, >> void *opaque) >> { >> NetClientState *nc = opaque; >> - size_t size = iov_size(iov, iovcnt); >> int ret; >> >> - if (size > INT_MAX) { >> - return size; >> - } >> >> if (nc->link_down) { >> - return size; >> + return iov_size(iov, iovcnt); >> } > In case you respin this patch again, please make > qemu_deliver_packet_iov() "static", so that it is clear that it can not > be called directly from the outside anymore. And in case there is no > need to respin, please consider to send a separate patch for 4.0 instead. Yes, will try to make it for the next version. > > Ok, thinking now load about the call chain: > > Anyway, qemu_deliver_packet_iov is not directly called from any other > file currently, so this is ok ... > > So let's see how it is used in net.c ... it's only used as paramter > here: qemu_new_net_queue(qemu_deliver_packet_iov, nc) ... > qemu_new_net_queue() assigns it to NetQueue->deliver which is only used > within queue.c. > > Functions using that ->deliver function pointer are the static functions > qemu_net_queue_deliver() and qemu_net_queue_deliver_iov(). First one > only uses one iov, so I don't think we can overflow the size here. > Second one is used in turn are used by the public function > qemu_net_queue_send_iov(). This has two callers, one in net.c in > qemu_sendv_packet_async() which you guard below ==> OK. > The other caller is in qemu_netfilter_pass_to_next() in filter.c - and > this function is called from many more other places ... but as far as I > can see, these don't call it in a way where the size could overflow. > > ==> Removing the check in qemu_deliver_packet_iov() sounds ok to me, if > it is checked in qemu_sendv_packet_async() instead. Yes. Let me add more in the commit message. > >> if (nc->receive_disabled) { >> @@ -745,10 +741,15 @@ ssize_t qemu_sendv_packet_async(NetClientState *sender, >> NetPacketSent *sent_cb) >> { >> NetQueue *queue; >> + size_t size = iov_size(iov, iovcnt); >> int ret; >> >> + if (size > NET_BUFSIZE) { >> + return size; >> + } > It's a little bit unfortunate that the unsigned size will be cast to > ssize_t, so a very large size could suddenly change sign. But as Eric > already wrote in his mail, it seems like the callers are either ignoring > the return value, or just checking for != 0, so it should be ok for now. > > To be more consistent, maybe it would be better to always return an > negative error code here instead? Yes, and we can do it for 4.0. > >> if (sender->link_down || !sender->peer) { >> - return iov_size(iov, iovcnt); >> + return size; >> } >> >> /* Let filters handle the packet first */ >> > I think I'm fine if this patch is applied in its current shape for rc4, so: > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > ... but please consider some follow-up patches to make > qemu_deliver_packet_iov() static, and maybe to return an error code in > qemu_sendv_packet_async() instead. Yes. Thanks for the reviewing.
diff --git a/net/net.c b/net/net.c index 07c194a8f6..affe1877cf 100644 --- a/net/net.c +++ b/net/net.c @@ -712,15 +712,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, void *opaque) { NetClientState *nc = opaque; - size_t size = iov_size(iov, iovcnt); int ret; - if (size > INT_MAX) { - return size; - } if (nc->link_down) { - return size; + return iov_size(iov, iovcnt); } if (nc->receive_disabled) { @@ -745,10 +741,15 @@ ssize_t qemu_sendv_packet_async(NetClientState *sender, NetPacketSent *sent_cb) { NetQueue *queue; + size_t size = iov_size(iov, iovcnt); int ret; + if (size > NET_BUFSIZE) { + return size; + } + if (sender->link_down || !sender->peer) { - return iov_size(iov, iovcnt); + return size; } /* Let filters handle the packet first */
We try to detect and drop too large packet (>INT_MAX) in 1592a9947036 ("net: ignore packet size greater than INT_MAX") during packet delivering. Unfortunately, this is not sufficient as we may hit another integer overflow when trying to queue such large packet in qemu_net_queue_append_iov(): - size of the allocation may overflow on 32bit - packet->size is integer which may overflow even on 64bit Fixing this by move the check to qemu_sendv_packet_async() which is the entrance of all networking codes and reduce the limit to NET_BUFSIZE to be more conservative. Cc: qemu-stable@nongnu.org Cc: Li Qiang <liq3ea@163.com> Reported-by: Li Qiang <liq3ea@gmail.com> Reviewed-by: Li Qiang <liq3ea@gmail.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- net/net.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)