Skip to content

Conversation

@jimmyzhe
Copy link
Contributor

When CONFIG_RISCV_ALWAYS_SWITCH_THROUGH_ECALL is enabled, do_swap() from _Fault triggers a PMP error again if the fault is caused by a PMP violation. This PR handles the PMP settings for faults caused by PMP violations:

  1. For kernel thread stack overflow:
    Remove the previous thread's PMP configuration.

  2. For user thread violates memory protection:
    Configure the PMP for the thread's privileged stack.

Although this bug occurs only when CONFIG_RISCV_ALWAYS_SWITCH_THROUGH_ECALL is enabled, I think these fixes are also compatible when CONFIG_RISCV_ALWAYS_SWITCH_THROUGH_ECALL is disabled.

Fixes: #75959

@npitre
Copy link

npitre commented Jul 16, 2024

A few comments:

  • Please elaborate more in the commit message for "protect privileged stack
    for fault handler" with a list of events illustrating the issue. I still
    don't see where the problem is.

  • I imagine the second patch is a consequence of the first? Please say so in
    the commit log. Otherwise it looks like you add stack guard just to remove
    it later.

  • Couldn't z_riscv_pmp_stackguard_disable() be implemented by simply clearing
    MSTATUS_MPRV?

jimmyzhe added 2 commits July 17, 2024 13:30
When RISCV_ALWAYS_SWITCH_THROUGH_ECALL is enabled, do_swap() enables PMP
checking in is_kernel_syscall.
If a user thread violates memory protection and do_swap() is called from
the fault handler, a PMP error occurs because the thread is in privileged
mode but still using the old user mode PMP setting.

Update the PMP setting to privileged mode for fault handler.
This also enables the stack guard for user thread's privileged stack in
fault handler.

Signed-off-by: Jimmy Zheng <[email protected]>
When RISCV_ALWAYS_SWITCH_THROUGH_ECALL is enabled, do_swap() enables PMP
checking in is_kernel_syscall.
If the PMP stack guard is triggered and do_swap() is called from the
fault handler, a PMP error occurs because the stack usage violates the
previous PMP setting.

Remove the stack guard setting during a stack overflow handler to allow
enabling PMP checking safely in fault handler.

Signed-off-by: Jimmy Zheng <[email protected]>
@jimmyzhe jimmyzhe force-pushed the fixed_riscv_ecall_pmp branch from af8175e to 2f3ae20 Compare July 17, 2024 06:00
@jimmyzhe
Copy link
Contributor Author

@npitre, thank you for your feedback.

Please elaborate more in the commit message for "protect privileged stack
for fault handler" with a list of events illustrating the issue. I still
don't see where the problem is.

The first commit majorly change PMP setting to privileged mode for faults from user mode.
Protect privileged stack just the side effect of this fix.
I have update the commit message to make it clearer.

I imagine the second patch is a consequence of the first? Please say so in
the commit log. Otherwise it looks like you add stack guard just to remove
it later.

These commits are independent to fix 2 different cases:

  1. user thread violates PMP memory protection
    The first commit changes PMP setting to privileged mode for fault triggered from user mode.
  2. kernel thread triggers PMP stack guard
    The second commit removes the thread's PMP setting for fault triggered by stack overflow.

Couldn't z_riscv_pmp_stackguard_disable() be implemented by simply clearing
MSTATUS_MPRV?

Disabling mstatus.mprv is not enough because do_swap() enables mstatus.mprv in is_kernel_syscall when CONFIG_RISCV_ALWAYS_SWITCH_THROUGH_ECALL is enabled.
The previous stack overflow PMP setting triggers an error during the do_swap() call from the fault handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: RISCV RISCV Architecture (32-bit & 64-bit)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

riscv: fault handler violates PMP when RISCV_ALWAYS_SWITCH_THROUGH_ECALL is enable

7 participants