[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH-for-5.2? v3 5/9] scsi: fix overflow in scsi_disk_new_request_
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH-for-5.2? v3 5/9] scsi: fix overflow in scsi_disk_new_request_dump |
Date: |
Mon, 9 Nov 2020 14:26:46 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1 |
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.
>
>
- Re: [PATCH v3 1/9] fuzz: Make fork_fuzz.ld compatible with LLVM's LLD, (continued)
[PATCH v3 7/9] cfi: Initial support for cfi-icall in QEMU, Daniele Buono, 2020/11/05
[PATCH v3 8/9] check-block: enable iotests with cfi-icall, Daniele Buono, 2020/11/05
[PATCH v3 9/9] configure,meson: support Control-Flow Integrity, Daniele Buono, 2020/11/05
[PATCH v3 3/9] hw/usb: reorder fields in UASStatus, Daniele Buono, 2020/11/05
Re: [PATCH v3 0/9] Add support for Control-Flow Integrity, Cornelia Huck, 2020/11/06