Re: [Qemu-devel] [PATCH v2] hw/sd: fix out-of-bounds check for multi block reads

[Alistair Francis]

On Sat, Sep 16, 2017 at 3:35 AM, Michael Olbrich
<address@hidden> wrote:
> The current code checks if the next block exceeds the size of the card.
> This generates an error while reading the last block of the card.
> Do the out-of-bounds check when starting to read a new block to fix this.
>
> This issue became visible with increased error checking in Linux 4.13.
>
> Signed-off-by: Michael Olbrich <address@hidden>

Thanks for the patch!

> ---
>
> Changes in v2:
>  - fixed warning
>
> I'm not quite sure if 0x00 is the correct return value, but it's used
> elsewhere in the same function when an error occurs, so it seems
> reasonable.

Returning 0 looks fine to me.

>
>  hw/sd/sd.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index ba47bff4db80..35347a5bbcde 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1797,8 +1797,13 @@ 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) {
> +                sd->card_status |= ADDRESS_ERROR;
> +                return 0x00;
> +            }

Why move it inside the if (sd->data_offset == 0) and not just below
the ret = sd->data[sd->data_offset ++] ?

Thanks,
Alistair

>              BLK_READ_BLOCK(sd->data_start, io_len);
> +        }
>          ret = sd->data[sd->data_offset ++];
>
>          if (sd->data_offset >= io_len) {
> @@ -1812,11 +1817,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.1
>
>