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

[Philippe Mathieu-Daudé]

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.