| Message ID | 20240221-arm32-lpae-pan-v2-3-991096bba5d8@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PAN for ARM32 using LPAE | expand |
On Wed, Feb 21, 2024 at 12:04:02AM +0100, Linus Walleij wrote: > @@ -24,9 +24,10 @@ > * perform such accesses (eg, via list poison values) which could then > * be exploited for priviledge escalation. > */ > +#if defined(CONFIG_CPU_SW_DOMAIN_PAN) > + > static __always_inline unsigned int uaccess_save_and_enable(void) > { > -#ifdef CONFIG_CPU_SW_DOMAIN_PAN This is an interesting way to reduce the #ifdef count... why switch it from #ifdef to #if defined ?
On Mon, Mar 11, 2024 at 5:02 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Wed, Feb 21, 2024 at 12:04:02AM +0100, Linus Walleij wrote: > > @@ -24,9 +24,10 @@ > > * perform such accesses (eg, via list poison values) which could then > > * be exploited for priviledge escalation. > > */ > > +#if defined(CONFIG_CPU_SW_DOMAIN_PAN) > > + > > static __always_inline unsigned int uaccess_save_and_enable(void) > > { > > -#ifdef CONFIG_CPU_SW_DOMAIN_PAN > > This is an interesting way to reduce the #ifdef count... why switch it > from #ifdef to #if defined ? Hm that looks like a development artifact from a point where we has if defined(A) && defined(B) and then one of them was removed. I'll fix it up and put an updated version in the patch tracker. Yours, Linus Walleij
On Tue, Mar 12, 2024 at 9:22 AM Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Mar 11, 2024 at 5:02 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Wed, Feb 21, 2024 at 12:04:02AM +0100, Linus Walleij wrote: > > > @@ -24,9 +24,10 @@ > > > * perform such accesses (eg, via list poison values) which could then > > > * be exploited for priviledge escalation. > > > */ > > > +#if defined(CONFIG_CPU_SW_DOMAIN_PAN) > > > + > > > static __always_inline unsigned int uaccess_save_and_enable(void) > > > { > > > -#ifdef CONFIG_CPU_SW_DOMAIN_PAN > > > > This is an interesting way to reduce the #ifdef count... why switch it > > from #ifdef to #if defined ? > > Hm that looks like a development artifact from a point where we > has if defined(A) && defined(B) and then one of them was removed. Ah no, now I see it. In the next patch a pattern with #elif defined(CONFIG_CPU_TTBR0_PAN) is used, so in order to not look confusing the #elif defined() is paired with #if defined(). I think the C++ preprocessor supports #elifdef but with plain cpp we are left with this. (At least last time I checked.) The commit message should be clearer on this though, so I need to fix that. Yours, Linus Walleij
On Tue, Mar 12, 2024, at 09:30, Linus Walleij wrote: > > I think the C++ preprocessor supports #elifdef but with plain cpp > we are left with this. (At least last time I checked.) I thought I read something about it when I experimentally built --std=gnu2x. Checking again, I see #elifdef is supported with both gcc-12 and clang-13, but is silently ignored on older versions, so we won't be able to use it for a while. Arnd
On Tue, Mar 12, 2024 at 09:30:37AM +0100, Linus Walleij wrote: > On Tue, Mar 12, 2024 at 9:22 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Mar 11, 2024 at 5:02 PM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > On Wed, Feb 21, 2024 at 12:04:02AM +0100, Linus Walleij wrote: > > > > @@ -24,9 +24,10 @@ > > > > * perform such accesses (eg, via list poison values) which could then > > > > * be exploited for priviledge escalation. > > > > */ > > > > +#if defined(CONFIG_CPU_SW_DOMAIN_PAN) > > > > + > > > > static __always_inline unsigned int uaccess_save_and_enable(void) > > > > { > > > > -#ifdef CONFIG_CPU_SW_DOMAIN_PAN > > > > > > This is an interesting way to reduce the #ifdef count... why switch it > > > from #ifdef to #if defined ? > > > > Hm that looks like a development artifact from a point where we > > has if defined(A) && defined(B) and then one of them was removed. > > Ah no, now I see it. In the next patch a pattern with > > #elif defined(CONFIG_CPU_TTBR0_PAN) > > is used, so in order to not look confusing the #elif defined() is paired > with #if defined(). Looking closer, there is inconsistency in patch 2 and patch 3. In patch 2, uaccess-asm.h, you move the #ifdef without changing it, and in the other files it becomes #if defined. In patch 3, you add #elif defined to all these files. So in uaccess-asm.h, we end up with: #ifdef #elif defined whereas others we have: #if defined #elif defined So I think uaccess-asm.h needs to be changed as well. It probably would make more sense in the patches if patch 2 if the #ifdef's were moved without changing them, and in patch 3 which introduces the #elif defined, to also then change the #ifdef to #if defined. Then the commit messages don't need modification because it's obvious why the change if #ifdef is happening.
diff --git a/arch/arm/include/asm/uaccess-asm.h b/arch/arm/include/asm/uaccess-asm.h index 65da32e1f1c1..ea42ba25920f 100644 --- a/arch/arm/include/asm/uaccess-asm.h +++ b/arch/arm/include/asm/uaccess-asm.h @@ -39,8 +39,9 @@ #endif .endm - .macro uaccess_disable, tmp, isb=1 #ifdef CONFIG_CPU_SW_DOMAIN_PAN + + .macro uaccess_disable, tmp, isb=1 /* * Whenever we re-enter userspace, the domains should always be * set appropriately. @@ -50,11 +51,9 @@ .if \isb instr_sync .endif -#endif .endm .macro uaccess_enable, tmp, isb=1 -#ifdef CONFIG_CPU_SW_DOMAIN_PAN /* * Whenever we re-enter userspace, the domains should always be * set appropriately. @@ -64,9 +63,18 @@ .if \isb instr_sync .endif -#endif .endm +#else + + .macro uaccess_disable, tmp, isb=1 + .endm + + .macro uaccess_enable, tmp, isb=1 + .endm + +#endif + #if defined(CONFIG_CPU_SW_DOMAIN_PAN) || defined(CONFIG_CPU_USE_DOMAINS) #define DACR(x...) x #else diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h index 9556d04387f7..9b9234d1bb6a 100644 --- a/arch/arm/include/asm/uaccess.h +++ b/arch/arm/include/asm/uaccess.h @@ -24,9 +24,10 @@ * perform such accesses (eg, via list poison values) which could then * be exploited for priviledge escalation. */ +#if defined(CONFIG_CPU_SW_DOMAIN_PAN) + static __always_inline unsigned int uaccess_save_and_enable(void) { -#ifdef CONFIG_CPU_SW_DOMAIN_PAN unsigned int old_domain = get_domain(); /* Set the current domain access to permit user accesses */ @@ -34,19 +35,27 @@ static __always_inline unsigned int uaccess_save_and_enable(void) domain_val(DOMAIN_USER, DOMAIN_CLIENT)); return old_domain; -#else - return 0; -#endif } static __always_inline void uaccess_restore(unsigned int flags) { -#ifdef CONFIG_CPU_SW_DOMAIN_PAN /* Restore the user access mask */ set_domain(flags); -#endif } +#else + +static inline unsigned int uaccess_save_and_enable(void) +{ + return 0; +} + +static inline void uaccess_restore(unsigned int flags) +{ +} + +#endif + /* * These two are intentionally not defined anywhere - if the kernel * code generates any references to them, that's a bug. diff --git a/arch/arm/lib/csumpartialcopyuser.S b/arch/arm/lib/csumpartialcopyuser.S index 6928781e6bee..04d8d9d741c7 100644 --- a/arch/arm/lib/csumpartialcopyuser.S +++ b/arch/arm/lib/csumpartialcopyuser.S @@ -13,7 +13,8 @@ .text -#ifdef CONFIG_CPU_SW_DOMAIN_PAN +#if defined(CONFIG_CPU_SW_DOMAIN_PAN) + .macro save_regs mrc p15, 0, ip, c3, c0, 0 stmfd sp!, {r1, r2, r4 - r8, ip, lr} @@ -25,7 +26,9 @@ mcr p15, 0, ip, c3, c0, 0 ret lr .endm + #else + .macro save_regs stmfd sp!, {r1, r2, r4 - r8, lr} .endm @@ -33,6 +36,7 @@ .macro load_regs ldmfd sp!, {r1, r2, r4 - r8, pc} .endm + #endif .macro load1b, reg1