On 02.09.25 19:20, Warner Losh wrote: > > > On Tue, Sep 2, 2025 at 10:40 AM Jan Kiszka <jan.kis...@siemens.com > <mailto:jan.kis...@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.kis...@siemens.com > <mailto:jan.kis...@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.kis...@siemens.com > <mailto:jan.kis...@siemens.com>> > >>>>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org > <mailto:phi...@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-099642685...@kaod.org/ <http:// > a85f-099642685...@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-28950ba48...@kaod.org/ <http:// > c2bf-28950ba48...@kaod.org/> > >>> > >>> [*] > >>> > >>>>>>> > >>>>>>> Digging further: > >>>>>>> https://lore.kernel.org/qemu- <https://lore.kernel.org/qemu-> > >>>>>>> > devel/9046a4327336d4425f1e7e7a973edef9e9948e80.ca...@pengutronix.de/ > <http://9046a4327336d4425f1e7e7a973edef9e9948e80.ca...@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
