On Tue, Sep 2, 2025 at 11:39 AM Jan Kiszka <jan.kis...@siemens.com> wrote:
> 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: <= > Ah, right. > I have a patch under test for staging that also enforces the 512-byte rule. > Yes. But that rule only applies to MMC cards and not SD cards which have a 512k rule since they don't have the EXT_CSD that MMC cards have. The qemu stack supports both kinds of cards. But I've not traced through app the ACMD support to see if we record this difference or not. Warner > 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 >
