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.
> }
> +
> + 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
>
