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

[Alistair Francis]

On Tue, Sep 19, 2017 at 1:23 AM, Michael Olbrich
<address@hidden> wrote:
> On Mon, Sep 18, 2017 at 02:28:26PM -0700, Alistair Francis wrote:
>> 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 ++] ?
>>
>> >              BLK_READ_BLOCK(sd->data_start, io_len);
>
> Mostly because of the line above. This copies the full block from the
> backend storage to sd->data, so we need to make sure that the data is
> actually available to fill sd->data, not if it's ok to access a certain
> byte within sd->data.

Doesn't this mean that the check is only done for the first block
then? When data_offset is 0.

Thanks,
Alistair

>
> Michael
>
>> > +        }
>> >          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
>> >
>> >
>>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |