Re: [Qemu-devel] [PATCH 22/23] virtio-scsi: fix buffer overrun on invalid state load

[Michael S. Tsirkin]

On Thu, Mar 06, 2014 at 07:30:17PM +0100, Andreas Färber 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 <address@hidden> 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 <address@hidden>
> >>> ---
> >>>  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 <= ARRAY_SIZE(req->elem.in_sg));
> >>> +    assert(req->elem.out_num <= 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...
> > 
> > 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

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


> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg