Re: [PATCH v7 05/17] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

[Alistair Francis]

On Tue, Jun 30, 2020 at 6:42 AM Philippe Mathieu-Daudé <f4bug@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 <pjp@fedoraproject.org>
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1880822
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

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