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
