On Thu, Jul 9, 2020 at 3:56 PM Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> 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 :) > OK Philipe. Then I'll prepare a patch and try send it to the list somewhere this weekend. > > Note that this was your orangepi-pc acceptance test that catched > this bug! > See https://travis-ci.org/github/philmd/qemu/jobs/705653532#L5672: > > Oh cool, that is great. Looks like it is working pretty well then. But lets be fair, I think it was you that contributed that part ;-) > 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). > OK thanks! > > 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 > > > -- Niek Linnenbank
