On 06/11/2018 03:16, Max Reitz wrote:
>> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
>> index c5497bbea8..8fc74ef0bd 100644
>> --- a/hw/scsi/scsi-generic.c
>> +++ b/hw/scsi/scsi-generic.c
>> @@ -16,6 +16,7 @@
>> #include "qemu-common.h"
>> #include "qemu/error-report.h"
>> #include "hw/scsi/scsi.h"
>> +#include "hw/scsi/emulation.h"
>> #include "sysemu/block-backend.h"
>>
>> #ifdef __linux__
>> @@ -209,9 +210,24 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq
>> *r, SCSIDevice *s)
>> }
>> }
>>
>> -static int scsi_emulate_block_limits(SCSIGenericReq *r)
>> +static int scsi_generic_emulate_block_limits(SCSIGenericReq *r, SCSIDevice
>> *s)
>> {
>> - r->buflen = scsi_disk_emulate_vpd_page(&r->req, r->buf);
>> + int len, buflen;
>
> buflen is unused, so this does not compile for me.
Yeah, I forgot to commit it.
>> + uint8_t buf[64];
>> +
>> + SCSIBlockLimits bl = {
>> + .max_io_sectors = blk_get_max_transfer(s->conf.blk) / s->blocksize
>> + };
>> +
>> + memset(r->buf, 0, r->buflen);
>> + stb_p(buf, s->type);
>> + stb_p(buf + 1, 0xb0);
>> + len = scsi_emulate_block_limits(buf + 4, &bl);
>> + assert(len <= sizeof(buf) - 4);
>
> Let's hope our stack grows downwards, otherwise we'll never get back
> here if there was an overflow. Maybe it would be better to pass the
> buffer length to scsi_emulate_block_limits() and then move the assertion
> there.
Isn't that a problem always with stacks that grow upwards? A buffer
overflow on an argument that points to a local variable will always
corrupt the stack frame (on the other hand, a stack that grows downwards
will corrupt the stack frame if the buffer overflow occurs directly in
the function with no call in between).
> With buflen dropped, and you taking full responsibility for any future
> bugs on ABIs with upwards stacks when someone extended
> scsi_emulate_block_limits(), forgetting to adjust the buffer size here
> (:-)):
I think it's quite likely that the bug would be caught first on a
downwards-stack architecture. :)
The complete delta between v1 and v2 will be
diff --git a/hw/scsi/emulation.c b/hw/scsi/emulation.c
index 94c2254bb4..06d62f3c38 100644
--- a/hw/scsi/emulation.c
+++ b/hw/scsi/emulation.c
@@ -3,10 +3,10 @@
#include "qemu/bswap.h"
#include "hw/scsi/emulation.h"
-int scsi_emulate_block_limits(uint8_t *outbuf, SCSIBlockLimits *bl)
+int scsi_emulate_block_limits(uint8_t *outbuf, const SCSIBlockLimits *bl)
{
/* required VPD size with unmap support */
- memset(outbuf, 0, 0x3C);
+ memset(outbuf, 0, 0x3c);
outbuf[0] = bl->wsnz; /* wsnz */
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index b8fa0a0f7e..7237b4162e 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -182,7 +182,7 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq
*r, SCSIDevice *s)
/* Also take care of the opt xfer len. */
stl_be_p(&r->buf[12],
MIN_NON_ZERO(max_transfer, ldl_be_p(&r->buf[12])));
- } else if (page == 0x00 && s->needs_vpd_bl_emulation) {
+ } else if (s->needs_vpd_bl_emulation && page == 0x00) {
/*
* Now we're capable of supplying the VPD Block Limits
* response if the hardware can't. Add it in the INQUIRY
@@ -268,15 +268,14 @@ static void scsi_read_complete(void * opaque, int ret)
* resulted in sense error but would need emulation.
* In this case, emulate a valid VPD response.
*/
- if (ret == 0 &&
+ if (s->needs_vpd_bl_emulation && ret == 0 &&
(r->io_header.driver_status & SG_ERR_DRIVER_SENSE) &&
- scsi_parse_sense_buf(r->req.sense, r->io_header.sb_len_wr).key
== ILLEGAL_REQUEST &&
- s->needs_vpd_bl_emulation) {
- int is_vpd_bl = r->req.cmd.buf[0] == INQUIRY &&
- r->req.cmd.buf[1] & 0x01 &&
- r->req.cmd.buf[2] == 0xb0;
-
- if (is_vpd_bl) {
+ r->req.cmd.buf[0] == INQUIRY &&
+ (r->req.cmd.buf[1] & 0x01) &&
+ r->req.cmd.buf[2] == 0xb0) {
+ SCSISense sense =
+ scsi_parse_sense_buf(r->req.sense, r->io_header.sb_len_wr);
+ if (sense.key == ILLEGAL_REQUEST) {
len = scsi_generic_emulate_block_limits(r, s);
/*
* No need to let scsi_read_complete go on and handle an
diff --git a/include/hw/scsi/emulation.h b/include/hw/scsi/emulation.h
index 42de7c30c2..09fba1ff39 100644
--- a/include/hw/scsi/emulation.h
+++ b/include/hw/scsi/emulation.h
@@ -11,6 +11,6 @@ typedef struct SCSIBlockLimits {
uint32_t max_io_sectors;
} SCSIBlockLimits;
-int scsi_emulate_block_limits(uint8_t *outbuf, SCSIBlockLimits *bl);
+int scsi_emulate_block_limits(uint8_t *outbuf, const SCSIBlockLimits *bl);
#endif
with most of the changes being just cosmetic to avoid the long line
without reindenting everything.
Paolo
> Reviewed-by: Max Reitz <mre...@redhat.com>
>
>> + stw_be_p(buf + 2, len);
>> +
>> + memcpy(r->buf, buf, MIN(r->buflen, len + 4));
>> +
>> r->io_header.sb_len_wr = 0;
>>
>> /*
>
signature.asc
Description: OpenPGP digital signature
