On 02.09.25 20:09, Philippe Mathieu-Daudé wrote:
> 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.
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
