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

[Michael Olbrich]

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.

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 |