| Message ID | 20171214145255.31545-15-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 14, 2017 at 6:52 AM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > Check for READ_MULTIPLE_BLOCK size limit violation first as opposed to > doing at the end of the command handler. Consider the following > scenario: > > Emulated host driver is trying to read last byte of the last sector > via CMD18/ADMA, so what would happen is the following: > > 1. "ret" is filled with desired byte and "sd->data_offset" is > incremented to 512 by > > ret = sd->data[sd->data_offset ++]; > > 2. sd->data_offset >= io_len becomes true, so > > sd->data_start += io_len; > > moves "sd->data_start" past valid data boundaries. > > 3. as a result "sd->data_start + io_len > sd->size" check becomes true > and sd->card_status is marked with ADDRESS_ERROR, telling emulated > host that the last CMD18 read failed, despite nothing bad/illegal > happening. > > To avoid having this false positive, move out-of-bounds check to > happen before BLK_READ_BLOCK(), so this way it will only trigger if > illegal read is truly about to happen. > Disregard this patch, exactly this fix is already present in latest QEMU master. Sorry for the noise. Thanks, Andrey Smirnov > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: Jason Wang <jasowang@redhat.com> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> > Cc: qemu-devel@nongnu.org > Cc: qemu-arm@nongnu.org > Cc: yurovsky@gmail.com > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > hw/sd/sd.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index ba47bff4db..ce4ef17be3 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -1797,8 +1797,17 @@ 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) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: Trying to read past card's capacity\n", > + __func__); > + 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 +1821,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.3 >
diff --git a/hw/sd/sd.c b/hw/sd/sd.c index ba47bff4db..ce4ef17be3 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -1797,8 +1797,17 @@ 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) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: Trying to read past card's capacity\n", + __func__); + 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 +1821,6 @@ uint8_t sd_read_data(SDState *sd) break; } } - - if (sd->data_start + io_len > sd->size) { - sd->card_status |= ADDRESS_ERROR; - break; - } } break;
Check for READ_MULTIPLE_BLOCK size limit violation first as opposed to doing at the end of the command handler. Consider the following scenario: Emulated host driver is trying to read last byte of the last sector via CMD18/ADMA, so what would happen is the following: 1. "ret" is filled with desired byte and "sd->data_offset" is incremented to 512 by ret = sd->data[sd->data_offset ++]; 2. sd->data_offset >= io_len becomes true, so sd->data_start += io_len; moves "sd->data_start" past valid data boundaries. 3. as a result "sd->data_start + io_len > sd->size" check becomes true and sd->card_status is marked with ADDRESS_ERROR, telling emulated host that the last CMD18 read failed, despite nothing bad/illegal happening. To avoid having this false positive, move out-of-bounds check to happen before BLK_READ_BLOCK(), so this way it will only trigger if illegal read is truly about to happen. Cc: Peter Maydell <peter.maydell@linaro.org> Cc: Jason Wang <jasowang@redhat.com> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> Cc: qemu-devel@nongnu.org Cc: qemu-arm@nongnu.org Cc: yurovsky@gmail.com Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> --- hw/sd/sd.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)