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

[Peter Maydell]

On Thu, 6 Jul 2023 at 16:25, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> 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

Could you put the necessary binary blobs up somewhere, to save
me trying to rebuild TF-A ?


> > > ---
> > >  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

Yes, I agree. I'm not sure your proposed fix is the right one,
though. I need to re-work through what I did in fcc0b0418fff
to remind myself of what the various fields in a S1Translate
struct are supposed to be, but I think .in_secure (and now
.in_space) are supposed to always match .in_mmu_idx, and
that's not necessarily the same as what the local is_secure
holds. (is_secure is the original ptw's in_secure, which
matches that ptw's .in_mmu_idx, not its .in_ptw_idx.)
So probably the right thing for the .in_secure check is
to change to "(s2_mmu_idx == ARMMMUIdx_Stage2_S ||
s2_mmu_idx == ARMMMUIdx_Phys_S)". Less sure about .in_space,
because that conditional is a bit more complicated.

thanks
-- PMM