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

[Peter Maydell]

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

thanks
-- PMM