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

[Philippe Mathieu-Daudé]

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.

Regards,

Phil.