Re: [Qemu-devel] [PATCH 3/4] scsi-generic: avoid invalid access to struct when emulating block limits

[Paolo Bonzini]

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 <address@hidden>
> 
>> +    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