On 7/7/20 10:30 AM, Philippe Mathieu-Daudé wrote: > On Tue, Jun 30, 2020 at 3:39 PM Philippe Mathieu-Daudé <f4...@amsat.org> > wrote: >> >> Only move the state machine to ReceivingData if there is no >> pending error. This avoids later OOB access while processing >> commands queued. >> >> "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01" >> >> 4.3.3 Data Read >> >> Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR >> occurred and no data transfer is performed. >> >> 4.3.4 Data Write >> >> Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR >> occurred and no data transfer is performed. >> >> WP_VIOLATION errors are not modified: the error bit is set, we >> stay in receive-data state, wait for a stop command. All further >> data transfer is ignored. See the check on sd->card_status at the >> beginning of sd_read_data() and sd_write_data(). >> >> Fixes: CVE-2020-13253 >> Cc: Prasad J Pandit <p...@fedoraproject.org> >> Reported-by: Alexander Bulekov <alx...@bu.edu> >> Buglink: https://bugs.launchpad.net/qemu/+bug/1880822 >> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org> >> --- >> v4: Only modify ADDRESS_ERROR, not WP_VIOLATION (pm215) >> --- >> hw/sd/sd.c | 34 ++++++++++++++++++++++------------ >> 1 file changed, 22 insertions(+), 12 deletions(-) >> >> diff --git a/hw/sd/sd.c b/hw/sd/sd.c >> index 04451fdad2..7e0d684aca 100644 >> --- a/hw/sd/sd.c >> +++ b/hw/sd/sd.c >> @@ -1167,13 +1167,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, >> SDRequest req) >> case 17: /* CMD17: READ_SINGLE_BLOCK */ >> switch (sd->state) { >> case sd_transfer_state: >> - sd->state = sd_sendingdata_state; >> - sd->data_start = addr; >> - sd->data_offset = 0; >> >> if (sd->data_start + sd->blk_len > sd->size) { >> sd->card_status |= ADDRESS_ERROR; >> + return sd_r1; >> } >> + >> + sd->state = sd_sendingdata_state; >> + sd->data_start = addr; >> + sd->data_offset = 0; >> return sd_r1; >> >> default: >> @@ -1184,13 +1186,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, >> SDRequest req) >> case 18: /* CMD18: READ_MULTIPLE_BLOCK */ >> switch (sd->state) { >> case sd_transfer_state: >> - sd->state = sd_sendingdata_state; >> - sd->data_start = addr; >> - sd->data_offset = 0; >> >> if (sd->data_start + sd->blk_len > sd->size) { >> sd->card_status |= ADDRESS_ERROR; >> + return sd_r1; > > Unfortunately this breaks guests (Linux at least) when sd->size is not > a power of 2, > as Linux doesn't expect unrealistic SD card sizes.
I can use blk_truncate() to expand the image (which must be RW anyway) to the ceil pow2 with something like: -- >8 -- diff --git a/hw/sd/sd.c b/hw/sd/sd.c index b44999c864..052934f867 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -2121,11 +2121,28 @@ static void sd_realize(DeviceState *dev, Error **errp) } if (sd->blk) { + int64_t blk_size; + if (blk_is_read_only(sd->blk)) { error_setg(errp, "Cannot use read-only drive as SD card"); return; } + blk_size = blk_getlength(sd->blk); + if (blk_size > 0) { + int64_t blk_size_aligned = pow2ceil(blk_size); + + if (blk_size != blk_size_aligned) { + ret = blk_truncate(sd->blk, blk_size_aligned, false, + PREALLOC_MODE_FALLOC, + BDRV_REQ_ZERO_WRITE, errp); + if (ret < 0) { + error_prepend(errp, "Could not resize image: "); + return; + } + } + } + ret = blk_set_perm(sd->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE, BLK_PERM_ALL, errp); if (ret < 0) { --- > >> } >> + >> + sd->state = sd_sendingdata_state; >> + sd->data_start = addr; >> + sd->data_offset = 0; >> return sd_r1; >> >> default: >> @@ -1230,14 +1234,17 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, >> SDRequest req) >> /* Writing in SPI mode not implemented. */ >> if (sd->spi) >> break; >> + >> + if (sd->data_start + sd->blk_len > sd->size) { >> + sd->card_status |= ADDRESS_ERROR; >> + return sd_r1; >> + } >> + >> sd->state = sd_receivingdata_state; >> sd->data_start = addr; >> sd->data_offset = 0; >> sd->blk_written = 0; >> >> - if (sd->data_start + sd->blk_len > sd->size) { >> - sd->card_status |= ADDRESS_ERROR; >> - } >> if (sd_wp_addr(sd, sd->data_start)) { >> sd->card_status |= WP_VIOLATION; >> } >> @@ -1257,14 +1264,17 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, >> SDRequest req) >> /* Writing in SPI mode not implemented. */ >> if (sd->spi) >> break; >> + >> + if (sd->data_start + sd->blk_len > sd->size) { >> + sd->card_status |= ADDRESS_ERROR; >> + return sd_r1; >> + } >> + >> sd->state = sd_receivingdata_state; >> sd->data_start = addr; >> sd->data_offset = 0; >> sd->blk_written = 0; >> >> - if (sd->data_start + sd->blk_len > sd->size) { >> - sd->card_status |= ADDRESS_ERROR; >> - } >> if (sd_wp_addr(sd, sd->data_start)) { >> sd->card_status |= WP_VIOLATION; >> } >> -- >> 2.21.3 >>