[Qemu-devel] Re: [PATCH] fix virtio_blk serial pci config breakage, v2

[john cooper <john.cooper@redhat.com>]

Michael S. Tsirkin wrote:
>> +    put_le16(p + 0, 0x0);                            /* ATA device */
>> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
> 
> QEMU version is currently a string like "0.11.50" which is exactly 8
> bytes. What if someone makes it longer?  padstr will not 0
> terminate string, and only partial data will be there.

This code treats the field similar to the logic from which
it derives (hw/ide.c) in that the field need not be nul
terminated.  Quiet truncation to 8 bytes can occur here
and in the existing usage but in a practical sense I don't
see much of a recourse.  We can flag a warning but the
data is realistically a best-effort attempt to provide
relevant information in this field.  IOW overflowing
this field probably isn't justification alone to modify
a too long qemu version string.

> Also, identify is pre-initialized to 0, isn't it?
> So just strcpy should be enough, here and elsewhere,
> no need to roll our own padstr.

Actually this is an oversight in the local padstr() which
should be padding the balance of the field with ' ' vs. '\0'.

>> +            memcpy(req->elem.in_sg[0].iov_base, s->identify,
>> +                req->elem.in_sg[0].iov_len);
> 
> Is this safe? Can guest make iov_len bigger than size of s->identity?

Good point, a malicious/buggy guest can.  The memcpy
length should be capped. 

>> +    virtio_identify_template(s);
>> +    strncpy((char *)&s->identify[VIRTIO_BLK_ID_SN],
>> +        (char *)drive_get_serial(bs), VIRTIO_BLK_ID_SN_BYTES);
> 
> This can silently truncate the serial, can't it?

Yes, it is the same disposition as ide/scsi's treatment
of the S/N.  My concern was of keeping the behavior
consistent.

Thanks,

-john

-- 
john.cooper@redhat.com