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

[Michael Olbrich <m.olbrich@pengutronix.de>]

On Tue, Sep 19, 2017 at 05:09:51PM -0700, Alistair Francis wrote:
> On Tue, Sep 19, 2017 at 1:23 AM, Michael Olbrich
> <m.olbrich@pengutronix.de> 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
> >> <m.olbrich@pengutronix.de> wrote:
> >> >  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.

No, data_offset is reset at the end of the block. That's not visible in the
patch. Here ist the relevant hunks with a bit more context:

     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;
+            }
             BLK_READ_BLOCK(sd->data_start, io_len);
+        }
         ret = sd->data[sd->data_offset ++];
 
         if (sd->data_offset >= io_len) {
             sd->data_start += io_len;
             sd->data_offset = 0;
[...]
-
-            if (sd->data_start + io_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
-                break;
-            }
         }
         break;

As you can see, the old check was inside the block that resets data_offset
to zero. This patch just delays exactly that check to the beginning of the
next access.

Regards,
Michael

-- 
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 |