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

[Philippe Mathieu-Daudé]

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é <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>
>>> ---
>>> 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) {
> ---