hw/sd/sd.c | 48 ++++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-)
From: Jan Kiszka <jan.kiszka@siemens.com>
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 <jan.kiszka@siemens.com>
---
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);
- 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;
}
--
2.43.0Hi Jan,
On 2/9/25 19:47, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> 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 <jan.kiszka@siemens.com>
> ---
>
> 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;
> }
>
On 02.09.25 20:09, Philippe Mathieu-Daudé wrote:
> Hi Jan,
>
> On 2/9/25 19:47, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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 <jan.kiszka@siemens.com>
>> ---
>>
>> 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.
Problem is that this was completely wrong once boot partition support
was added. I agree that we must not expose more disk than there is
backing for (which was to my current understanding the background of the
CVE), but we need to use the correct rules for that.
But we probably also need to check if the backing disk minus boot and
rpmb partitions is not smaller than 0...
Jan
>
> 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;
>> }
>>
--
Siemens AG, Foundational Technologies
Linux Expert Center
On 02.09.25 20:21, Jan Kiszka wrote:
> On 02.09.25 20:09, Philippe Mathieu-Daudé wrote:
>> Hi Jan,
>>
>> On 2/9/25 19:47, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> 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 <jan.kiszka@siemens.com>
>>> ---
>>>
>>> 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.
>
> Problem is that this was completely wrong once boot partition support
> was added. I agree that we must not expose more disk than there is
> backing for (which was to my current understanding the background of the
> CVE), but we need to use the correct rules for that.
To underline that: An 8 MB eMMC image with 512K boot partitions will
make the guest recognize this as 8MB - although only 7 MB will be
available for the user data area. This is apparently only causing access
errors for the guest now, no longer out-of-bound accesses.
>
> But we probably also need to check if the backing disk minus boot and
> rpmb partitions is not smaller than 0...
>
I've enhanced my original size-check fix accordingly and will include
that in the next version of my series.
Jan
--
Siemens AG, Foundational Technologies
Linux Expert Center
© 2016 - 2025 Red Hat, Inc.