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é\ > \<f4...@amsat.org <mailto:f4...@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-f4...@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é <f4...@amsat.org > <mailto:f4...@amsat.org>> wrote: > > On 7/7/20 6:06 PM, Peter Maydell wrote: > > On Tue, 7 Jul 2020 at 17:04, Alistair Francis > <alistai...@gmail.com <mailto:alistai...@gmail.com>> wrote: > >> > >> On Tue, Jul 7, 2020 at 6:22 AM Philippe Mathieu-Daudé > <f4...@amsat.org <mailto:f4...@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é <f4...@amsat.org > <mailto:f4...@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 >