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. Regards, Phil.