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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.

Regards,

Phil.