Re: [PATCH] target/arm: Fix ptw parameters in S1_ptw_translate() for debug contexts

[Jean-Philippe Brucker]

On Thu, Jul 06, 2023 at 03:28:32PM +0100, Peter Maydell wrote:
> On Thu, 6 Jul 2023 at 15:12, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > Arm TF-A fails to boot via semihosting following a recent change to the
> > MMU code. Semihosting attempts to read parameters passed by TF-A in
> > secure RAM via cpu_memory_rw_debug(). While performing the S1
> > translation, we call S1_ptw_translate() on the page table descriptor
> > address, with an MMU index of ARMMMUIdx_Phys_S. At the moment
> > S1_ptw_translate() doesn't interpret this as a secure access, and as a
> > result we attempt to read the page table descriptor from the non-secure
> > address space, which fails.
> >
> > Fixes: fe4a5472ccd6 ("target/arm: Use get_phys_addr_with_struct in 
> > S1_ptw_translate")
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> > I'm not entirely sure why the semihosting parameters are accessed
> > through stage-1 translation rather than directly as physical addresses,
> > but I'm not familiar with semihosting.
> 
> The semihosting ABI says the guest code should pass "a pointer
> to the parameter block". It doesn't say explicitly, but the
> straightforward interpretation is "a pointer that the guest
> itself could dereference to read/write the values", which means
> a virtual address, not a physical one. It would be pretty
> painful for the guest to have to figure out "what is the
> physaddr for this virtual address" to pass it to the semihosting
> call.
> 
> Do you have a repro case for this bug? Did it work
> before commit fe4a5472ccd6 ?

Yes I bisected to fe4a5472ccd6 by trying to run TF-A, following
instructions here:
https://github.com/ARM-software/arm-trusted-firmware/blob/master/docs/plat/qemu.rst

Building TF-A (HEAD 8e31faa05):
make -j CROSS_COMPILE=aarch64-linux-gnu- 
BL33=edk2/Build/ArmVirtQemuKernel-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd PLAT=qemu 
DEBUG=1 LOG_LEVEL=40 all fip

Installing it to QEMU runtime dir:
ln -sf tf-a/build/qemu/debug/bl1.bin build/qemu-cca/run/
ln -sf tf-a/build/qemu/debug/bl2.bin build/qemu-cca/run/
ln -sf tf-a/build/qemu/debug/bl31.bin build/qemu-cca/run/
ln -sf edk2/Build/ArmVirtQemuKernel-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd 
build/qemu-cca/run/bl33.bin

Running QEMU with HEAD=fe4a5472cc:
qemu-system-aarch64 -M virt,secure=on -cpu max -m 1G -nographic -bios bl1.bin 
-semihosting-config enable=on,target=native -d guest_errors

    NOTICE:  Booting Trusted Firmware
    NOTICE:  BL1: v2.9(debug):v2.9.0-280-g8e31faa05
    NOTICE:  BL1: Built : 16:23:20, Jul  6 2023
    INFO:    BL1: RAM 0xe0ee000 - 0xe0f6000
    INFO:    BL1: Loading BL2
    WARNING: Firmware Image Package header check failed.
    Invalid read at addr 0xE0EF900, size 8, region '(null)', reason: rejected
    WARNING: Failed to obtain reference to image id=1 (-2)
    ERROR:   Failed to load BL2 firmware.

with HEAD=4a7d7702cd:
    ...
    INFO:    BL1: Loading BL2
    WARNING: Firmware Image Package header check failed.
    INFO:    Loading image id=1 at address 0xe06b000
    INFO:    Image id=1 loaded: 0xe06b000 - 0xe073201
    NOTICE:  BL1: Booting BL2
    INFO:    Entry point address = 0xe06b000
    INFO:    SPSR = 0x3c5
    ...


(Note that there is an issue with TF-A missing ENABLE_FEAT_FGT for qemu at
the moment, which prevents booting Linux with -cpu max. I'll send the fix
to TF-A after this, but this reproducer should at least boot edk2.)

> > ---
> >  target/arm/ptw.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> > index 9aaff1546a..e3a738c28e 100644
> > --- a/target/arm/ptw.c
> > +++ b/target/arm/ptw.c
> > @@ -465,10 +465,8 @@ static bool S1_ptw_translate(CPUARMState *env, 
> > S1Translate *ptw,
> >          S1Translate s2ptw = {
> >              .in_mmu_idx = s2_mmu_idx,
> >              .in_ptw_idx = ptw_idx_for_stage_2(env, s2_mmu_idx),
> > -            .in_secure = s2_mmu_idx == ARMMMUIdx_Stage2_S,
> > -            .in_space = (s2_mmu_idx == ARMMMUIdx_Stage2_S ? ARMSS_Secure
> > -                         : space == ARMSS_Realm ? ARMSS_Realm
> > -                         : ARMSS_NonSecure),
> > +            .in_secure = is_secure,
> > +            .in_space = space,
> 
> If the problem is fe4a5472ccd6 then this seems an odd change to
> be making, because in_secure and in_space were set that way
> before that commit too...

I think that commit merged both sides of the
"regime_is_stage2(s2_mmu_idx)" test, but only kept testing for secure
through ARMMMUIdx_Stage2_S, and removed the test through ARMMMUIdx_Phys_S

Thanks,
Jean