Re: [Qemu-arm] [PATCH v2 14/15] sd: Check for READ_MULTIPLE_BLOCK size limit violation first

[Andrey Smirnov]

On Thu, Dec 14, 2017 at 6:52 AM, Andrey Smirnov
<address@hidden> wrote:
> Check for READ_MULTIPLE_BLOCK size limit violation first as opposed to
> doing at the end of the command handler. Consider the following
> scenario:
>
> Emulated host driver is trying to read last byte of the last sector
> via CMD18/ADMA, so what would happen is the following:
>
> 1. "ret" is filled with desired byte and "sd->data_offset" is
>    incremented to 512 by
>
>             ret = sd->data[sd->data_offset ++];
>
> 2. sd->data_offset >= io_len becomes true, so
>
>             sd->data_start += io_len;
>
>    moves "sd->data_start" past valid data boundaries.
>
> 3. as a result "sd->data_start + io_len > sd->size" check becomes true
>    and sd->card_status is marked with ADDRESS_ERROR, telling emulated
>    host that the last CMD18 read failed, despite nothing bad/illegal
>    happening.
>
> To avoid having this false positive, move out-of-bounds check to
> happen before BLK_READ_BLOCK(), so this way it will only trigger if
> illegal read is truly about to happen.
>

Disregard this patch, exactly this fix is already present in latest QEMU master.

Sorry for the noise.

Thanks,
Andrey Smirnov

> Cc: Peter Maydell <address@hidden>
> Cc: Jason Wang <address@hidden>
> Cc: Philippe Mathieu-Daudé <address@hidden>
> Cc: address@hidden
> Cc: address@hidden
> Cc: address@hidden
> Signed-off-by: Andrey Smirnov <address@hidden>
> ---
>  hw/sd/sd.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index ba47bff4db..ce4ef17be3 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1797,8 +1797,17 @@ uint8_t sd_read_data(SDState *sd)
>          break;
>
>      case 18:   /* CMD18:  READ_MULTIPLE_BLOCK */
> -        if (sd->data_offset == 0)
> +        if (sd->data_offset == 0) {
> +            if (sd->data_start + io_len > sd->size) {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "%s: Trying to read past card's capacity\n",
> +                              __func__);
> +                sd->card_status |= ADDRESS_ERROR;
> +                return 0x00;
> +            }
> +
>              BLK_READ_BLOCK(sd->data_start, io_len);
> +        }
>          ret = sd->data[sd->data_offset ++];
>
>          if (sd->data_offset >= io_len) {
> @@ -1812,11 +1821,6 @@ uint8_t sd_read_data(SDState *sd)
>                      break;
>                  }
>              }
> -
> -            if (sd->data_start + io_len > sd->size) {
> -                sd->card_status |= ADDRESS_ERROR;
> -                break;
> -            }
>          }
>          break;
>
> --
> 2.14.3
>