From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50191) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLdSl-0002P0-1G for qemu-devel@nongnu.org; Thu, 06 Mar 2014 13:56:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLdAV-0000N8-SH for qemu-devel@nongnu.org; Thu, 06 Mar 2014 13:37:57 -0500 Date: Thu, 6 Mar 2014 20:36:34 +0200 From: "Michael S. Tsirkin" Message-ID: <20140306183634.GA29043@redhat.com> References: <1386087086-3691-1-git-send-email-mst@redhat.com> <1386087086-3691-23-git-send-email-mst@redhat.com> <529E2FEC.20405@redhat.com> <5318BEB9.8090105@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <5318BEB9.8090105@suse.de> 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: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: QEMU Developers , Paolo Bonzini , qemu-stable , Anthony Liguori , Peter Maydell On Thu, Mar 06, 2014 at 07:30:17PM +0100, Andreas F=E4rber wrote: > 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->ele= m)); > >>> > >>> 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->el= em)); > >>> + 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 lo= ok > > at fixing it on top of this series. >=20 > 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? >=20 > Regards, > Andreas BTW I don't see a lot of value in supporting NDEBUG and a bunch of other things will probably break if we do. For now I think we should just stick #ifdef NDEBUG #error building with NDEBUG is not supported #endif > --=20 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn= berg