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 <dbu...@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. > >