Pan fix #6169

jenswi-linaro · 2023-07-11T18:49:49Z

Fixes the missing PSTATE.PAN bit when returning with syscall_sys_return(). Did crash as expected without PR #6161.

gagachang · 2023-07-12T08:22:02Z

I have a question about ldelf_syscall_* functions.
Take ldelf_syscall_open_bin() for example, it seems that the const TEE_UUID *uuid is located within user memory.
If the kernel wants to access it, it needs user-access functions.
Am I correct?

I got the following stack trace when I tried CFG_PAN=y.
I seems that the exception is raised when kernel accesses uuid.

E/TC:0 0 Core data-abort at address 0x40013fa8 (translation fault)
E/TC:0 0 cpu	#0
E/TC:0 0 cause	000000000000000d epc	000000008e0639e2
E/TC:0 0 tval	0000000040013fa8 status	8000000200006100
E/TC:0 0 ra	000000008e064124 sp	0000000000000000
E/TC:0 0 gp	0000000000000000 tp	0000000000000aff
E/TC:0 0 t0	0000000000000001 t1	0000000000000000
E/TC:0 0 t2	0000000000000000 s0	000000008e0d78d0
E/TC:0 0 s1	000000008e0d77c0 a0	000000008e0d7960
E/TC:0 0 a1	0000000000000025 a2	0000000040013fa8
E/TC:0 0 a3	0000000000000050 a4	0000000040013fa8
E/TC:0 0 a5	0000000040013fa8 a5	0000000040013fa8
E/TC:0 0 a6	000000008e074060 a7	0000000000000000
E/TC:0 0 s2	000000008e061254 s3	000000008e0d7810
E/TC:0 0 s4	000000008e0d7810 s5	000000008e0d7830
E/TC:0 0 s6	000000008e099ac0 s7	000000008e0d7830
E/TC:0 0 s8	000000008e00bb5c s9	000000008e0b5220
E/TC:0 0 s10	000000008e0d79d8 s11	000000008e0d7a7f
E/TC:0 0 t3	0000000000000000 t4	0000000000000000
E/TC:0 0 t5	0000000000000000 t6	0000000000000000
E/TC:0 0 TEE load address @ 0x8e000000
E/TC:0 0 Call stack:
E/TC:0 0  0x8e0639e2 uuid2str at optee_os/lib/libutils/ext/snprintk.c:229
E/TC:0 0  0x8e064120 kprintf at optee_os/lib/libutils/ext/snprintk.c:427
E/TC:0 0  0x8e063958 __vsnprintf at optee_os/lib/libutils/ext/snprintk.c:143
E/TC:0 0  0x8e0638f6 vsnprintk at optee_os/lib/libutils/ext/snprintk.c:131
E/TC:0 0  0x8e064d7c trace_vprintf at optee_os/lib/libutils/ext/trace.c:163
E/TC:0 0  0x8e064b26 trace_printf at optee_os/lib/libutils/ext/trace.c:108
E/TC:0 0  0x8e0145ac ldelf_syscall_open_bin at optee_os/core/kernel/ldelf_syscalls.c:142
E/TC:0 0  0x8e00067a scall_do_call at optee_os/core/arch/riscv/kernel/arch_scall_rv.S:32
E/TC:0 0  0x8e001b82 thread_scall_handler at optee_os/core/arch/riscv/kernel/thread_arch.c:126
E/TC:0 0  0x8e001daa thread_user_ecall_handler at optee_os/core/arch/riscv/kernel/thread_arch.c:175
E/TC:0 0  0x8e0021fe thread_exception_handler at optee_os/core/arch/riscv/kernel/thread_arch.c:251
E/TC:0 0  0x8e0022da thread_trap_handler at optee_os/core/arch/riscv/kernel/thread_arch.c:297
E/TC:0 0  0x8e000358 trap_from_user at optee_os/core/arch/riscv/kernel/thread_rv.S:132

jenswi-linaro · 2023-07-12T08:44:22Z

I assume you have removed the enter_user_access() and exit_user_access() from scall_handle_ldelf().
Yes, some fixing is needed for the ldelf syscalls too. I'm currently looking at that, I've also run into the uuid abort when testing some small changes.

gagachang · 2023-07-12T14:29:56Z

I assume you have removed the enter_user_access() and exit_user_access() from scall_handle_ldelf().

Thanks for hint.
After deeper debugging I found it is our trusted-firmware's issue, not OP-TEE.
Now the exception is gone.

seonghp · 2023-07-13T05:32:35Z

Oh, IIUC so it was not fault of the QEMU (it's always easy to blame others), but the SPSR_EL1.PAN was not set properly before eret to EL1 after handling return() or panic() system calls, so the PAN exceptions were not raised. Thank you for catching this.

gagachang · 2023-07-13T09:04:03Z

Did you guys encounter PAN exception on xtest 1033 ?

It seems that in the pass value test, TA provides user buffer to system PTA.
(https://github.com/OP-TEE/optee_test/blob/master/ta/supp_plugin/ta_entry.c#L60)
The system PTA will crash when it accesses that user buffer.

jenswi-linaro · 2023-07-17T08:46:16Z

Did you guys encounter PAN exception on xtest 1033 ?

It seems that in the pass value test, TA provides user buffer to system PTA. (https://github.com/OP-TEE/optee_test/blob/master/ta/supp_plugin/ta_entry.c#L60) The system PTA will crash when it accesses that user buffer.

Oh, another SPSR fabrication problem hiding that. I'll take care of that too in the PR.

gagachang · 2023-07-17T08:58:06Z

Oh, another SPSR fabrication problem hiding that. I'll take care of that too in the PR.

Thanks!

I just found that xtest 2001 has the same problem.
socket_open(), socket_send(), socket_recv() and socket_ioctl() also access user buffers.

jenswi-linaro · 2023-07-17T09:23:16Z

Yeah, I just noticed that too. I'm taking care of that. Then there's also xtest 6001, I'm looking at that at the moment.

jenswi-linaro · 2023-07-18T10:57:27Z

Rewrote "core: arm64: preserve PSTATE.PAN when making SPSR" to address another SPSR fabrication issue. Now we have a common function to fabricate an SPSR based on PSTATE. Also added a few more patches fixing more PAN problems discovered with the SPSR fabrication problem out of the way.

This update fixes all issues with PAN I'm currently aware of. I'm planning to add more fine-grained PAN for ldelf syscalls too, but that will go into another pull request.

jenswi-linaro · 2023-07-18T15:58:14Z

CI timeout should be fixed by OP-TEE/build#665

jenswi-linaro · 2023-07-19T14:50:50Z

All checks has passed now.

jenswi-linaro · 2023-07-19T14:54:11Z

Please help to review these PAN fixes.

seonghp · 2023-07-19T15:34:48Z

Thank you for the work! I will take time to further review the PR.

At the first glance, regarding the commit "core: arm64: preserve PSTATE.PAN when making SPSR", IIUC the function handle_user_mode_panic(), also setup the SPSR, too, but the commit seems to not take into account this case.

seonghp · 2023-07-19T16:13:27Z

Regarding "core: add user buffer to tee_invoke_supp_plugin_rpc()", do we have any call-sites where buf_core is not NULL? Is data_core argument left for this added for future-proofing?

jenswi-linaro · 2023-07-20T06:09:28Z

At the first glance, regarding the commit "core: arm64: preserve PSTATE.PAN when making SPSR", IIUC the function handle_user_mode_panic(), also setup the SPSR, too, but the commit seems to not take into account this case.

Thanks, I'll fix that.

Regarding "core: add user buffer to tee_invoke_supp_plugin_rpc()", do we have any call-sites where buf_core is not NULL? Is data_core argument left for this added for future-proofing?

Yes, that's for future-proofing.

jenswi-linaro · 2023-07-20T06:48:16Z

Update to take care of the handle_user_mode_panic() issue. Let me know if I should squash in the "review" commits.

seonghp · 2023-07-20T07:00:58Z

core/arch/arm/kernel/abort.c

-	daif = (ai->regs->spsr >> SPSR_32_AIF_SHIFT) & SPSR_32_AIF_MASK;
-	/* XXX what about DAIF_D? */


One thing I am not sure about is that if it is okay to remove this line. It seems like el0_sync_abort does not touch the memory at the offset THREAD_ABG_REG_SPSR, but I am just wondering if I am missing something. Other than this, it looks good to me and I think it can be squashed into previous commit.

THREAD_ABT_REG_SPSR is updated at:

optee_os/core/arch/arm/kernel/thread_a64.S

Lines 1027 to 1029 in f7e4fc1

mrs x2, spsr_el1

/* Store spsr, sp_el0 */

stp x2, x3, [sp, #THREAD_ABT_REG_SPSR]

However, there is a problem. We might need to create the new SPSR based on the EL0 SPSR since for instance (some of?) the DAIF bits are masked when taking an exception.

I am wondering if we need to keep the previous EL0 SPSR value here. If the abort_handler() hits the handle_user_mode_panic() then it will never return to the EL0 iiuc, and the previous spsr value has been already printed (in get_fault_type()).

I changed back to the old way of setting the SPSR, except that I preserve the PAN bit in the same way as the DAIF bits.

LGTM as sticking to the old way would introduce less side effects.

jenswi-linaro · 2023-07-20T10:38:38Z

Squashed in the updates and added another sentence to "core: arm64: preserve PSTATE.PAN when making SPSR"

gagachang · 2023-07-21T06:36:01Z

Applied parts of this PR and tested on RISC-V architecture with CFG_PAN=y.
Now we can pass xtest without PAN exception.
Thanks @jenswi-linaro !

+-----------------------------------------------------
26795 subtests of which 0 failed
104 test cases of which 0 failed
0 test cases were skipped

jenswi-linaro · 2023-07-21T09:54:59Z

I noticed that we don't have the PAN bit set while at S-EL0 so SPSR_EL1.PAN isn't set. So we need to copy the PSTATE.PAN bit instead.

seonghp · 2023-07-26T07:29:48Z

I am wondering if this PR is on radar of other maintainers for review.

jforissier · 2023-07-26T09:01:45Z

@seonghp it is on my to-do list and hopefully others can review too, but note this is summer time and some people are on vacation 😉

seonghp · 2023-07-26T09:05:36Z

Good news! Thank you for the confirmation 👍

jforissier

Small fixes in the commit descriptions:

For "core: fix reading result in ldelf_dlopen()": s/start to a use/start to use a/
For "core: syscall_storage_obj_rename(): fix direct user memory access": s/lead a data/lead to a data/
For "core: arm64: preserve PSTATE.PAN when making SPSR": s/only go to/only going to/ plus see one comment in the code below.

With that and for the whole series:

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

One more thing: how come the CI wasn't failing without these fixes? The QEMUv8_check job has a line with CFG_PAN=y and QEMU is started with -cpu max, so PAN is supposed to be active. Is it only because the test cases are not exercising the proper parts of the code?

jforissier · 2023-07-26T13:26:28Z

core/arch/arm/kernel/thread.c

+{
+	uint64_t spsr = SPSR_64(SPSR_64_MODE_EL1, SPSR_64_MODE_SP_EL0, 0);
+
+	spsr = SPSR_64(SPSR_64_MODE_EL1, SPSR_64_MODE_SP_EL0, 0);


Duplicated initialization

jenswi-linaro · 2023-07-31T14:25:48Z

One more thing: how come the CI wasn't failing without these fixes? The QEMUv8_check job has a line with CFG_PAN=y and QEMU is started with -cpu max, so PAN is supposed to be active. Is it only because the test cases are not exercising the proper parts of the code?

It's because "core: arm64: preserve PSTATE.PAN when making SPSR" fixes a few bugs that otherwise effectively disables PAN in some cases.

Adds check_user_access() to simplify checking if a user mode memory buffer may be accessed as expected. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Adds default implementations for copy_to_user_private() and copy_to_user() when CFG_WITH_USER_TA=n. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Adds a user buffer to tee_invoke_supp_plugin_rpc() so direct user memory access can be used when called with a buffer in user memory instead of core memory. tee_invoke_supp_plugin_rpc() can still take a core memory buffer as an argument if needed. PTA_SYSTEM_SUPP_PLUGIN_INVOKE in the system PTA is updated to pass the memref as a user memory buffer instead of a core memory buffer. This fixes a direct privileged memory access to user space memory. Fixes: 4e15432 ("core: Apply finer-grained PAN") Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

The commit 52e7b1a ("core: use user-access functions in ldelf interaction") start to use a bounce buffer to initialize the argument for LDELF_DL_ENTRY_DLSYM. However, it also reads the result of LDELF_DL_ENTRY_DLSYM from the bounce buffer. This is an error since the result of LDELF_DL_ENTRY_DLSYM still remains on the stack used by ldelf. So fix this by reading the result from the ldelf stack. Fixes: 52e7b1a ("core: use user-access functions in ldelf interaction") Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Replaces direct user memory accesses in the socket PTA with copy_to_user() and copy_from_user(). This avoids PAN errors when PAN is active. Fixes: 4e15432 ("core: Apply finer-grained PAN") Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Fixes a direct user memory access in syscall_storage_obj_rename() which can lead to a data abort if PAN is enabled. Fixes: 84f7897 ("core: use user-access functions for storage svc") Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Updates the create(), read(), and write() function pointers in struct ts_store_ops to take a user space buffer in addition to the previous core buffer. Core buffers are normal secure memory while user space buffers should only be accessed using the user_access.h functions. The different FS storage implementations are updated accordingly. Note that the RPMB FS storage implementation resorts to using enter_user_access() and exit_user_access() due to internal complexities. Fixes: 4e15432 ("core: Apply finer-grained PAN") Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Adds the wrapper function read_pan() to read PSTATE.PAN, also adds a SPSR_64_PAN define. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Adds the helper function feat_pan_implemented() to extract the implemented PAN version. No version is 0 so this function can be used tested as a boolean too. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

When setup_unwind_user_mode() prepares to resume execution after syscall_sys_return() or when a thread is suspended a new SPSR is fabricated base on the current PSTATE. Until now when remaining in S-EL1 to fabricate an SPSR only the PSTATE.DAIF bits had to be taken into account. However, with PSTATE.PAN there's yet another bit to consider. Since PSTATE has a few more bits and more may be added as AArch64 evolves this problem is only going to get worse. So implement this in a single internal C function to replace current open codes C and assembly versions. The AArch64 assembly versions of thread_rpc() are renamed to thread_rpc_spsr() to indicate that SPSR is passed in the second argument instead of having it open coded internally in the assembly function. New C wrapper functions are added to preserve the old thread_rpc() interface as needed. handle_user_mode_panic() is still basing its created SPSR on the saved SPSR from S-EL0, but now PAN bit is copied too. Fixes: 6fa59c9 ("arm64: Introduce permissive PAN implementation") Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

jenswi-linaro · 2023-07-31T14:47:54Z

Comments addressed, tag applied.

jforissier · 2023-08-02T09:30:07Z

@gagachang @seonghp any comment?

gagachang · 2023-08-02T10:02:21Z

@gagachang @seonghp any comment?

LGTM, I tested this PR on RISC-V and worked well.

seonghp · 2023-08-02T11:02:18Z

LGTM. Thanks for your work!

jenswi-linaro force-pushed the pan_fix branch from af029fe to 19690bc Compare July 18, 2023 10:54

jenswi-linaro mentioned this pull request Jul 19, 2023

qemu_v8: use ENABLE_SVE_FOR_NS=2 in TF_A_FLAGS OP-TEE/build#667

Merged

seonghp reviewed Jul 20, 2023

View reviewed changes

jenswi-linaro force-pushed the pan_fix branch from 009804d to f7d0f2e Compare July 20, 2023 10:38

jforissier approved these changes Jul 26, 2023

View reviewed changes

jenswi-linaro added 2 commits July 31, 2023 16:29

core: add check_user_access() helper function

9425ee6

Adds check_user_access() to simplify checking if a user mode memory buffer may be accessed as expected. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

core: add default copy_to_user{,_private}()

c84aafc

Adds default implementations for copy_to_user_private() and copy_to_user() when CFG_WITH_USER_TA=n. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

jenswi-linaro added 8 commits July 31, 2023 16:29

arm64: add read_pan() and SPSR_64_PAN

47cde83

Adds the wrapper function read_pan() to read PSTATE.PAN, also adds a SPSR_64_PAN define. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

jenswi-linaro force-pushed the pan_fix branch from d1f493b to 07f94ac Compare July 31, 2023 14:47

jforissier merged commit 0e84f8a into OP-TEE:master Aug 3, 2023
6 checks passed

jenswi-linaro deleted the pan_fix branch August 4, 2023 06:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pan fix #6169

Pan fix #6169

jenswi-linaro commented Jul 11, 2023

gagachang commented Jul 12, 2023

jenswi-linaro commented Jul 12, 2023

gagachang commented Jul 12, 2023

seonghp commented Jul 13, 2023

gagachang commented Jul 13, 2023 •

edited

Loading

jenswi-linaro commented Jul 17, 2023

gagachang commented Jul 17, 2023 •

edited

Loading

jenswi-linaro commented Jul 17, 2023

jenswi-linaro commented Jul 18, 2023

jenswi-linaro commented Jul 18, 2023

jenswi-linaro commented Jul 19, 2023

jenswi-linaro commented Jul 19, 2023

seonghp commented Jul 19, 2023

seonghp commented Jul 19, 2023

jenswi-linaro commented Jul 20, 2023

jenswi-linaro commented Jul 20, 2023

seonghp Jul 20, 2023

jenswi-linaro Jul 20, 2023

seonghp Jul 20, 2023

jenswi-linaro Jul 20, 2023

seonghp Jul 20, 2023

jenswi-linaro commented Jul 20, 2023

gagachang commented Jul 21, 2023

jenswi-linaro commented Jul 21, 2023

seonghp commented Jul 26, 2023

jforissier commented Jul 26, 2023

seonghp commented Jul 26, 2023

jforissier left a comment

jforissier Jul 26, 2023

jenswi-linaro commented Jul 31, 2023

jenswi-linaro commented Jul 31, 2023

jforissier commented Aug 2, 2023

gagachang commented Aug 2, 2023

seonghp commented Aug 2, 2023

		daif = (ai->regs->spsr >> SPSR_32_AIF_SHIFT) & SPSR_32_AIF_MASK;
		/* XXX what about DAIF_D? */

	mrs x2, spsr_el1
	/* Store spsr, sp_el0 */
	stp x2, x3, [sp, #THREAD_ABT_REG_SPSR]

Pan fix #6169

Pan fix #6169

Conversation

jenswi-linaro commented Jul 11, 2023

gagachang commented Jul 12, 2023

jenswi-linaro commented Jul 12, 2023

gagachang commented Jul 12, 2023

seonghp commented Jul 13, 2023

gagachang commented Jul 13, 2023 • edited Loading

jenswi-linaro commented Jul 17, 2023

gagachang commented Jul 17, 2023 • edited Loading

jenswi-linaro commented Jul 17, 2023

jenswi-linaro commented Jul 18, 2023

jenswi-linaro commented Jul 18, 2023

jenswi-linaro commented Jul 19, 2023

jenswi-linaro commented Jul 19, 2023

seonghp commented Jul 19, 2023

seonghp commented Jul 19, 2023

jenswi-linaro commented Jul 20, 2023

jenswi-linaro commented Jul 20, 2023

seonghp Jul 20, 2023

Choose a reason for hiding this comment

jenswi-linaro Jul 20, 2023

Choose a reason for hiding this comment

seonghp Jul 20, 2023

Choose a reason for hiding this comment

jenswi-linaro Jul 20, 2023

Choose a reason for hiding this comment

seonghp Jul 20, 2023

Choose a reason for hiding this comment

jenswi-linaro commented Jul 20, 2023

gagachang commented Jul 21, 2023

jenswi-linaro commented Jul 21, 2023

seonghp commented Jul 26, 2023

jforissier commented Jul 26, 2023

seonghp commented Jul 26, 2023

jforissier left a comment

Choose a reason for hiding this comment

jforissier Jul 26, 2023

Choose a reason for hiding this comment

jenswi-linaro commented Jul 31, 2023

jenswi-linaro commented Jul 31, 2023

jforissier commented Aug 2, 2023

gagachang commented Aug 2, 2023

seonghp commented Aug 2, 2023

gagachang commented Jul 13, 2023 •

edited

Loading

gagachang commented Jul 17, 2023 •

edited

Loading