On 7/7/20 12:24 PM, Philippe Mathieu-Daudé wrote: > 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'll go with Peter's suggestion from IRC: "insist the blk device is the right size and make it an error if not". > > 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) { > ---