| Message ID | 20230717155316.17714-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCH-for-8.1] hw/sd/sdcard: Allow users to bypass the power-of-2 size check | expand |
On Mon, Jul 17, 2023 at 05:53:16PM +0200, Philippe Mathieu-Daudé wrote: > Since we committed a9bcedd15a ("hw/sd/sdcard: Do not allow invalid > SD card sizes") to preclude some guests to access beyond the size > of the card (leading to security issues such CVE-2020-13253), various > users complained this prevent them to run guests potencially well > behaving with non-power-of-2 card sizes. In order to allow them to > experiment with such guests, add a property to disable the pow2 > check. > > Resolves: https://bugs.launchpad.net/qemu/+bug/1910586 > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/297 > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1754 IIUC from skimming those issues, it is more or less agreed that having a power-of-2 check is not the right thing to do in QEMU. We've only kept it because no one has done the work to figure out what the correct solution is so far and we didn't want to leave the CVE open. In theory we might oneday do the correct fix and remove this bogus pow2 check. With that in mind... > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/sd/sd.c | 31 ++++++++++++++++++++++++++++--- > 1 file changed, 28 insertions(+), 3 deletions(-) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index 77a717d355..feada6607a 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -108,6 +108,7 @@ struct SDState { > uint8_t spec_version; > BlockBackend *blk; > bool spi; > + bool bypass_pow2_size_check; > > /* Runtime changeables */ > > @@ -2126,6 +2127,9 @@ static void sd_instance_finalize(Object *obj) > timer_free(sd->ocr_power_timer); > } > > +#define PROP_NAME_BYPASS_POW2_SIZE_CHECK \ > + "allow-unsafe-unsupported-not-power-of-2-size" ...this property is at best a hack caused by our inability to correctly fix the CVE so far. This suggests it ought to have the 'x-' prefix to indicate it isn't our desired long term solution and is liable to change. With regards, Daniel
On Mon, 17 Jul 2023 at 16:53, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Since we committed a9bcedd15a ("hw/sd/sdcard: Do not allow invalid > SD card sizes") to preclude some guests to access beyond the size > of the card (leading to security issues such CVE-2020-13253), various > users complained this prevent them to run guests potencially well > behaving with non-power-of-2 card sizes. In order to allow them to > experiment with such guests, add a property to disable the pow2 > check. > > Resolves: https://bugs.launchpad.net/qemu/+bug/1910586 > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/297 > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1754 > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> I agree with Daniel that this should be an x- property. We still need to figure out what's actually going on here. > @@ -2151,7 +2155,13 @@ static void sd_realize(DeviceState *dev, Error **errp) > } > > blk_size = blk_getlength(sd->blk); > - if (blk_size > 0 && !is_power_of_2(blk_size)) { > + if (sd->bypass_pow2_size_check) { > + warn_report_once("Unsupported property '%s' enabled: some guests" > + " might trigger data corruption and/or crash" > + " (thus this process is vulnerable to" > + " CVE-2020-13253).", > + PROP_NAME_BYPASS_POW2_SIZE_CHECK); If the guest really can trigger an overrun of a QEMU buffer, then we shouldn't be letting users opt in to this behaviour. ("Buggy guest might do something odd that causes it to get confused or crash or just not get data out of the SD card" is fine.) Again, we need to figure out what's actually happening here... > + } else if (blk_size > 0 && !is_power_of_2(blk_size)) { > int64_t blk_size_aligned = pow2ceil(blk_size); > char *blk_size_str; > > @@ -2161,11 +2171,15 @@ static void sd_realize(DeviceState *dev, Error **errp) > > blk_size_str = size_to_str(blk_size_aligned); > error_append_hint(errp, > - "SD card size has to be a power of 2, e.g. %s.\n" > + "SD card size should be a power of 2, e.g. %s.\n" > "You can resize disk images with" > " 'qemu-img resize <imagefile> <new-size>'\n" > "(note that this will lose data if you make the" > - " image smaller than it currently is).\n", > + " image smaller than it currently is).\n" > + "Note: you can disable this check by setting" > + " the '" PROP_NAME_BYPASS_POW2_SIZE_CHECK "'" > + " global property but this is DANGEROUS" > + " and unsupported.\n", This is overly vague. In particular, we shouldn't save the specifics of what the user is opting into for when the user has actually enabled the x- option... thanks -- PMM
diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 77a717d355..feada6607a 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -108,6 +108,7 @@ struct SDState { uint8_t spec_version; BlockBackend *blk; bool spi; + bool bypass_pow2_size_check; /* Runtime changeables */ @@ -2126,6 +2127,9 @@ static void sd_instance_finalize(Object *obj) timer_free(sd->ocr_power_timer); } +#define PROP_NAME_BYPASS_POW2_SIZE_CHECK \ + "allow-unsafe-unsupported-not-power-of-2-size" + static void sd_realize(DeviceState *dev, Error **errp) { SDState *sd = SD_CARD(dev); @@ -2151,7 +2155,13 @@ static void sd_realize(DeviceState *dev, Error **errp) } blk_size = blk_getlength(sd->blk); - if (blk_size > 0 && !is_power_of_2(blk_size)) { + if (sd->bypass_pow2_size_check) { + warn_report_once("Unsupported property '%s' enabled: some guests" + " might trigger data corruption and/or crash" + " (thus this process is vulnerable to" + " CVE-2020-13253).", + PROP_NAME_BYPASS_POW2_SIZE_CHECK); + } else if (blk_size > 0 && !is_power_of_2(blk_size)) { int64_t blk_size_aligned = pow2ceil(blk_size); char *blk_size_str; @@ -2161,11 +2171,15 @@ static void sd_realize(DeviceState *dev, Error **errp) blk_size_str = size_to_str(blk_size_aligned); error_append_hint(errp, - "SD card size has to be a power of 2, e.g. %s.\n" + "SD card size should be a power of 2, e.g. %s.\n" "You can resize disk images with" " 'qemu-img resize <imagefile> <new-size>'\n" "(note that this will lose data if you make the" - " image smaller than it currently is).\n", + " image smaller than it currently is).\n" + "Note: you can disable this check by setting" + " the '" PROP_NAME_BYPASS_POW2_SIZE_CHECK "'" + " global property but this is DANGEROUS" + " and unsupported.\n", blk_size_str); g_free(blk_size_str); @@ -2190,6 +2204,17 @@ static Property sd_properties[] = { * board to ensure that ssi transfers only occur when the chip select * is asserted. */ DEFINE_PROP_BOOL("spi", SDState, spi, false), + /* + * Some guests (at least Linux) consider sizes that are not a power + * of 2 as a firmware bug and round the card size up to the next + * power of 2. For simplicity and security (see CVE-2020-13253) we + * only model guest access to the full drive, so we only accept drives + * having a power-of-2 size. That said, some guests might behave + * correctly with non-power-of-2 cards. Users want to experiment + * booting such guests so we provide a way to disable the check. + */ + DEFINE_PROP_BOOL(PROP_NAME_BYPASS_POW2_SIZE_CHECK, + SDState, bypass_pow2_size_check, false), DEFINE_PROP_END_OF_LIST() };
Since we committed a9bcedd15a ("hw/sd/sdcard: Do not allow invalid SD card sizes") to preclude some guests to access beyond the size of the card (leading to security issues such CVE-2020-13253), various users complained this prevent them to run guests potencially well behaving with non-power-of-2 card sizes. In order to allow them to experiment with such guests, add a property to disable the pow2 check. Resolves: https://bugs.launchpad.net/qemu/+bug/1910586 Resolves: https://gitlab.com/qemu-project/qemu/-/issues/297 Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1754 Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/sd/sd.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-)