| Message ID | 8d5438da7174ecb32e1c28cdb49987648df6ef15.1685963081.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4] powerpc/kuap: Avoid unnecessary reads of MD_AP | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
On Mon Jun 5, 2023 at 9:04 PM AEST, Christophe Leroy wrote: > It is possible to disable KUAP at boottime with 'nosmap' parameter. > > That is implemented with jump_label hence adds a 'nop' in front > of each open/close of userspace access. > > From a security point of view it makes sence to disallow disabling > KUAP. And on processors like the 8xx where 'nop' is not seamless, > it saves a few cycles. > > So add a CONFIG item to make it optionnal. I love counting cycles, but a CONFIG option for one nop... I think you have me beat. Is that sustainable? What if instead of the static branch you patched in nops to the lock/unlock asm? AFAIKS there does not look like much (any?) C code in the kuap enabled branches. Thanks, Nick > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/include/asm/kup.h | 2 +- > arch/powerpc/mm/init-common.c | 3 +++ > arch/powerpc/platforms/Kconfig.cputype | 10 ++++++++++ > 3 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h > index 74b7f4cee2ed..f3280169aeec 100644 > --- a/arch/powerpc/include/asm/kup.h > +++ b/arch/powerpc/include/asm/kup.h > @@ -53,7 +53,7 @@ extern struct static_key_false disable_kuap_key; > > static __always_inline bool kuap_is_disabled(void) > { > - return static_branch_unlikely(&disable_kuap_key); > + return IS_ENABLED(CONFIG_PPC_KUAP_BOOTTIME) && static_branch_unlikely(&disable_kuap_key); > } > #endif > #else > diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c > index 74e140b1efef..994ee58f0092 100644 > --- a/arch/powerpc/mm/init-common.c > +++ b/arch/powerpc/mm/init-common.c > @@ -48,6 +48,9 @@ early_param("nosmep", parse_nosmep); > > static int __init parse_nosmap(char *p) > { > + if (!IS_ENABLED(CONFIG_PPC_KUAP_BOOTTIME)) > + return 0; > + > disable_kuap = true; > pr_warn("Disabling Kernel Userspace Access Protection\n"); > return 0; > diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype > index 45fd975ef521..f75c2d5cd182 100644 > --- a/arch/powerpc/platforms/Kconfig.cputype > +++ b/arch/powerpc/platforms/Kconfig.cputype > @@ -502,6 +502,16 @@ config PPC_KUAP > > If you're unsure, say Y. > > +config PPC_KUAP_BOOTTIME > + bool "Allow disabling Kernel Userspace Access Protection at boottime" > + depends on PPC_KUAP > + default y > + help > + Allow the user to disable Kernel Userspace Access Protection (KUAP) > + at boot time using 'nosmap' kernel parameter. > + > + If you're unsure, say Y. > + > config PPC_KUAP_DEBUG > bool "Extra debugging for Kernel Userspace Access Protection" > depends on PPC_KUAP > -- > 2.40.1
Le 06/06/2023 à 11:27, Nicholas Piggin a écrit : > On Mon Jun 5, 2023 at 9:04 PM AEST, Christophe Leroy wrote: >> It is possible to disable KUAP at boottime with 'nosmap' parameter. >> >> That is implemented with jump_label hence adds a 'nop' in front >> of each open/close of userspace access. >> >> From a security point of view it makes sence to disallow disabling >> KUAP. And on processors like the 8xx where 'nop' is not seamless, >> it saves a few cycles. >> >> So add a CONFIG item to make it optionnal. > > I love counting cycles, but a CONFIG option for one nop... I think > you have me beat. Unlike most other powerpc, a nop on 8xx is one cycle. So if we can avoid it on hot pathes .... > > Is that sustainable? What if instead of the static branch you patched in > nops to the lock/unlock asm? AFAIKS there does not look like much (any?) > C code in the kuap enabled branches. Yes that's probably the solution at the end. At the moment I'm struggling with UACCESS objtool validation and I think I will temporarily disable boottime 'nosmap' parameter for PPC32. The problem is that with the static branch, objtool visits both branches but it does it independently for each branch, it is not able to match two static branches using the same key. So the result is that it detects UACCESS enable without UACCESS disable and vice-versa. So the solution at the end will be to patch-out the mtspr, for the time being I'm just going to disable it completely for PPC32. Christophe
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h index 74b7f4cee2ed..f3280169aeec 100644 --- a/arch/powerpc/include/asm/kup.h +++ b/arch/powerpc/include/asm/kup.h @@ -53,7 +53,7 @@ extern struct static_key_false disable_kuap_key; static __always_inline bool kuap_is_disabled(void) { - return static_branch_unlikely(&disable_kuap_key); + return IS_ENABLED(CONFIG_PPC_KUAP_BOOTTIME) && static_branch_unlikely(&disable_kuap_key); } #endif #else diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c index 74e140b1efef..994ee58f0092 100644 --- a/arch/powerpc/mm/init-common.c +++ b/arch/powerpc/mm/init-common.c @@ -48,6 +48,9 @@ early_param("nosmep", parse_nosmep); static int __init parse_nosmap(char *p) { + if (!IS_ENABLED(CONFIG_PPC_KUAP_BOOTTIME)) + return 0; + disable_kuap = true; pr_warn("Disabling Kernel Userspace Access Protection\n"); return 0; diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype index 45fd975ef521..f75c2d5cd182 100644 --- a/arch/powerpc/platforms/Kconfig.cputype +++ b/arch/powerpc/platforms/Kconfig.cputype @@ -502,6 +502,16 @@ config PPC_KUAP If you're unsure, say Y. +config PPC_KUAP_BOOTTIME + bool "Allow disabling Kernel Userspace Access Protection at boottime" + depends on PPC_KUAP + default y + help + Allow the user to disable Kernel Userspace Access Protection (KUAP) + at boot time using 'nosmap' kernel parameter. + + If you're unsure, say Y. + config PPC_KUAP_DEBUG bool "Extra debugging for Kernel Userspace Access Protection" depends on PPC_KUAP
It is possible to disable KUAP at boottime with 'nosmap' parameter. That is implemented with jump_label hence adds a 'nop' in front of each open/close of userspace access. From a security point of view it makes sence to disallow disabling KUAP. And on processors like the 8xx where 'nop' is not seamless, it saves a few cycles. So add a CONFIG item to make it optionnal. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- arch/powerpc/include/asm/kup.h | 2 +- arch/powerpc/mm/init-common.c | 3 +++ arch/powerpc/platforms/Kconfig.cputype | 10 ++++++++++ 3 files changed, 14 insertions(+), 1 deletion(-)