Re: [PATCH 2/2] hw/sd/sdcard: Do not allow invalid SD card sizes

[Niek Linnenbank]

Hi Philippe,

Just tried out your patch on latest master, and I noticed I couldn't apply it without getting this error:

$ git am ~/Downloads/patches/\[PATCH\ 2_2\]\ hw_sd_sdcard\:\ Do\ not\ allow\ invalid\ SD\ card\ sizes\ -\ Philippe\ Mathieu-Daudé\ \<f4bug@amsat.org\>\ -\ 2020-07-07\ 1521.eml
Applying: hw/sd/sdcard: Do not allow invalid SD card sizes
error: patch failed: hw/sd/sd.c:2130
error: hw/sd/sd.c: patch does not apply
Patch failed at 0001 hw/sd/sdcard: Do not allow invalid SD card sizes
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

The first patch did go OK. Maybe this one just needs to be rebased, or I made a mistake.

So I manually copy & pasted the change into hw/sd/sd.c to test it.

It looks like the check works, but my concern is that with this change, we will be getting this error on 'off-the-shelf' images as well.

For example, the latest Raspbian image size also isn't a power of two:

$ ./arm-softmmu/qemu-system-arm -M raspi2 -sd ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
WARNING: Image format was not specified for '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and probing guessed raw.
         Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
         Specify the 'raw' format explicitly to remove the restrictions.
qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2 GiB)

If we do decide that the change is needed, I would like to propose that we also give the user some instructions

on how to fix it, maybe some 'dd' command? In my opinion that should also go in some of the documentation file(s),

possibly also in the one for the OrangePi PC at docs/system/arm/orangepi.rst (I can also provide a patch for that if you wish).

Kind regards,

Niek

On Tue, Jul 7, 2020 at 6:11 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

On 7/7/20 6:06 PM, Peter Maydell wrote:
> On Tue, 7 Jul 2020 at 17:04, Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Tue, Jul 7, 2020 at 6:22 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> QEMU allows to create SD card with unrealistic sizes. This could work,
>>> but some guests (at least Linux) consider sizes that are not a power
>>> of 2 as a firmware bug and fix the card size to the next power of 2.
>>>
>>> Before CVE-2020-13253 fix, this would allow OOB read/write accesses
>>> past the image size end.
>>>
>>> CVE-2020-13253 has been fixed as:
>>>
>>>     Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>>>     occurred and no data transfer is performed.
>>>
>>>     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().
>>>
>>> While this is the correct behavior, in case QEMU create smaller SD
>>> cards, guests still try to access past the image size end, and QEMU
>>> considers this is an invalid address, thus "all further data transfer
>>> is ignored". This is wrong and make the guest looping until
>>> eventually timeouts.
>>>
>>> Fix by not allowing invalid SD card sizes.  Suggesting the expected
>>> size as a hint:
>>>
>>>   $ qemu-system-arm -M orangepi-pc -drive file=rootfs.ext2,if=sd,format=raw
>>>   qemu-system-arm: Invalid SD card size: 60 MiB (expecting at least 64 MiB)
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/sd/sd.c | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index cb81487e5c..c45106b78e 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -32,6 +32,7 @@
>>>
>>>  #include "qemu/osdep.h"
>>>  #include "qemu/units.h"
>>> +#include "qemu/cutils.h"
>>>  #include "hw/irq.h"
>>>  #include "hw/registerfields.h"
>>>  #include "sysemu/block-backend.h"
>>> @@ -2130,11 +2131,26 @@ 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 && !is_power_of_2(blk_size)) {
>>> +            int64_t blk_size_aligned = pow2ceil(blk_size);
>>> +            char *blk_size_str = size_to_str(blk_size);
>>> +            char *blk_size_aligned_str = size_to_str(blk_size_aligned);
>>> +
>>> +            error_setg(errp, "Invalid SD card size: %s (expecting at least %s)",
>>> +                       blk_size_str, blk_size_aligned_str);
>>
>> Should we print that we expect a power of 2? This isn't always obvious
>> from the message.
>
> Mmm, I was thinking that. Perhaps
>  "expecting a power of 2, e.g. %s"
> ?

OK, thanks guys!

>
> thanks
> -- PMM
>

--

Niek Linnenbank