Hi Jan,
On 2/9/25 19:47, Jan Kiszka wrote:
From: Jan Kiszka <[email protected]>
There was another mistake in the size check which 8c81d38ea5ae now
revealed: alignment rules depend on the size of the image. Up to and
include 2GB, the power-of-2 rule applies. For larger images, multiples
of 512 sectors must be used. Fix the check accordingly.
Signed-off-by: Jan Kiszka <[email protected]>
---
Not fully tested yet, but as staging is broken right now, I wanted to
provide this already for early feedback.
hw/sd/sd.c | 48 ++++++++++++++++++++++++++++++------------------
1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2d34781fe4..0f33784bd0 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2759,6 +2759,26 @@ static void sd_instance_finalize(Object *obj)
timer_free(sd->ocr_power_timer);
}
+static void blk_size_error(int64_t blk_size, int64_t blk_size_aligned,
+ const char *rule, Error **errp)
+{
+ char *blk_size_str;
+
+ blk_size_str = size_to_str(blk_size);
+ error_setg(errp, "Invalid SD card size: %s", blk_size_str);
+ g_free(blk_size_str);
+
+ blk_size_str = size_to_str(blk_size_aligned);
+ error_append_hint(errp,
+ "SD card size has to be %s, 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",
+ rule, blk_size_str);
+ g_free(blk_size_str);
+}
+
static void sd_realize(DeviceState *dev, Error **errp)
{
SDState *sd = SDMMC_COMMON(dev);
@@ -2782,24 +2802,16 @@ static void sd_realize(DeviceState *dev, Error **errp)
}
blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
- if (blk_size > 0 && !is_power_of_2(blk_size)) {
- int64_t blk_size_aligned = pow2ceil(blk_size);
- char *blk_size_str;
-
- blk_size_str = size_to_str(blk_size);
- error_setg(errp, "Invalid SD card size: %s", blk_size_str);
- g_free(blk_size_str);
-
- 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"
- "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",
- blk_size_str);
First, no rush, your previous patch didn't make it to master
(CI failing) ;)
Again, this painful restriction is due to CVE-2020-13253. Per
merge commit 3a9163af4e3:
Fix CVE-2020-13253
By using invalidated address, guest can do out-of-bounds accesses.
These patches fix the issue by only allowing SD card image sizes
power of 2, and not switching to SEND_DATA state when the address
is invalid (out of range).
This issue was found using QEMU fuzzing mode (using
--enable-fuzzing, see docs/devel/fuzzing.txt) and reported by
Alexander Bulekov.
Reproducer:
https://bugs.launchpad.net/qemu/+bug/1880822/comments/1
Reproducer being:
---
#!/bin/sh
cat << EOF > inp
outl 0xcf8 0x80001810
outl 0xcfc 0xe1068000
outl 0xcf8 0x80001814
outl 0xcf8 0x80001804
outw 0xcfc 0x7
outl 0xcf8 0x8000fa20
write 0xe106802c 0x1 0x6d
write 0xe106800f 0x1 0xf7
write 0xe106800a 0x6 0x9b4b9b5a9b69
write 0xe1068028 0x3 0x6d6d6d
write 0xe106800f 0x1 0x02
write 0xe1068005 0xb 0x055cfbffffff000000ff03
write 0xe106800c 0x1d
0x050bc6c6c6c6c6c6c6c6762e4c5e0bc603040000000000e10200110000
write 0xe1068003 0xd 0x2b6de02c3a6de02c496de02c58
EOF
../bin/qemu-system-x86_64 -qtest stdio -enable-kvm -monitor none \
-serial none -M pc-q35-5.0 -device sdhci-pci,sd-spec-version=3 \
-device sd-card,drive=mydrive -nographic \
-drive if=sd,index=0,file=null-co://,format=raw,id=mydrive < inp
---
I guess remembering we fixed this one then another path was fuzzed,
so we audited and realized restricting to ^2 was the safest option.
I'm not against unrestricting the model, but I don't want we raise new
CVEs. Users adapted truncating their images to pow2 and accepted the
downside.
I'll run some tests, but it might take some time.
Thanks for working on this fix so quickly,
Phil.
- g_free(blk_size_str);
-
+ if (blk_size > 0 && blk_size <= SDSC_MAX_CAPACITY &&
+ !is_power_of_2(blk_size)) {
+ blk_size_error(blk_size, pow2ceil(blk_size), "a power of 2", errp);
+ return;
+ } else if (blk_size > SDSC_MAX_CAPACITY &&
+ blk_size % (1 << HWBLOCK_SHIFT) != 0) {
+ int64_t blk_size_aligned =
+ ((blk_size >> HWBLOCK_SHIFT) + 1) << HWBLOCK_SHIFT;
+ blk_size_error(blk_size, blk_size_aligned, "multiples of 512",
+ errp);
return;
}
