Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Jan Kiszka <jan.kiszka@siemens.com>]

On 02.09.25 18:24, Jan Kiszka wrote:
> On 02.09.25 18:20, Philippe Mathieu-Daudé wrote:
>> On 2/9/25 18:14, Philippe Mathieu-Daudé wrote:
>>> On 2/9/25 18:00, Cédric Le Goater wrote:
>>>> On 9/2/25 17:55, Philippe Mathieu-Daudé wrote:
>>>>> On 2/9/25 17:47, Cédric Le Goater wrote:
>>>>>> On 9/2/25 17:45, Philippe Mathieu-Daudé wrote:
>>>>>>> On 2/9/25 17:43, Philippe Mathieu-Daudé wrote:
>>>>>>>> On 2/9/25 17:34, Jan Kiszka wrote:
>>>>>>>>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>>>>>>>>>> On 1/9/25 07:56, Jan Kiszka wrote:
>>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>>
>>>>>>>>>>> The power-of-2 rule applies to the user data area, not the
>>>>>>>>>>> complete
>>>>>>>>>>> block image. The latter can be concatenation of boot partition
>>>>>>>>>>> images
>>>>>>>>>>> and the user data.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>>>> ---
>>>>>>>>>>>    hw/sd/sd.c | 2 +-
>>>>>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>>>>>>>> index 8c290595f0..16aee210b4 100644
>>>>>>>>>>> --- a/hw/sd/sd.c
>>>>>>>>>>> +++ b/hw/sd/sd.c
>>>>>>>>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev,
>>>>>>>>>>> Error
>>>>>>>>>>> **errp)
>>>>>>>>>>>                return;
>>>>>>>>>>>            }
>>>>>>>>>>>    -        blk_size = blk_getlength(sd->blk);
>>>>>>>>>>> +        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;
>>>>>>>>>>
>>>>>>>>>> This seems to break the tests/functional/arm/
>>>>>>>>>> test_aspeed_rainier.py
>>>>>>>>>> test due to mmc-p10bmc-20240617.qcow2 size:
>>>>>>>>>>
>>>>>>>>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -
>>>>>>>>>> display none -
>>>>>>>>>> vga none -chardev socket,id=mon,fd=5 -mon
>>>>>>>>>> chardev=mon,mode=control -
>>>>>>>>>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>>>>>>>>>> chardev:console -drive file=/builds/qemu-project/qemu/
>>>>>>>>>> functional- cache/
>>>>>>>>>> download/
>>>>>>>>>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>>>>>>>>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>>>>>>>>>> SD card size has to be a power of 2, e.g. 16 GiB.
>>>>>>>>>>
>>>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hmm, then the test was always wrong as well. I suspect the
>>>>>>>>> aspeed is
>>>>>>>>> enabling boot partitions by default, and the image was created
>>>>>>>>> to pass
>>>>>>>>> the wrong alignment check. Where / by whom is the image maintained?
>>>>>>>>
>>>>>>>> Cédric Le Goater (Cc'ed).
>>>>>>>>
>>>>>>>> The test comes from:
>>>>>>>> https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb-
>>>>>>>> a85f-09964268533d@kaod.org/
>>>>>>>>
>>>>>>>> Maybe also relevant to your suspicion:
>>>>>>>> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd-
>>>>>>>> c2bf-28950ba48ccb@kaod.org/
>>>
>>> [*]
>>>
>>>>>>>
>>>>>>> Digging further:
>>>>>>> https://lore.kernel.org/qemu-
>>>>>>> devel/9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/
>>>>>>>
>>>>>>
>>>>>> yes commit c078298301a8 might have some impact there.
>>>>>
>>>>> With Jan patch, your script doesn't need anymore the
>>>>>
>>>>>    echo "Fixing size to keep qemu happy..."
>>>>>
>>>>> kludge.
>>>>
>>>> which script ?
>>>
>>> The one you pasted in [*]:
>>>
>>> -- 
>>> #!/bin/sh
>>>
>>> URLBASE=https://jenkins.openbmc.org/view/latest/job/latest-master/
>>> label=docker-builder,target=witherspoon-tacoma/lastSuccessfulBuild/
>>> artifact/openbmc/build/tmp/deploy/images/witherspoon-tacoma/
>>>
>>> IMAGESIZE=128
>>> OUTFILE=mmc.img
>>>
>>> FILES="u-boot.bin u-boot-spl.bin obmc-phosphor-image-witherspoon-
>>> tacoma.wic.xz"
>>>
>>> for file in ${FILES}; do
>>>
>>>      if test -f ${file}; then
>>>          echo "${file}: Already downloaded"
>>>      else
>>>          echo "${file}: Downloading"
>>>          wget -nv ${URLBASE}/${file}
>>>      fi
>>> done
>>>
>>> echo
>>>
>>> echo "Creating empty image..."
>>> dd status=none if=/dev/zero of=${OUTFILE} bs=1M count=${IMAGESIZE}
>>> echo "Adding SPL..."
>>> dd status=none if=u-boot-spl.bin of=${OUTFILE} conv=notrunc
>>> echo "Adding u-boot..."
>>> dd status=none if=u-boot.bin of=${OUTFILE} conv=notrunc bs=1K seek=64
>>> echo "Adding userdata..."
>>> unxz -c obmc-phosphor-image-witherspoon-tacoma.wic.xz | dd
>>> status=progress of=${OUTFILE} conv=notrunc bs=1M seek=2
>>> echo "Fixing size to keep qemu happy..."
>>> truncate --size 16G ${OUTFILE}
>>>
>>> echo "Done!"
>>> echo
>>> echo " qemu-system-arm -M tacoma-bmc -nographic -drive
>>> file=mmc.img,if=sd,index=2,format=raw"
>>> ---
>>
>> FTR the alignment check was added to shut up fuzzed CVEs in commit
>> a9bcedd15a5 ("hw/sd/sdcard: Do not allow invalid SD card sizes"):
>>
>>     QEMU allows to create SD card with unrealistic sizes. This could
>>     work, but some guests (at least Linux) consider sizes that are not
>>     a power of 2 as a firmware bug and fix the card size to the next
>>     power of 2.
>>
>>     While the possibility to use small SD card images has been seen as
>>     a feature, it became a bug with CVE-2020-13253, where the guest is
>>     able to do OOB read/write accesses past the image size end.
>>
>>     In a pair of commits we will fix CVE-2020-13253 as:
>>
>>         Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>>         occurred and no data transfer is performed.
>>
>>         Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>>         occurred and no data transfer is performed.
>>
>>         WP_VIOLATION errors are not modified: the error bit is set, we
>>         stay in receive-data state, wait for a stop command. All further
>>         data transfer is ignored. See the check on sd->card_status at
>>         the beginning of sd_read_data() and sd_write_data().
>>
>>     While this is the correct behavior, in case QEMU create smaller SD
>>     cards, guests still try to access past the image size end, and QEMU
>>     considers this is an invalid address, thus "all further data
>>     transfer is ignored". This is wrong and make the guest looping until
>>     eventually timeouts.
>>
>>     Fix by not allowing invalid SD card sizes (suggesting the expected
>>     size as a hint):
>>
>>       $ qemu-system-arm -M orangepi-pc -drive
>> file=rootfs.ext2,if=sd,format=raw
>>       qemu-system-arm: Invalid SD card size: 60 MiB
>>       SD card size has to be a power of 2, e.g. 64 MiB.
>>       You can resize disk images with 'qemu-img resize <imagefile> <new-
>> size>'
>>       (note that this will lose data if you make the image smaller than
>> it currently is).
>>
>>
>> I expect us to be safe and able to deal with non-pow2 regions if we use
>> QEMUSGList from the "system/dma.h" API. But this is a rework nobody had
>> time to do so far.
> 
> We have to tell two things apart: partitions sizes on the one side and
> backing storage sizes. The partitions sizes are (to my reading) clearly
> defined in the spec, and the user partition (alone!) has to be power of
> 2. The boot and RPMB partitions are multiples of 128K. The sum of them
> all is nowhere limited to power of 2 or even only multiples of 128K.
> 

Re-reading the part of the device capacity, the rules are more complex:
 - power of two up to 2 GB
 - multiple of 512 bytes beyond that

So that power-of-two enforcement was and still is likely too strict.

But I still see no indication, neither in the existing eMMC code of QEMU
nor the spec, that the boot and RPMB partition sizes are included in that.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center