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 <m...@redhat.com> 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 <m...@redhat.com>
> >>> ---
> >>> 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
