On Fri, May 31, 2024 at 05:24:44PM +0200, Ard Biesheuvel wrote:
> > I'm able to reproduce this even without RME. This code was introduced
> > recently by c98f7f755089 ("ArmVirtPkg: Use dynamic PCD to set the SMCCC
> > conduit"). Maybe Ard (Cc'd) knows what could be going wrong here.
> >
> > A slightly reduced reproducer:
> >
> > $ cd edk2/
> > $ build -b DEBUG -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc
> > $ cd ..
> >
> > $ git clone https://github.com/ARM-software/arm-trusted-firmware.git tf-a
> > $ cd tf-a/
> > $ make -j CROSS_COMPILE=aarch64-linux-gnu- PLAT=qemu DEBUG=1 LOG_LEVEL=40
> > QEMU_USE_GIC_DRIVER=QEMU_GICV3
> > BL33=../edk2/Build/ArmVirtQemuKernel-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd all
> > fip && \
> > dd if=build/qemu/debug/bl1.bin of=flash.bin && \
> > dd if=build/qemu/debug/fip.bin of=flash.bin seek=64 bs=4096
> > $ qemu-system-aarch64 -M virt,virtualization=on,secure=on,gic-version=3
> > -cpu max -m 2G -smp 8 -monitor none -serial mon:stdio -nographic -bios
> > flash.bin
> >
>
> Hmm, this is not something I anticipated.
>
> The problem here is that ArmVirtQemuKernel does not actually support
> dynamic PCDs, so instead, the PCD here is 'patchable', which means
> that the underlying value is just overwritten in the binary image, and
> does not propagate to the rest of the firmware. I assume the write
> ends up targettng a location that does not tolerate this.
Yes, the QemuVirtMemInfoLib declares this region read-only, so we end up
with a permission fault
// Map the FV region as normal executable memory
VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
VirtualMemoryTable[2].VirtualBase = VirtualMemoryTable[2].PhysicalBase;
VirtualMemoryTable[2].Length = FixedPcdGet32 (PcdFvSize);
VirtualMemoryTable[2].Attributes =
ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO;
Making it writable doesn't seem sufficient, since I then get a "HVC issued
at EL2" fault. I'll keep debugging.
Thanks,
Jean
>
> Running ArmVirtQemu or ArmVirtQemuKernel at EL2 has really only ever
> worked by accident, it was simply never intended for that. The fix in
> question was a last minute tweak to prevent some CVE fixes pushed by
> MicroSoft from breaking network boot entirely, and now that the
> release has been made, I guess we should revisit this and fix it
> properly.
>
> So the underlying issue here is that on these platforms, we need to
> decide at runtime whether to use HVC or SMC instructions for SMCCC
> calls. This code attempts to record this into a dynamic PCD once at
> boot, in a way that permits other users of the same library to simply
> hardcode this in the platform definition (given that bare metal
> platforms never need this flexibility).
