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

["Andreas Färber" <afaerber@suse.de>]

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 <mst@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 <mst@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

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