Re: [PATCH-for-5.2? v3 5/9] scsi: fix overflow in scsi_disk_new_request_dump

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 11/6/20 3:43 PM, Philippe Mathieu-Daudé wrote:
> On 11/6/20 3:32 PM, Philippe Mathieu-Daudé wrote:
>> On 11/5/20 11:19 PM, Daniele Buono wrote:
>>> scsi_disk_new_request_dump is used to dump the content of a scsi request
>>> for tracing. It does that by decoding the command to get the size of the
>>> command buffer, and then printing the content of such buffer on a string.
>>>
>>> When using gcc with link-time optimizations, it warns that the argument of
>>> malloc may be too large.
>>>
>>> In function 'scsi_disk_new_request_dump',
>>>     inlined from 'scsi_new_request' at ../qemu-cfi-v3/hw/scsi/scsi-disk.c:2588:9:
>>> ../qemu-cfi-v3/hw/scsi/scsi-disk.c:2562:17: warning: argument 1 value '18446744073709551612' exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
>>>      line_buffer = g_malloc(len * 5 + 1);
>>>                  ^
>>> ../qemu-cfi-v3/hw/scsi/scsi-disk.c: In function 'scsi_new_request':
>>> /usr/include/glib-2.0/glib/gmem.h:78:10: note: in a call to allocation function 'g_malloc' declared here
>>>  gpointer g_malloc         (gsize  n_bytes) G_GNUC_MALLOC G_GNUC_ALLOC_SIZE(1);
>>>
>>> len is a signed integer filled up by scsi_cdb_length which can return -1
>>> if it can't decode the command. In this case, g_malloc would probably fail.
>>> However, an unknown command here is a possibility, and since this is used for
>>> tracing, we should try to print the command anyway, for debugging purposes.
>>>
>>> Since knowing the size of the command in the buffer is impossible (could not
>>> decode the command), only print the header by setting len=1 if scsi_cdb_length
>>> returned -1
>>>
>>> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
>>> ---
>>> If we had a way to know the (maximum) size of the buffer, we could
>>> alternatively dump the whole buffer, instead of dumping only the
>>> first byte. Not sure if this can be done, nor if it is considered
>>> a better option.
>>>
>>> We could also produce an error instead/in addition to just dumping
>>> the buffer, if the command cannot be decoded.
>>>
>>>  hw/scsi/scsi-disk.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>>> index e859534eaf..d70dfdd9dc 100644
>>> --- a/hw/scsi/scsi-disk.c
>>> +++ b/hw/scsi/scsi-disk.c
>>> @@ -2559,6 +2559,10 @@ static void scsi_disk_new_request_dump(uint32_t lun, uint32_t tag, uint8_t *buf)
>>>      int len = scsi_cdb_length(buf);
>>>      char *line_buffer, *p;
>>>  
>>> +    if (len < 0) {
>>> +        len = 1;
>>> +    }
>>> +
>>>      line_buffer = g_malloc(len * 5 + 1);
>>>  
>>>      for (i = 0, p = line_buffer; i < len; i++) {
>>>
>>
>> I think scsi_cdb_length() should always return >=1,
>> and scsi_req_parse_cdb() return if len <= 1.
> 
> Looking at how this works, scsi_req_new() shouldn't take
> only a pointer to buffer without knowing its size...
> We should add a buflen argument and propagate it.
> 
> Then we can check if scsi_cdb_length() <= buflen,
> and dump buflen if unknown opcode.

I did it. Will post later as this is 6.0 material.

> 
> Regards,
> 
> Phil.
> 
>