Re: [PATCH 10/10] scsi/scsi_bus: fix races in REPORT LUNS

[Maxim Levitsky]

On Fri, 2020-09-25 at 13:26 -0400, Paolo Bonzini wrote:
> From: Maxim Levitsky <mlevitsk@redhat.com>
> 
> Currently scsi_target_emulate_report_luns iterates over the child device list
> twice, and there is no guarantee that this list is the same in both 
> iterations.
> 
> The reason for iterating twice is that the first iteration calculates
> how much memory to allocate.  However if we use a dynamic array we can
> avoid iterating twice, and therefore we avoid this race.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1866707
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <20200913160259.32145-10-mlevitsk@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi/scsi-bus.c | 68 ++++++++++++++++++++++------------------------
>  1 file changed, 33 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index eda8cb7e70..b901e701f0 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -438,19 +438,23 @@ struct SCSITargetReq {
>  static void store_lun(uint8_t *outbuf, int lun)
>  {
>      if (lun < 256) {
> +        /* Simple logical unit addressing method*/
> +        outbuf[0] = 0;
>          outbuf[1] = lun;
> -        return;
> +    } else {
> +        /* Flat space addressing method */
> +        outbuf[0] = 0x40 | (lun >> 8);
> +        outbuf[1] = (lun & 255);
>      }
> -    outbuf[1] = (lun & 255);
> -    outbuf[0] = (lun >> 8) | 0x40;
>  }
>  
>  static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
>  {
>      BusChild *kid;
> -    int i, len, n;
>      int channel, id;
> -    bool found_lun0;
> +    uint8_t tmp[8] = {0};
> +    int len = 0;
> +    GByteArray *buf;
>  
>      if (r->req.cmd.xfer < 16) {
>          return false;
> @@ -458,46 +462,40 @@ static bool 
> scsi_target_emulate_report_luns(SCSITargetReq *r)
>      if (r->req.cmd.buf[2] > 2) {
>          return false;
>      }
> +
> +    /* reserve space for 63 LUNs*/
> +    buf = g_byte_array_sized_new(512);
> +
>      channel = r->req.dev->channel;
>      id = r->req.dev->id;
> -    found_lun0 = false;
> -    n = 0;
>  
> -    RCU_READ_LOCK_GUARD();
> +    /* add size (will be updated later to correct value */
My mistake here - I forgot closing ')' - as I said, there will
be significantly less issues like that in my patches soon.
> +    g_byte_array_append(buf, tmp, 8);
> +    len += 8;
>  
> -    QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
> -        DeviceState *qdev = kid->child;
> -        SCSIDevice *dev = SCSI_DEVICE(qdev);
> +    /* add LUN0 */
> +    g_byte_array_append(buf, tmp, 8);
> +    len += 8;
>  
> -        if (dev->channel == channel && dev->id == id) {
> -            if (dev->lun == 0) {
> -                found_lun0 = true;
> +    WITH_RCU_READ_LOCK_GUARD() {
> +        QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
> +            DeviceState *qdev = kid->child;
> +            SCSIDevice *dev = SCSI_DEVICE(qdev);
> +
> +            if (dev->channel == channel && dev->id == id && dev->lun != 0) {
> +                store_lun(tmp, dev->lun);
> +                g_byte_array_append(buf, tmp, 8);
> +                len += 8;
>              }
> -            n += 8;
>          }
>      }
> -    if (!found_lun0) {
> -        n += 8;
> -    }
> -
> -    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[0], n);
> -    i = found_lun0 ? 8 : 16;
> -    QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
> -        DeviceState *qdev = kid->child;
> -        SCSIDevice *dev = SCSI_DEVICE(qdev);
>  
> -        if (dev->channel == channel && dev->id == id) {
> -            store_lun(&r->buf[i], dev->lun);
> -            i += 8;
> -        }
> -    }
> +    r->buf_len = len;
> +    r->buf = g_byte_array_free(buf, FALSE);
> +    r->len = MIN(len, r->req.cmd.xfer & ~7);
>  
> -    assert(i == n + 8);
> -    r->len = len;
> +    /* store the LUN list length */
> +    stl_be_p(&r->buf[0], len - 8);
>      return true;
>  }
>  
No doubt that I will RCU_READ_LOCK_GUARD more from now on.

Best regards,
        Maxim Levitsky