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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4774 bytes --]

On 29.10.18 18:34, Paolo Bonzini wrote:
> Emulation of the block limits VPD page called back into scsi-disk.c,
> which however expected the request to be for a SCSIDiskState and
> accessed a scsi-generic device outside the bounds of its struct
> (namely to retrieve s->max_unmap_size and s->max_io_size).
> 
> To avoid this, move the emulation code to a separate function that
> takes a new SCSIBlockLimits struct and marshals it into the VPD
> response format.
> 
> Reported-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi/Makefile.objs       |  2 +-
>  hw/scsi/emulation.c         | 42 +++++++++++++++++
>  hw/scsi/scsi-disk.c         | 92 ++++++++-----------------------------
>  hw/scsi/scsi-generic.c      | 22 +++++++--
>  include/hw/scsi/emulation.h | 16 +++++++
>  include/hw/scsi/scsi.h      |  1 -
>  6 files changed, 98 insertions(+), 77 deletions(-)
>  create mode 100644 hw/scsi/emulation.c
>  create mode 100644 include/hw/scsi/emulation.h
> 

[...]

> diff --git a/hw/scsi/emulation.c b/hw/scsi/emulation.c
> new file mode 100644
> index 0000000000..94c2254bb4
> --- /dev/null
> +++ b/hw/scsi/emulation.c
> @@ -0,0 +1,42 @@
> +#include "qemu/osdep.h"
> +#include "qemu/units.h"
> +#include "qemu/bswap.h"
> +#include "hw/scsi/emulation.h"
> +
> +int scsi_emulate_block_limits(uint8_t *outbuf, SCSIBlockLimits *bl)

I'd make @bl a const *, but it's not like qemu is the kind of code base
to complain about strict const-ness.

> +{
> +    /* required VPD size with unmap support */
> +    memset(outbuf, 0, 0x3C);

Upper case hex here...

> +
> +    outbuf[0] = bl->wsnz; /* wsnz */
> +
> +    if (bl->max_io_sectors) {
> +        /* optimal transfer length granularity.  This field and the optimal
> +         * transfer length can't be greater than maximum transfer length.
> +         */
> +        stw_be_p(outbuf + 2, MIN(bl->min_io_size, bl->max_io_sectors));
> +
> +        /* maximum transfer length */
> +        stl_be_p(outbuf + 4, bl->max_io_sectors);
> +
> +        /* optimal transfer length */
> +        stl_be_p(outbuf + 8, MIN(bl->opt_io_size, bl->max_io_sectors));
> +    } else {
> +        stw_be_p(outbuf + 2, bl->min_io_size);
> +        stl_be_p(outbuf + 8, bl->opt_io_size);
> +    }
> +
> +    /* max unmap LBA count */
> +    stl_be_p(outbuf + 16, bl->max_unmap_sectors);
> +
> +    /* max unmap descriptors */
> +    stl_be_p(outbuf + 20, bl->max_unmap_descr);
> +
> +    /* optimal unmap granularity; alignment is zero */
> +    stl_be_p(outbuf + 24, bl->unmap_sectors);
> +
> +    /* max write same size, make it the same as maximum transfer length */
> +    stl_be_p(outbuf + 36, bl->max_io_sectors);
> +
> +    return 0x3c;

...and lower case here?  (Sorry, I couldn't just bear it.)

> +}

[...]

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

> +    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.

Or if you know that qemu does not support any architecture ABIs where
the stack can grow up, that's OK, too.


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
(:-)):

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +    stw_be_p(buf + 2, len);
> +
> +    memcpy(r->buf, buf, MIN(r->buflen, len + 4));
> +
>      r->io_header.sb_len_wr = 0;
>  
>      /*


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]