[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 05/17] hw/sd/sdcard: Do not switch to ReceivingData if add
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v7 05/17] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid |
Date: |
Tue, 7 Jul 2020 12:24:02 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
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
>>