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

[Philippe Mathieu-Daudé]

On 7/7/20 10:29 PM, Niek Linnenbank wrote:
> 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 <mailto: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.

Sorry it was not clear on the cover:

  Part 1 is already reviewed:
  https://www.mail-archive.com/qemu-devel@nongnu.org/msg718150.html
  Based-on: <20200630133912.9428-1-f4bug@amsat.org>

This series is based on the "Part 1".

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

On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
This is not in the default Darwin packages.
On Windows I have no clue.

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

Good idea, if you can send that patch that would a precious help,
and I'd include it with the other patches :)

Note that this was your orangepi-pc acceptance test that catched
this bug!
See https://travis-ci.org/github/philmd/qemu/jobs/705653532#L5672:

 CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7), cr=50c5387d
 OF: fdt: Machine model: Xunlong Orange Pi PC
 Kernel command line: printk.time=0 console=ttyS0,115200
root=/dev/mmcblk0 rootwait rw panic=-1 noreboot
 sunxi-mmc 1c0f000.mmc: Linked as a consumer to regulator.2
 sunxi-mmc 1c0f000.mmc: Got CD GPIO
 sunxi-mmc 1c0f000.mmc: initialized, max. request size: 16384 KB
 mmc0: host does not support reading read-only switch, assuming write-enable
 mmc0: Problem switching card into high-speed mode!
 mmc0: new SD card at address 4567
 mmcblk0: mmc0:4567 QEMU! 60.0 MiB
 EXT4-fs (mmcblk0): mounting ext2 file system using the ext4 subsystem
 EXT4-fs (mmcblk0): mounted filesystem without journal. Opts: (null)
 VFS: Mounted root (ext2 filesystem) on device 179:0.
 EXT4-fs (mmcblk0): re-mounted. Opts: block_validity,barrier,user_xattr,acl
 Populating /dev using udev: udevd[204]: starting version 3.2.7
 udevadm settle failed
 done
 udevd[205]: worker [208]
/devices/platform/soc/1c0f000.mmc/mmc_host/mmc0/mmc0:4567/block/mmcblk0
is taking a long time
Runner error occurred: Timeout reached
Original status: ERROR

(I'll add that in the commit description too).

Thanks for your testing/review!

> Kind regards,
> 
> Niek
> 
> 
> On Tue, Jul 7, 2020 at 6:11 PM Philippe Mathieu-Daudé <f4bug@amsat.org
> <mailto: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 <mailto:alistair23@gmail.com>> wrote:
>     >>
>     >> On Tue, Jul 7, 2020 at 6:22 AM Philippe Mathieu-Daudé
>     <f4bug@amsat.org <mailto: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
>     <mailto: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
>