| Message ID | 20170916103523.1482-1-m.olbrich@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] hw/sd: fix out-of-bounds check for multi block reads | expand |
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 > >
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 > > > > >
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 |
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
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
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
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
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
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
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;
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(-)