PAC key changes when kernel code is preempted

PAC key changes when kernel code is preempted

[Derrick McKee]

Hi,

I am noticing that when kernel code is preempted, PAC keys seem to
change when resuming execution.  For instance, when I read
APDAKeyHi_EL1 and APDAKeyLo_EL1, sleep, and read them again, the
values are different.  Is this the intended behavior?  If so, how can
I ensure that the keys do not change?  The different keys are causing
PAC authentication to fail on pointers signed using the stale key.
Thanks.

--
Derrick McKee
Email: derrick.mckee@gmail.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: PAC key changes when kernel code is preempted

[Mark Rutland]

On Fri, Apr 30, 2021 at 10:40:04AM -0400, Derrick McKee wrote:
> Hi,
> 
> I am noticing that when kernel code is preempted, PAC keys seem to
> change when resuming execution.  For instance, when I read
> APDAKeyHi_EL1 and APDAKeyLo_EL1, sleep, and read them again, the
> values are different.  Is this the intended behavior? 

This is expected; kernel-side we only use the IA keys (which should stay
the same from a kernel task's PoV), and the other keys (IB, DA, DB, GA)
are not supposed to be used within the kernel.

Up to and including v5.12, the other keys are switched at entry to/from
userspace, and so may change from the PoV of a kernel thread across
preemption.

With the patches merged for v5.13, the other keys will be
switched with the task, but userspace can reset these at any time, and
they are still not supposed to be used within the kernel.

> If so, how can I ensure that the keys do not change?  The different
> keys are causing PAC authentication to fail on pointers signed using
> the stale key.  Thanks.

I take it this is non-mainline code? We shouldn't be using the other
keys today.

If you want to use the other keys, you'll need to alter the
context-switch and userspace entry/exit logic to have kernel versions of
the other keys, and switch them at the same points we switch the IA
keys. 

For reference, see commit:

  b90e483938ce387c ("arm64: pac: Optimize kernel entry/exit key installation code paths")

Thank,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: PAC key changes when kernel code is preempted

[Derrick McKee]

On Fri, Apr 30, 2021 at 11:04 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Fri, Apr 30, 2021 at 10:40:04AM -0400, Derrick McKee wrote:
> > Hi,
> >
> > I am noticing that when kernel code is preempted, PAC keys seem to
> > change when resuming execution.  For instance, when I read
> > APDAKeyHi_EL1 and APDAKeyLo_EL1, sleep, and read them again, the
> > values are different.  Is this the intended behavior?
>
> This is expected; kernel-side we only use the IA keys (which should stay
> the same from a kernel task's PoV), and the other keys (IB, DA, DB, GA)
> are not supposed to be used within the kernel.
>
> Up to and including v5.12, the other keys are switched at entry to/from
> userspace, and so may change from the PoV of a kernel thread across
> preemption.

Thanks for the response.  When was the preservation and restoration of
the IA key added to the kernel, because in my 5.10 code base, I am
seeing the IA key change for the kernel between system calls.

> With the patches merged for v5.13, the other keys will be
> switched with the task, but userspace can reset these at any time, and
> they are still not supposed to be used within the kernel.
>
> > If so, how can I ensure that the keys do not change?  The different
> > keys are causing PAC authentication to fail on pointers signed using
> > the stale key.  Thanks.
>
> I take it this is non-mainline code? We shouldn't be using the other
> keys today.

Yeah, I'm a doctorate researcher attempting to combine PAC and MTE to
harden kernel modules, which is why I am trying to make sure PAC keys
are the same. Pointers to kernel data are being signed and
authenticated using different keys.  It actually doesn't matter for
our purposes what key is used, just that the key remains the same
whenever kernel code gets executed.

Thanks.
--
Derrick McKee

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] Ensure kernel AI key is not changed on fork

[Derrick McKee]

The kernel uses the IA key for PAC signing, 
and this key should remain unchanged from the kernel point of view.
This patch ensures that the IA key remains constant on fork, 
if it has been previously set.
The software is provided on an as-is basis.

Signed-off-by: Derrick McKee <derrick.mckee@gmail.com>
Signed-off-by: Yianni Giannaris <yiannig@mit.edu>
---
 arch/arm64/include/asm/pointer_auth.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
index d50416be99be..9748413e72fd 100644
--- a/arch/arm64/include/asm/pointer_auth.h
+++ b/arch/arm64/include/asm/pointer_auth.h
@@ -69,10 +69,13 @@ static inline void ptrauth_keys_init_user(struct ptrauth_keys_user *keys)
 	ptrauth_keys_install_user(keys);
 }
 
-static __always_inline void ptrauth_keys_init_kernel(struct ptrauth_keys_kernel *keys)
+static __always_inline void
+ptrauth_keys_init_kernel(struct ptrauth_keys_kernel *keys)
 {
-	if (system_supports_address_auth())
-		get_random_bytes(&keys->apia, sizeof(keys->apia));
+	if (keys->apia.lo == 0 && keys->apia.hi == 0) {
+		if (system_supports_address_auth())
+			get_random_bytes(&keys->apia, sizeof(keys->apia));
+	}
 }
 
 static __always_inline void ptrauth_keys_switch_kernel(struct ptrauth_keys_kernel *keys)
-- 
2.31.1

[PATCH] Ensure kernel AI key is not changed on fork

[Derrick McKee]

The kernel uses the IA key for PAC signing, 
and this key should remain unchanged from the kernel point of view.
This patch ensures that the IA key remains constant on fork, 
if it has been previously set.
The software is provided on an as-is basis.

Signed-off-by: Derrick McKee <derrick.mckee@gmail.com>
Signed-off-by: Yianni Giannaris <yiannig@mit.edu>
---
 arch/arm64/include/asm/pointer_auth.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
index d50416be99be..9748413e72fd 100644
--- a/arch/arm64/include/asm/pointer_auth.h
+++ b/arch/arm64/include/asm/pointer_auth.h
@@ -69,10 +69,13 @@ static inline void ptrauth_keys_init_user(struct ptrauth_keys_user *keys)
 	ptrauth_keys_install_user(keys);
 }
 
-static __always_inline void ptrauth_keys_init_kernel(struct ptrauth_keys_kernel *keys)
+static __always_inline void
+ptrauth_keys_init_kernel(struct ptrauth_keys_kernel *keys)
 {
-	if (system_supports_address_auth())
-		get_random_bytes(&keys->apia, sizeof(keys->apia));
+	if (keys->apia.lo == 0 && keys->apia.hi == 0) {
+		if (system_supports_address_auth())
+			get_random_bytes(&keys->apia, sizeof(keys->apia));
+	}
 }
 
 static __always_inline void ptrauth_keys_switch_kernel(struct ptrauth_keys_kernel *keys)
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Ensure kernel AI key is not changed on fork

[Mark Rutland]

On Thu, May 20, 2021 at 11:18:54AM -0400, Derrick McKee wrote:
> The kernel uses the IA key for PAC signing, 
> and this key should remain unchanged from the kernel point of view.
> This patch ensures that the IA key remains constant on fork, 
> if it has been previously set.
> The software is provided on an as-is basis.
> 
> Signed-off-by: Derrick McKee <derrick.mckee@gmail.com>
> Signed-off-by: Yianni Giannaris <yiannig@mit.edu>

On the kernel side, we use a unique IA key per kernel thread, and while
this must remain the same *for that kernel thread*, the kernel IA key
should differ across kernel threads when a fork() occurs.

I think you're trying to use the keys in a different way than upstream
intends to, and we do not need this change as-is.

So NAK to this patch as it stands.

Thanks,
Mark.

> ---
>  arch/arm64/include/asm/pointer_auth.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
> index d50416be99be..9748413e72fd 100644
> --- a/arch/arm64/include/asm/pointer_auth.h
> +++ b/arch/arm64/include/asm/pointer_auth.h
> @@ -69,10 +69,13 @@ static inline void ptrauth_keys_init_user(struct ptrauth_keys_user *keys)
>  	ptrauth_keys_install_user(keys);
>  }
>  
> -static __always_inline void ptrauth_keys_init_kernel(struct ptrauth_keys_kernel *keys)
> +static __always_inline void
> +ptrauth_keys_init_kernel(struct ptrauth_keys_kernel *keys)
>  {
> -	if (system_supports_address_auth())
> -		get_random_bytes(&keys->apia, sizeof(keys->apia));
> +	if (keys->apia.lo == 0 && keys->apia.hi == 0) {
> +		if (system_supports_address_auth())
> +			get_random_bytes(&keys->apia, sizeof(keys->apia));
> +	}
>  }
>  
>  static __always_inline void ptrauth_keys_switch_kernel(struct ptrauth_keys_kernel *keys)
> -- 
> 2.31.1
> 

Re: [PATCH] Ensure kernel AI key is not changed on fork

[Mark Rutland]

On Thu, May 20, 2021 at 11:18:54AM -0400, Derrick McKee wrote:
> The kernel uses the IA key for PAC signing, 
> and this key should remain unchanged from the kernel point of view.
> This patch ensures that the IA key remains constant on fork, 
> if it has been previously set.
> The software is provided on an as-is basis.
> 
> Signed-off-by: Derrick McKee <derrick.mckee@gmail.com>
> Signed-off-by: Yianni Giannaris <yiannig@mit.edu>

On the kernel side, we use a unique IA key per kernel thread, and while
this must remain the same *for that kernel thread*, the kernel IA key
should differ across kernel threads when a fork() occurs.

I think you're trying to use the keys in a different way than upstream
intends to, and we do not need this change as-is.

So NAK to this patch as it stands.

Thanks,
Mark.

> ---
>  arch/arm64/include/asm/pointer_auth.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
> index d50416be99be..9748413e72fd 100644
> --- a/arch/arm64/include/asm/pointer_auth.h
> +++ b/arch/arm64/include/asm/pointer_auth.h
> @@ -69,10 +69,13 @@ static inline void ptrauth_keys_init_user(struct ptrauth_keys_user *keys)
>  	ptrauth_keys_install_user(keys);
>  }
>  
> -static __always_inline void ptrauth_keys_init_kernel(struct ptrauth_keys_kernel *keys)
> +static __always_inline void
> +ptrauth_keys_init_kernel(struct ptrauth_keys_kernel *keys)
>  {
> -	if (system_supports_address_auth())
> -		get_random_bytes(&keys->apia, sizeof(keys->apia));
> +	if (keys->apia.lo == 0 && keys->apia.hi == 0) {
> +		if (system_supports_address_auth())
> +			get_random_bytes(&keys->apia, sizeof(keys->apia));
> +	}
>  }
>  
>  static __always_inline void ptrauth_keys_switch_kernel(struct ptrauth_keys_kernel *keys)
> -- 
> 2.31.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Ensure kernel AI key is not changed on fork

[Derrick McKee]

On Thu, May 20, 2021 at 12:00 PM Mark Rutland <mark.rutland@arm.com> wrote:

> On the kernel side, we use a unique IA key per kernel thread, and while
> this must remain the same *for that kernel thread*, the kernel IA key
> should differ across kernel threads when a fork() occurs.

Thank you for the clarification.

> I think you're trying to use the keys in a different way than upstream
> intends to, and we do not need this change as-is.
>
> So NAK to this patch as it stands.

Given the above discussion, I agree with the NAK.

>
> > ---
> >  arch/arm64/include/asm/pointer_auth.h | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
> > index d50416be99be..9748413e72fd 100644
> > --- a/arch/arm64/include/asm/pointer_auth.h
> > +++ b/arch/arm64/include/asm/pointer_auth.h
> > @@ -69,10 +69,13 @@ static inline void ptrauth_keys_init_user(struct ptrauth_keys_user *keys)
> >       ptrauth_keys_install_user(keys);
> >  }
> >
> > -static __always_inline void ptrauth_keys_init_kernel(struct ptrauth_keys_kernel *keys)
> > +static __always_inline void
> > +ptrauth_keys_init_kernel(struct ptrauth_keys_kernel *keys)
> >  {
> > -     if (system_supports_address_auth())
> > -             get_random_bytes(&keys->apia, sizeof(keys->apia));
> > +     if (keys->apia.lo == 0 && keys->apia.hi == 0) {
> > +             if (system_supports_address_auth())
> > +                     get_random_bytes(&keys->apia, sizeof(keys->apia));
> > +     }
> >  }
> >
> >  static __always_inline void ptrauth_keys_switch_kernel(struct ptrauth_keys_kernel *keys)
> > --
> > 2.31.1
> >



-- 
Derrick McKee
Phone: (703) 957-9362
Email: derrick.mckee@gmail.com

Re: [PATCH] Ensure kernel AI key is not changed on fork

[Derrick McKee]

On Thu, May 20, 2021 at 12:00 PM Mark Rutland <mark.rutland@arm.com> wrote:

> On the kernel side, we use a unique IA key per kernel thread, and while
> this must remain the same *for that kernel thread*, the kernel IA key
> should differ across kernel threads when a fork() occurs.

Thank you for the clarification.

> I think you're trying to use the keys in a different way than upstream
> intends to, and we do not need this change as-is.
>
> So NAK to this patch as it stands.

Given the above discussion, I agree with the NAK.

>
> > ---
> >  arch/arm64/include/asm/pointer_auth.h | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
> > index d50416be99be..9748413e72fd 100644
> > --- a/arch/arm64/include/asm/pointer_auth.h
> > +++ b/arch/arm64/include/asm/pointer_auth.h
> > @@ -69,10 +69,13 @@ static inline void ptrauth_keys_init_user(struct ptrauth_keys_user *keys)
> >       ptrauth_keys_install_user(keys);
> >  }
> >
> > -static __always_inline void ptrauth_keys_init_kernel(struct ptrauth_keys_kernel *keys)
> > +static __always_inline void
> > +ptrauth_keys_init_kernel(struct ptrauth_keys_kernel *keys)
> >  {
> > -     if (system_supports_address_auth())
> > -             get_random_bytes(&keys->apia, sizeof(keys->apia));
> > +     if (keys->apia.lo == 0 && keys->apia.hi == 0) {
> > +             if (system_supports_address_auth())
> > +                     get_random_bytes(&keys->apia, sizeof(keys->apia));
> > +     }
> >  }
> >
> >  static __always_inline void ptrauth_keys_switch_kernel(struct ptrauth_keys_kernel *keys)
> > --
> > 2.31.1
> >



-- 
Derrick McKee
Phone: (703) 957-9362
Email: derrick.mckee@gmail.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel