[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:34:27 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
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) {
> ---