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

[Philippe Mathieu-Daudé]

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 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
>>