On Tue, 7 Jul 2020 at 17:04, Alistair Francis <alistai...@gmail.com> wrote: > > On Tue, Jul 7, 2020 at 6:22 AM Philippe Mathieu-Daudé <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> > > --- > > 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" ? thanks -- PMM