1. Patchew
  2. QEMU
  3. View series

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

Michael Olbrich posted 1 patch 8 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170916103523.1482-1-m.olbrich@pengutronix.de
Test checkpatch passed
Test docker passed
Test s390x passed
hw/sd/sd.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
[Qemu-devel] [PATCH v2] hw/sd: fix out-of-bounds check for multi block reads
Posted by Michael Olbrich 8 years, 4 months ago 
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 <m.olbrich@pengutronix.de>
---

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.

 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;
+            }
             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
Re: [Qemu-devel] [PATCH v2] hw/sd: fix out-of-bounds check for multi block reads
Posted by Alistair Francis 8 years, 4 months ago 
On Sat, Sep 16, 2017 at 3:35 AM, Michael Olbrich
<m.olbrich@pengutronix.de> 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 <m.olbrich@pengutronix.de>

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
>
>
Re: [Qemu-devel] [PATCH v2] hw/sd: fix out-of-bounds check for multi block reads
Posted by Michael Olbrich 8 years, 4 months ago 
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:
> > 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 <m.olbrich@pengutronix.de>
> 
> 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 |
Re: [Qemu-devel] [PATCH v2] hw/sd: fix out-of-bounds check for multi block reads
Posted by Alistair Francis 8 years, 4 months ago 
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:
>> > 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 <m.olbrich@pengutronix.de>
>>
>> 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 |
Re: [Qemu-devel] [PATCH v2] hw/sd: fix out-of-bounds check for multi block reads
Posted by Michael Olbrich 8 years, 4 months ago 
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 |
Re: [Qemu-devel] [PATCH v2] hw/sd: fix out-of-bounds check for multi block reads
Posted by Peter Maydell 8 years, 4 months ago 
On 20 September 2017 at 07:19, Michael Olbrich <m.olbrich@pengutronix.de> wrote:
> 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.
> [...]

Alistair, were you planning to provide a reviewed-by: for this
patch (or did you have more review comments on it)?

thanks
-- PMM
Re: [Qemu-devel] [PATCH v2] hw/sd: fix out-of-bounds check for multi block reads
Posted by Alistair Francis 8 years, 4 months ago 
On Mon, Sep 25, 2017 at 12:27 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 20 September 2017 at 07:19, Michael Olbrich <m.olbrich@pengutronix.de> wrote:
>> 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.
>> [...]
>
> Alistair, were you planning to provide a reviewed-by: for this
> patch (or did you have more review comments on it)?

Ah woops, this slipped through. Looks fine to me then.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair

>
> thanks
> -- PMM
Re: [Qemu-devel] [PATCH v2] hw/sd: fix out-of-bounds check for multi block reads
Posted by Peter Maydell 8 years, 4 months ago 
On 25 September 2017 at 22:16, Alistair Francis <alistair23@gmail.com> wrote:
> On Mon, Sep 25, 2017 at 12:27 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> Alistair, were you planning to provide a reviewed-by: for this
>> patch (or did you have more review comments on it)?
>
> Ah woops, this slipped through. Looks fine to me then.
>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Cheers. Should we cc:stable on this too? I'm inclined towards 'yes'.

thanks
-- PMM
Re: [Qemu-devel] [PATCH v2] hw/sd: fix out-of-bounds check for multi block reads
Posted by Alistair Francis 8 years, 4 months ago 
On Mon, Sep 25, 2017 at 3:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 September 2017 at 22:16, Alistair Francis <alistair23@gmail.com> wrote:
>> On Mon, Sep 25, 2017 at 12:27 PM, Peter Maydell
>> <peter.maydell@linaro.org> wrote:
>>> Alistair, were you planning to provide a reviewed-by: for this
>>> patch (or did you have more review comments on it)?
>>
>> Ah woops, this slipped through. Looks fine to me then.
>>
>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>
> Cheers. Should we cc:stable on this too? I'm inclined towards 'yes'.

Yeah, I don't see a reason not to.

Thanks,
Alistair

>
> thanks
> -- PMM
Re: [Qemu-devel] [PATCH v2] hw/sd: fix out-of-bounds check for multi block reads
Posted by Peter Maydell 8 years, 4 months ago 
On 25 September 2017 at 23:53, Alistair Francis <alistair23@gmail.com> wrote:
> On Mon, Sep 25, 2017 at 3:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 25 September 2017 at 22:16, Alistair Francis <alistair23@gmail.com> wrote:
>>> On Mon, Sep 25, 2017 at 12:27 PM, Peter Maydell
>>> <peter.maydell@linaro.org> wrote:
>>>> Alistair, were you planning to provide a reviewed-by: for this
>>>> patch (or did you have more review comments on it)?
>>>
>>> Ah woops, this slipped through. Looks fine to me then.
>>>
>>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>>
>> Cheers. Should we cc:stable on this too? I'm inclined towards 'yes'.
>
> Yeah, I don't see a reason not to.

Applied to target-arm.next, thanks.

-- PMM

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.