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 19:20, Warner Losh wrote:
> 
> 
> On Tue, Sep 2, 2025 at 10:40 AM Jan Kiszka <jan.kiszka@siemens.com
> <mailto:jan.kiszka@siemens.com>> wrote:
> 
>     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
>     <mailto: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
>     <mailto:jan.kiszka@siemens.com>>
>     >>>>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org
>     <mailto: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
>     <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-
>     <https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb->
>     >>>>>>>> a85f-09964268533d@kaod.org/ <http://
>     a85f-09964268533d@kaod.org/>
>     >>>>>>>>
>     >>>>>>>> Maybe also relevant to your suspicion:
>     >>>>>>>> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd-
>     <https://lore.kernel.org/qemu-devel/e401d119-402e-0edd->
>     >>>>>>>> c2bf-28950ba48ccb@kaod.org/ <http://
>     c2bf-28950ba48ccb@kaod.org/>
>     >>>
>     >>> [*]
>     >>>
>     >>>>>>>
>     >>>>>>> Digging further:
>     >>>>>>> https://lore.kernel.org/qemu- <https://lore.kernel.org/qemu->
>     >>>>>>>
>     devel/9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/
>     <http://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/ <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
> 
> 
> Kinda. It is power of 2 up to 2GiB, but there were a number of 4GiB cards
> that were not high capacity cards that encoded their size and were otherwise
> low capacity cards. Qemu doesn't need to support that. Its existing capacity
> check should be enough.
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index da5bdd134a..18b3f93965 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2151,7 +2151,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
>          }
> 
>          blk_size = blk_getlength(sd->blk);
> -        if (blk_size > 0 && !is_power_of_2(blk_size)) {
> +        if (blk_size > 0 && (blksize < SDSC_MAX_CAPACITY && !
> is_power_of_2(blk_size)) {

Close but not yet correct: <=

I have a patch under test for staging that also enforces the 512-byte rule.

Jan

>              int64_t blk_size_aligned = pow2ceil(blk_size);
>              char *blk_size_str;
> 
> is what I'm running with, but it should have a second check for 512k
> size if not an ext_csd situation.
> 
> High capacity cards, though have a limitation where it's the number of
> 1024 sectors (which are 512 bytes), so the limit is 512k. It encodes the
> CSD differently than normal capacity cards. Thankfully, we have this in
> our code already.
> 
> And really high capacity cards have an extended structure the size of
> the card is reported in, and that appears to be in sectors.
>  
> 
>     So that power-of-two enforcement was and still is likely too strict.
> 
> 
> Agreed.
> 
> Warner
>  
> 
>     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
> 

-- 
Siemens AG, Foundational Technologies
Linux Expert Center