From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40559) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLd3r-0007v1-DK for qemu-devel@nongnu.org; Thu, 06 Mar 2014 13:30:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLd3j-0006B6-Us for qemu-devel@nongnu.org; Thu, 06 Mar 2014 13:30:27 -0500 Message-ID: <5318BEB9.8090105@suse.de> Date: Thu, 06 Mar 2014 19:30:17 +0100 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1386087086-3691-1-git-send-email-mst@redhat.com> <1386087086-3691-23-git-send-email-mst@redhat.com> <529E2FEC.20405@redhat.com> In-Reply-To: <529E2FEC.20405@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 22/23] virtio-scsi: fix buffer overrun on invalid state load List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Peter Maydell , "Michael S. Tsirkin" , qemu-stable , Anthony Liguori , QEMU Developers Am 03.12.2013 20:24, schrieb Paolo Bonzini: > Il 03/12/2013 19:19, Peter Maydell ha scritto: >> On 3 December 2013 16:29, Michael S. Tsirkin wrote: >>> CVE-2013-4542 >>> >>> hw/scsi/scsi-bus.c invokes load_request. >>> >>> virtio_scsi_load_request does: >>> qemu_get_buffer(f, (unsigned char *)&req->elem, sizeof(req->elem)= ); >>> >>> this probably can make elem invalid, for example, >>> make in_num or out_num huge, then: >>> >>> virtio_scsi_parse_req(s, vs->cmd_vqs[n], req); >>> >>> will do: >>> >>> if (req->elem.out_num > 1) { >>> qemu_sgl_init_external(req, &req->elem.out_sg[1], >>> &req->elem.out_addr[1], >>> req->elem.out_num - 1); >>> } else { >>> qemu_sgl_init_external(req, &req->elem.in_sg[1], >>> &req->elem.in_addr[1], >>> req->elem.in_num - 1); >>> } >>> >>> and this will access out of array bounds. >>> suggested patch: >>> >>> Signed-off-by: Michael S. Tsirkin >>> --- >>> hw/scsi/virtio-scsi.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c >>> index 26d95a1..51cc929 100644 >>> --- a/hw/scsi/virtio-scsi.c >>> +++ b/hw/scsi/virtio-scsi.c >>> @@ -147,6 +147,8 @@ static void *virtio_scsi_load_request(QEMUFile *f= , SCSIRequest *sreq) >>> qemu_get_be32s(f, &n); >>> assert(n < vs->conf.num_queues); >>> qemu_get_buffer(f, (unsigned char *)&req->elem, sizeof(req->elem= )); >>> + assert(req->elem.in_num <=3D ARRAY_SIZE(req->elem.in_sg)); >>> + assert(req->elem.out_num <=3D ARRAY_SIZE(req->elem.out_sg)); >> >> Wouldn't it be better to fail migration, as other patches in >> this series do? "Silent security hole if you compile with >> -DNDEBUG" is a little bit unfriendly... >=20 > The problem is that SCSIBusInfo's load_request cannot fail. I can look > at fixing it on top of this series. Paolo, with this series not yet in, am I seeing correctly that no changes to allow failing this have been committed? Can you consider coordinating this with mst? Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg