mbox series

[V2,for,3.1,0/4] Fix possible OOB during queuing packets

Message ID 20181129121449.4322-1-jasowang@redhat.com
Headers show
Series Fix possible OOB during queuing packets | expand

Message

Jason Wang Nov. 29, 2018, 12:14 p.m. UTC
Hi:

This series tries to fix a possible OOB during queueing packets
through qemu_net_queue_append_iov(). This could happen when it tries
to queue a packet whose size is larger than INT_MAX which may lead
integer overflow. We've fixed similar issue in the past during
qemu_net_queue_deliver_iov() by ignoring large packets there. Let's
just move the check earlier to qemu_sendv_packet_async() and reduce
the limitation to NET_BUFSIZE. A simple qtest were also added this.

Please review.

Thanks

Jason Wang (4):
  net: drop too large packet early
  virtio-net-test: remove unused macro
  virtio-net-test: accept variable length argument in pci_test_start()
  virtio-net-test: add large tx buffer test

 net/net.c               | 13 +++++----
 tests/virtio-net-test.c | 63 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 63 insertions(+), 13 deletions(-)

Comments

Eric Blake Nov. 29, 2018, 2:05 p.m. UTC | #1
On 11/29/18 6:14 AM, Jason Wang wrote:
> Hi:
> 
> This series tries to fix a possible OOB during queueing packets
> through qemu_net_queue_append_iov(). This could happen when it tries
> to queue a packet whose size is larger than INT_MAX which may lead
> integer overflow. We've fixed similar issue in the past during
> qemu_net_queue_deliver_iov() by ignoring large packets there. Let's
> just move the check earlier to qemu_sendv_packet_async() and reduce
> the limitation to NET_BUFSIZE. A simple qtest were also added this.
> 
> Please review.

How important is this for 3.1?  We've missed -rc3.  Is this CVE quality 
because of a guest being able to cause mayhem by intentionally getting 
into this condition (in which case, we need it, as well as a CVE 
assigned)? Is it pre-existing in 3.0 at which point waiting for 4.0 is 
no worse off than what we already are?
Prasad Pandit Nov. 30, 2018, 9:18 a.m. UTC | #2
+-- On Thu, 29 Nov 2018, Eric Blake wrote --+
| How important is this for 3.1?  We've missed -rc3.  Is this CVE quality 
| because of a guest being able to cause mayhem by intentionally getting into 
| this condition (in which case, we need it, as well as a CVE assigned)? Is it 
| pre-existing in 3.0 at which point waiting for 4.0 is no worse off than what 
| we already are?

It is a revised patch to fix 'CVE-2018-17963' issue. Earlier patch was 
included in -rc0.

$ git tag --contains 1592a9947036d60dde5404204a5d45975133caf5
v3.1.0-rc0
v3.1.0-rc1
v3.1.0-rc2
v3.1.0-rc3

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Jason Wang Nov. 30, 2018, 1:04 p.m. UTC | #3
On 2018/11/30 下午5:18, P J P wrote:
> +-- On Thu, 29 Nov 2018, Eric Blake wrote --+
> | How important is this for 3.1?  We've missed -rc3.  Is this CVE quality
> | because of a guest being able to cause mayhem by intentionally getting into
> | this condition (in which case, we need it, as well as a CVE assigned)?


Yes, malicious guest can do this, but only with some specific setup e.g 
with hubports.


>   Is it
> | pre-existing in 3.0 at which point waiting for 4.0 is no worse off than what
> | we already are?
>
> It is a revised patch to fix 'CVE-2018-17963' issue. Earlier patch was
> included in -rc0.
>
> $ git tag --contains 1592a9947036d60dde5404204a5d45975133caf5
> v3.1.0-rc0
> v3.1.0-rc1
> v3.1.0-rc2
> v3.1.0-rc3
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

Yes, it could be treated as a follow up fixes for CVE-2018-17963. I 
think we need this.

Thanks
Daniel P. Berrangé Nov. 30, 2018, 1:29 p.m. UTC | #4
On Fri, Nov 30, 2018 at 02:48:19PM +0530, P J P wrote:
> +-- On Thu, 29 Nov 2018, Eric Blake wrote --+
> | How important is this for 3.1?  We've missed -rc3.  Is this CVE quality 
> | because of a guest being able to cause mayhem by intentionally getting into 
> | this condition (in which case, we need it, as well as a CVE assigned)? Is it 
> | pre-existing in 3.0 at which point waiting for 4.0 is no worse off than what 
> | we already are?
> 
> It is a revised patch to fix 'CVE-2018-17963' issue. Earlier patch was 
> included in -rc0.
> 
> $ git tag --contains 1592a9947036d60dde5404204a5d45975133caf5
> v3.1.0-rc0
> v3.1.0-rc1
> v3.1.0-rc2
> v3.1.0-rc3

If we've already tried to fix CVE-2018-17963 in 3.1.0 and failed to address
some edge cases, then it would be really desirable to get this into 3.1.0
too.  If we waited until 4.0.0, then we'd need to consider it to be a new
CVE on the grounds that 3.1.0 released a flawed fix.


Regards,
Daniel