scsi: Silence gcc warning

[PATCH] scsi: Silence gcc warning

Posted by Eric Blake 3 years, 2 months ago

On Fedora 33, gcc 10.2.1 notes that scsi_cdb_length(buf) can set
len==-1, which in turn overflows g_malloc():

[5/5] Linking target qemu-system-x86_64
In function ‘scsi_disk_new_request_dump’,
    inlined from ‘scsi_new_request’ at ../hw/scsi/scsi-disk.c:2608:9:
../hw/scsi/scsi-disk.c:2582:19: warning: argument 1 value ‘18446744073709551612’ exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
 2582 |     line_buffer = g_malloc(len * 5 + 1);
      |                   ^

Silence it with a decent assertion, since we only convert a buffer to
bytes when we have a valid cdb length.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 hw/scsi/scsi-disk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index ed52fcd49ff0..b3311a5657b7 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2579,6 +2579,7 @@ 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;

+    assert(len > 0 && len <= 16);
     line_buffer = g_malloc(len * 5 + 1);

     for (i = 0, p = line_buffer; i < len; i++) {
-- 
2.30.0

Re: [PATCH] scsi: Silence gcc warning

Posted by Philippe Mathieu-Daudé 3 years, 2 months ago

On 2/9/21 4:23 PM, Eric Blake wrote:
> On Fedora 33, gcc 10.2.1 notes that scsi_cdb_length(buf) can set
> len==-1, which in turn overflows g_malloc():
> 
> [5/5] Linking target qemu-system-x86_64
> In function ‘scsi_disk_new_request_dump’,
>     inlined from ‘scsi_new_request’ at ../hw/scsi/scsi-disk.c:2608:9:
> ../hw/scsi/scsi-disk.c:2582:19: warning: argument 1 value ‘18446744073709551612’ exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
>  2582 |     line_buffer = g_malloc(len * 5 + 1);
>       |                   ^
> 
> Silence it with a decent assertion, since we only convert a buffer to
> bytes when we have a valid cdb length.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  hw/scsi/scsi-disk.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index ed52fcd49ff0..b3311a5657b7 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2579,6 +2579,7 @@ 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;
> 
> +    assert(len > 0 && len <= 16);
>      line_buffer = g_malloc(len * 5 + 1);
> 
>      for (i = 0, p = line_buffer; i < len; i++) {
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH] scsi: Silence gcc warning

Posted by Eric Blake 3 years, 1 month ago

Adding qemu-trivial in cc.

On 2/9/21 9:44 AM, Philippe Mathieu-Daudé wrote:
> On 2/9/21 4:23 PM, Eric Blake wrote:
>> On Fedora 33, gcc 10.2.1 notes that scsi_cdb_length(buf) can set
>> len==-1, which in turn overflows g_malloc():
>>
>> [5/5] Linking target qemu-system-x86_64
>> In function ‘scsi_disk_new_request_dump’,
>>     inlined from ‘scsi_new_request’ at ../hw/scsi/scsi-disk.c:2608:9:
>> ../hw/scsi/scsi-disk.c:2582:19: warning: argument 1 value ‘18446744073709551612’ exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
>>  2582 |     line_buffer = g_malloc(len * 5 + 1);
>>       |                   ^
>>
>> Silence it with a decent assertion, since we only convert a buffer to
>> bytes when we have a valid cdb length.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  hw/scsi/scsi-disk.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index ed52fcd49ff0..b3311a5657b7 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -2579,6 +2579,7 @@ 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;
>>
>> +    assert(len > 0 && len <= 16);
>>      line_buffer = g_malloc(len * 5 + 1);
>>
>>      for (i = 0, p = line_buffer; i < len; i++) {
>>
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] scsi: Silence gcc warning

Posted by Laurent Vivier 3 years, 1 month ago

Le 08/03/2021 à 20:54, Eric Blake a écrit :
> Adding qemu-trivial in cc.
> 
> On 2/9/21 9:44 AM, Philippe Mathieu-Daudé wrote:
>> On 2/9/21 4:23 PM, Eric Blake wrote:
>>> On Fedora 33, gcc 10.2.1 notes that scsi_cdb_length(buf) can set
>>> len==-1, which in turn overflows g_malloc():
>>>
>>> [5/5] Linking target qemu-system-x86_64
>>> In function ‘scsi_disk_new_request_dump’,
>>>     inlined from ‘scsi_new_request’ at ../hw/scsi/scsi-disk.c:2608:9:
>>> ../hw/scsi/scsi-disk.c:2582:19: warning: argument 1 value ‘18446744073709551612’ exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
>>>  2582 |     line_buffer = g_malloc(len * 5 + 1);
>>>       |                   ^
>>>
>>> Silence it with a decent assertion, since we only convert a buffer to
>>> bytes when we have a valid cdb length.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>  hw/scsi/scsi-disk.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>>> index ed52fcd49ff0..b3311a5657b7 100644
>>> --- a/hw/scsi/scsi-disk.c
>>> +++ b/hw/scsi/scsi-disk.c
>>> @@ -2579,6 +2579,7 @@ 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;
>>>
>>> +    assert(len > 0 && len <= 16);
>>>      line_buffer = g_malloc(len * 5 + 1);
>>>
>>>      for (i = 0, p = line_buffer; i < len; i++) {
>>>
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
> 

Applied to my trivial-patches branch.

Thanks,
Laurent