hw/sd/sd.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-)
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(-)
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()
};
--
2.38.1
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
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2024 Red Hat, Inc.