Re: [PATCH v2] scsi: Allocate SCSITargetReq r->buf dynamically

[Paolo Bonzini <pbonzini@redhat.com>]

Il 09/10/2013 09:41, Asias He ha scritto:
> r->buf is hardcoded to 2056 which is (256 + 1) * 8, allowing 256 luns at
> most. If more than 256 luns are specified by user, we have buffer
> overflow in scsi_target_emulate_report_luns.
> 
> To fix, we allocate the buffer dynamically.
> 
> Signed-off-by: Asias He <asias@redhat.com>
> Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> Changes in v2:
> * Use SCSI_INQUIRY_LEN instead of 36 in scsi_target_emulate_inquiry
> * Do not exceed xfer for REQUEST_SENSE
> 
>  hw/scsi/scsi-bus.c     | 45 ++++++++++++++++++++++++++++++++++-----------
>  include/hw/scsi/scsi.h |  2 ++
>  2 files changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 4d36841..24ec52f 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -11,6 +11,8 @@ static char *scsibus_get_dev_path(DeviceState *dev);
>  static char *scsibus_get_fw_dev_path(DeviceState *dev);
>  static int scsi_req_parse(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf);
>  static void scsi_req_dequeue(SCSIRequest *req);
> +static uint8_t *scsi_target_alloc_buf(SCSIRequest *req, size_t len);
> +static void scsi_target_free_buf(SCSIRequest *req);
>  
>  static Property scsi_props[] = {
>      DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
> @@ -317,7 +319,8 @@ typedef struct SCSITargetReq SCSITargetReq;
>  struct SCSITargetReq {
>      SCSIRequest req;
>      int len;
> -    uint8_t buf[2056];
> +    uint8_t *buf;
> +    int buf_len;
>  };
>  
>  static void store_lun(uint8_t *outbuf, int lun)
> @@ -361,14 +364,12 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
>      if (!found_lun0) {
>          n += 8;
>      }
> -    len = MIN(n + 8, r->req.cmd.xfer & ~7);
> -    if (len > sizeof(r->buf)) {
> -        /* TODO: > 256 LUNs? */
> -        return false;
> -    }
>  
> +    scsi_target_alloc_buf(&r->req, n + 8);
> +
> +    len = MIN(n + 8, r->req.cmd.xfer & ~7);
>      memset(r->buf, 0, len);
> -    stl_be_p(&r->buf, n);
> +    stl_be_p(&r->buf[0], n);
>      i = found_lun0 ? 8 : 16;
>      QTAILQ_FOREACH(kid, &r->req.bus->qbus.children, sibling) {
>          DeviceState *qdev = kid->child;
> @@ -387,6 +388,9 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
>  static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
>  {
>      assert(r->req.dev->lun != r->req.lun);
> +
> +    scsi_target_alloc_buf(&r->req, SCSI_INQUIRY_LEN);
> +
>      if (r->req.cmd.buf[1] & 0x2) {
>          /* Command support data - optional, not implemented */
>          return false;
> @@ -411,7 +415,7 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
>              return false;
>          }
>          /* done with EVPD */
> -        assert(r->len < sizeof(r->buf));
> +        assert(r->len < r->buf_len);
>          r->len = MIN(r->req.cmd.xfer, r->len);
>          return true;
>      }
> @@ -422,7 +426,7 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
>      }
>  
>      /* PAGE CODE == 0 */
> -    r->len = MIN(r->req.cmd.xfer, 36);
> +    r->len = MIN(r->req.cmd.xfer, SCSI_INQUIRY_LEN);
>      memset(r->buf, 0, r->len);
>      if (r->req.lun != 0) {
>          r->buf[0] = TYPE_NO_LUN;
> @@ -455,8 +459,9 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>          }
>          break;
>      case REQUEST_SENSE:
> +        scsi_target_alloc_buf(&r->req, SCSI_SENSE_LEN);
>          r->len = scsi_device_get_sense(r->req.dev, r->buf,
> -                                       MIN(req->cmd.xfer, sizeof r->buf),
> +                                       MIN(req->cmd.xfer, r->buf_len),
>                                         (req->cmd.buf[1] & 1) == 0);
>          if (r->req.dev->sense_is_ua) {
>              scsi_device_unit_attention_reported(req->dev);
> @@ -501,11 +506,29 @@ static uint8_t *scsi_target_get_buf(SCSIRequest *req)
>      return r->buf;
>  }
>  
> +static uint8_t *scsi_target_alloc_buf(SCSIRequest *req, size_t len)
> +{
> +    SCSITargetReq *r = DO_UPCAST(SCSITargetReq, req, req);
> +
> +    r->buf = g_malloc(len);
> +    r->buf_len = len;
> +
> +    return r->buf;
> +}
> +
> +static void scsi_target_free_buf(SCSIRequest *req)
> +{
> +    SCSITargetReq *r = DO_UPCAST(SCSITargetReq, req, req);
> +
> +    g_free(r->buf);
> +}
> +
>  static const struct SCSIReqOps reqops_target_command = {
>      .size         = sizeof(SCSITargetReq),
>      .send_command = scsi_target_send_command,
>      .read_data    = scsi_target_read_data,
>      .get_buf      = scsi_target_get_buf,
> +    .free_req     = scsi_target_free_buf,
>  };
>  
>  
> @@ -1365,7 +1388,7 @@ int scsi_build_sense(uint8_t *in_buf, int in_len,
>          buf[7] = 10;
>          buf[12] = sense.asc;
>          buf[13] = sense.ascq;
> -        return MIN(len, 18);
> +        return MIN(len, SCSI_SENSE_LEN);
>      } else {
>          /* Return descriptor format sense buffer */
>          buf[0] = 0x72;
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 1b66510..76f6ac2 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -9,6 +9,8 @@
>  #define MAX_SCSI_DEVS	255
>  
>  #define SCSI_CMD_BUF_SIZE     16
> +#define SCSI_SENSE_LEN      18
> +#define SCSI_INQUIRY_LEN    36
>  
>  typedef struct SCSIBus SCSIBus;
>  typedef struct SCSIBusInfo SCSIBusInfo;
> 

Applied to scsi-next branch, thanks.

Paolo