On Thu, Dec 14, 2017 at 6:52 AM, Andrey Smirnov
<andrew.smir...@gmail.com> 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 <peter.mayd...@linaro.org>
> Cc: Jason Wang <jasow...@redhat.com>
> Cc: Philippe Mathieu-Daudé <f4...@amsat.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-...@nongnu.org
> Cc: yurov...@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smir...@gmail.com>
> ---
> 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
>
