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

[Paolo Bonzini <pbonzini@redhat.com>]

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