arch: riscv: reset global pointer on exception #81155
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @cwshu |
npitre
previously approved these changes
Nov 8, 2024
rruuaanng
added
area: RISCV
Add macros to read / write hardware registers. Signed-off-by: Yong Cong Sin <ycsin@meta.com> Signed-off-by: Yong Cong Sin <yongcong.sin@gmail.com>
Reset the gp on exception entry from u-mode to protect the kernel against a possible rogue user thread. Signed-off-by: Yong Cong Sin <yongcong.sin@gmail.com> Signed-off-by: Yong Cong Sin <ycsin@meta.com>
Add a test to make sure that the `gp` global pointer register used for relative addressing when `CONFIG_RISCV_GP` is enabled can't be corrupted by a rogue user thread. Signed-off-by: Yong Cong Sin <ycsin@meta.com> Signed-off-by: Yong Cong Sin <yongcong.sin@gmail.com>
|
Added test |
zephyrbot
requested review from
carlocaione,
dcpleung,
edersondisouza,
fkokosinski,
katsuster,
kgugala,
mgielda,
nashif and
tgorochowik
cfriedt
reviewed
Nov 11, 2024
|
Chris Friedt wrote:
Is the point here to illustrate that we _can_ use it from within uninterruptible context?
If `CONFIG_RISCV_GP` is set then gp is used by all the code compiled by gcc to reference global variables.
I let you imagine how futile the security model is if user threads may mess with it.
|
npitre
approved these changes
Nov 12, 2024
|
Just curious if we back up / restore GP already? If not, we may want to consider doing so. |
|
On Tue, 12 Nov 2024, Chris Friedt wrote:
@npitre, let me know if this aligns with your suggestion as well. I
understand it's adding one more register to push / pop, but it's maybe
worth it?
No it is not.
The gp, when so instructed by the build system, is relied upon by the
linker to produce tighter code for addressing global variables. In such
case it stands for "Global Pointer" and not "General Purpose". It is
then used potentially everywhere a relocation is involved: isr, thread
context, user context, etc. It is assumed by the linker to hold the same
value everywhere, be set once and never modified afterwards. You cannot
just accidentally "corrupt it (you're likely to crash right away). Even
the assembler needs this norelax mode to touch it otherwise it complains
with an error.
Please see
https://www.sifive.com/blog/all-aboard-part-3-linker-relaxation-in-riscv-toolchain
for more details.
So _preserving_ the gp is completely useless and wasteful. Restoring it
is also wasteful _unless_ untrusted code may be involved. That untrusted
code could potentially create a privilege elevation attack with a
carefully chosen gp value and syscall combination to trick the
privileged kernel code into updating the wrong global variable. That's
the reason why exiting user mode must re-set the gp value.
Now if we had an hypervisor switching between different OSes then
obviously that hypervisor would have to save and restore the gp value as
it is going to differ between different systems. But within a given
environment it is not meant to change.
|
fkokosinski
approved these changes
Nov 13, 2024
cfriedt
approved these changes
Nov 13, 2024
cfriedt
added
Security Review
ceolin
approved these changes
Nov 14, 2024
Agree ! this should be merged ASAP imho. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reset the gp on exception entry from u-mode to protect the kernel against a possible rogue user thread when global pointer (GP) relative addressing is enabled (
CONFIG_RISCV_GP=y).Fixes #81372