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.kis...@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.kis...@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
