target/arm: Fix ptw parameters in S1_ptw_translate() for debug contexts

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.
---
 target/arm/ptw.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

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 ?

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

>              .in_debug = true,
>          };
>          GetPhysAddrResult s2 = { };

thanks
-- PMM

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

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

On Thu, Jul 06, 2023 at 04:42:02PM +0100, Peter Maydell wrote:
> > > 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 ?

Uploaded to:
https://jpbrucker.net/tmp/2023-07-06-repro-qemu-tfa.tar.gz

Thanks,
Jean

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

On Thu, 6 Jul 2023 at 17:10, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> On Thu, Jul 06, 2023 at 04:42:02PM +0100, Peter Maydell wrote:
> > > > 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 ?
>
> Uploaded to:
> https://jpbrucker.net/tmp/2023-07-06-repro-qemu-tfa.tar.gz

Thanks, I've got that and can repro the failure. I probably won't
be able to get a patch sorted before Monday, I'm afraid.

-- PMM

W dniu 6.07.2023 o 17:25, Jean-Philippe Brucker pisze:

> (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.)

Which reminds me that qemu/qemu_sbsa targets are still out of sync in 
TF-A when it comes to enabled features ;(

On Thu, 6 Jul 2023 at 17:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 6 Jul 2023 at 17:10, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > On Thu, Jul 06, 2023 at 04:42:02PM +0100, Peter Maydell wrote:
> > > > > 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 ?
> >
> > Uploaded to:
> > https://jpbrucker.net/tmp/2023-07-06-repro-qemu-tfa.tar.gz
>
> Thanks, I've got that and can repro the failure. I probably won't
> be able to get a patch sorted before Monday, I'm afraid.

Tentative patch, which works on the test case:

--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -449,7 +449,7 @@ static bool S2_attrs_are_device(uint64_t hcr, uint8_t attrs)
 static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
                              hwaddr addr, ARMMMUFaultInfo *fi)
 {
-    ARMSecuritySpace space = ptw->in_space;
+    ARMSecuritySpace s2_space;
     bool is_secure = ptw->in_secure;
     ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
     ARMMMUIdx s2_mmu_idx = ptw->in_ptw_idx;
@@ -457,6 +457,9 @@ static bool S1_ptw_translate(CPUARMState *env,
S1Translate *ptw,

     ptw->out_virt = addr;

+    s2_space = regime_is_stage2(s2_mmu_idx) ?
+        ptw->in_space : arm_phys_to_space(s2_mmu_idx);
+
     if (unlikely(ptw->in_debug)) {
         /*
          * From gdbstub, do not use softmmu so that we don't modify the
@@ -465,10 +468,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 = arm_space_is_secure(s2_space),
+            .in_space = s2_space,
             .in_debug = true,
         };
         GetPhysAddrResult s2 = { };

But I need to check whether just using the ptw->in_space
as the stage 2 walk space is correct, which will have to
wait til Monday.

-- PMM