* [PATCH 1/2] arm64: uaccess: disable preempt during uaccess through ttbr0
@ 2021-12-21 13:42 Pingfan Liu
2021-12-21 13:42 ` [PATCH 2/2] arm64: uaccess: disable preempt during uaccess PSTATE.{PAN, TCO} Pingfan Liu
2021-12-22 12:59 ` [PATCH 1/2] arm64: uaccess: disable preempt during uaccess through ttbr0 Catalin Marinas
0 siblings, 2 replies; 6+ messages in thread
From: Pingfan Liu @ 2021-12-21 13:42 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Pingfan Liu, Catalin Marinas, Will Deacon, Mark Rutland,
Vincenzo Frascino, Andrey Konovalov, Arnd Bergmann
If using software PAN, the ttbr0 should keep unchanged, otherwise,
considering the following scenario:
task1
__uaccess_ttbr0_enable()
switch_mm(this,next,tsk), which resets ttbr0 to __pa_symbol(reserved_pg_dir)
switch_mm(prev,this,tsk), which can not re-install the user page table automatically
Tackle this issue by disabling preemption.
Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
Cc: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
To: linux-arm-kernel@lists.infradead.org
---
arch/arm64/include/asm/uaccess.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 3a5ff5e20586..406888877bbd 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -121,6 +121,7 @@ static inline bool uaccess_ttbr0_disable(void)
if (!system_uses_ttbr0_pan())
return false;
__uaccess_ttbr0_disable();
+ preempt_enable();
return true;
}
@@ -128,6 +129,7 @@ static inline bool uaccess_ttbr0_enable(void)
{
if (!system_uses_ttbr0_pan())
return false;
+ preempt_disable();
__uaccess_ttbr0_enable();
return true;
}
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] arm64: uaccess: disable preempt during uaccess PSTATE.{PAN, TCO}
2021-12-21 13:42 [PATCH 1/2] arm64: uaccess: disable preempt during uaccess through ttbr0 Pingfan Liu
@ 2021-12-21 13:42 ` Pingfan Liu
2021-12-22 13:02 ` [PATCH 2/2] arm64: uaccess: disable preempt during uaccess PSTATE.{PAN,TCO} Catalin Marinas
2021-12-22 12:59 ` [PATCH 1/2] arm64: uaccess: disable preempt during uaccess through ttbr0 Catalin Marinas
1 sibling, 1 reply; 6+ messages in thread
From: Pingfan Liu @ 2021-12-21 13:42 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Pingfan Liu, Catalin Marinas, Will Deacon, Mark Rutland,
Vincenzo Frascino, Andrey Konovalov, Arnd Bergmann
For the pair of
uaccess_enable_privileged()/uaccess_disable_privileged(), if preemption
happens, the exposure of the access to user space by another code piece
is unexpected.
For the pair of
__uaccess_enable_tco_async()/__uaccess_disable_tco_async(), the change
of PSTATE.TCO is exposed unexpectedly.
For the two cases, simply disable preemption to fix the issue.
Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
Cc: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
To: linux-arm-kernel@lists.infradead.org
---
arch/arm64/include/asm/uaccess.h | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 406888877bbd..37d5531f210d 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -193,14 +193,18 @@ static inline void __uaccess_enable_tco(void)
*/
static inline void __uaccess_disable_tco_async(void)
{
- if (system_uses_mte_async_or_asymm_mode())
- __uaccess_disable_tco();
+ if (system_uses_mte_async_or_asymm_mode()) {
+ __uaccess_disable_tco();
+ preempt_enable();
+ }
}
static inline void __uaccess_enable_tco_async(void)
{
- if (system_uses_mte_async_or_asymm_mode())
+ if (system_uses_mte_async_or_asymm_mode()) {
+ preempt_disable();
__uaccess_enable_tco();
+ }
}
static inline void uaccess_disable_privileged(void)
@@ -208,13 +212,16 @@ static inline void uaccess_disable_privileged(void)
__uaccess_disable_tco();
if (uaccess_ttbr0_disable())
- return;
+ goto out;
__uaccess_enable_hw_pan();
+out:
+ preempt_enable();
}
static inline void uaccess_enable_privileged(void)
{
+ preempt_disable();
__uaccess_enable_tco();
if (uaccess_ttbr0_enable())
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] arm64: uaccess: disable preempt during uaccess through ttbr0
2021-12-21 13:42 [PATCH 1/2] arm64: uaccess: disable preempt during uaccess through ttbr0 Pingfan Liu
2021-12-21 13:42 ` [PATCH 2/2] arm64: uaccess: disable preempt during uaccess PSTATE.{PAN, TCO} Pingfan Liu
@ 2021-12-22 12:59 ` Catalin Marinas
2021-12-23 9:36 ` Pingfan Liu
1 sibling, 1 reply; 6+ messages in thread
From: Catalin Marinas @ 2021-12-22 12:59 UTC (permalink / raw)
To: Pingfan Liu
Cc: linux-arm-kernel, Will Deacon, Mark Rutland, Vincenzo Frascino,
Andrey Konovalov, Arnd Bergmann
On Tue, Dec 21, 2021 at 09:42:41PM +0800, Pingfan Liu wrote:
> If using software PAN, the ttbr0 should keep unchanged, otherwise,
> considering the following scenario:
> task1
> __uaccess_ttbr0_enable()
> switch_mm(this,next,tsk), which resets ttbr0 to __pa_symbol(reserved_pg_dir)
> switch_mm(prev,this,tsk), which can not re-install the user page table automatically
Have you found a real problem with this in practice or just by code
inspection?
The assumption is that during uaccess_ttbr0_enable/disable regions, the
only way to get into switch_mm() is as a result of a page fault or
interrupt. The __swpan_{entry,exit}_el1 functions should take care of
restoring ttbr0 when returning to the interrupted context.
> Tackle this issue by disabling preemption.
>
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> To: linux-arm-kernel@lists.infradead.org
> ---
> arch/arm64/include/asm/uaccess.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 3a5ff5e20586..406888877bbd 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -121,6 +121,7 @@ static inline bool uaccess_ttbr0_disable(void)
> if (!system_uses_ttbr0_pan())
> return false;
> __uaccess_ttbr0_disable();
> + preempt_enable();
> return true;
> }
>
> @@ -128,6 +129,7 @@ static inline bool uaccess_ttbr0_enable(void)
> {
> if (!system_uses_ttbr0_pan())
> return false;
> + preempt_disable();
> __uaccess_ttbr0_enable();
> return true;
> }
preempt_disable() won't help much here if, for example, the subsequent
uaccess gets a fault and need to sleep until the accessed page gets
available. I suspect you'd get some sleeping in atomic warning as well
with the right debug options enabled.
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] arm64: uaccess: disable preempt during uaccess PSTATE.{PAN,TCO}
2021-12-21 13:42 ` [PATCH 2/2] arm64: uaccess: disable preempt during uaccess PSTATE.{PAN, TCO} Pingfan Liu
@ 2021-12-22 13:02 ` Catalin Marinas
2021-12-23 9:39 ` Pingfan Liu
0 siblings, 1 reply; 6+ messages in thread
From: Catalin Marinas @ 2021-12-22 13:02 UTC (permalink / raw)
To: Pingfan Liu
Cc: linux-arm-kernel, Will Deacon, Mark Rutland, Vincenzo Frascino,
Andrey Konovalov, Arnd Bergmann
On Tue, Dec 21, 2021 at 09:42:42PM +0800, Pingfan Liu wrote:
> For the pair of
> uaccess_enable_privileged()/uaccess_disable_privileged(), if preemption
> happens, the exposure of the access to user space by another code piece
> is unexpected.
How can this happen? We set PAN on exception entry.
> For the pair of
> __uaccess_enable_tco_async()/__uaccess_disable_tco_async(), the change
> of PSTATE.TCO is exposed unexpectedly.
As above, we disable TCO on exception entry.
> For the two cases, simply disable preemption to fix the issue.
>
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> To: linux-arm-kernel@lists.infradead.org
> ---
> arch/arm64/include/asm/uaccess.h | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 406888877bbd..37d5531f210d 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -193,14 +193,18 @@ static inline void __uaccess_enable_tco(void)
> */
> static inline void __uaccess_disable_tco_async(void)
> {
> - if (system_uses_mte_async_or_asymm_mode())
> - __uaccess_disable_tco();
> + if (system_uses_mte_async_or_asymm_mode()) {
> + __uaccess_disable_tco();
> + preempt_enable();
> + }
> }
>
> static inline void __uaccess_enable_tco_async(void)
> {
> - if (system_uses_mte_async_or_asymm_mode())
> + if (system_uses_mte_async_or_asymm_mode()) {
> + preempt_disable();
> __uaccess_enable_tco();
> + }
> }
As with the previous patch, disabling preemption prior to a
might_sleep() is wrong.
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] arm64: uaccess: disable preempt during uaccess through ttbr0
2021-12-22 12:59 ` [PATCH 1/2] arm64: uaccess: disable preempt during uaccess through ttbr0 Catalin Marinas
@ 2021-12-23 9:36 ` Pingfan Liu
0 siblings, 0 replies; 6+ messages in thread
From: Pingfan Liu @ 2021-12-23 9:36 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-arm-kernel, Will Deacon, Mark Rutland, Vincenzo Frascino,
Andrey Konovalov, Arnd Bergmann
On Wed, Dec 22, 2021 at 12:59:32PM +0000, Catalin Marinas wrote:
> On Tue, Dec 21, 2021 at 09:42:41PM +0800, Pingfan Liu wrote:
> > If using software PAN, the ttbr0 should keep unchanged, otherwise,
> > considering the following scenario:
> > task1
> > __uaccess_ttbr0_enable()
> > switch_mm(this,next,tsk), which resets ttbr0 to __pa_symbol(reserved_pg_dir)
> > switch_mm(prev,this,tsk), which can not re-install the user page table automatically
>
> Have you found a real problem with this in practice or just by code
> inspection?
>
By code inspection.
> The assumption is that during uaccess_ttbr0_enable/disable regions, the
> only way to get into switch_mm() is as a result of a page fault or
> interrupt. The __swpan_{entry,exit}_el1 functions should take care of
> restoring ttbr0 when returning to the interrupted context.
>
Yes, I read the code careless and miss it. I am chewing it.
Thank you very much for pointing me it.
> > Tackle this issue by disabling preemption.
> >
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> > Cc: Andrey Konovalov <andreyknvl@gmail.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > To: linux-arm-kernel@lists.infradead.org
> > ---
> > arch/arm64/include/asm/uaccess.h | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> > index 3a5ff5e20586..406888877bbd 100644
> > --- a/arch/arm64/include/asm/uaccess.h
> > +++ b/arch/arm64/include/asm/uaccess.h
> > @@ -121,6 +121,7 @@ static inline bool uaccess_ttbr0_disable(void)
> > if (!system_uses_ttbr0_pan())
> > return false;
> > __uaccess_ttbr0_disable();
> > + preempt_enable();
> > return true;
> > }
> >
> > @@ -128,6 +129,7 @@ static inline bool uaccess_ttbr0_enable(void)
> > {
> > if (!system_uses_ttbr0_pan())
> > return false;
> > + preempt_disable();
> > __uaccess_ttbr0_enable();
> > return true;
> > }
>
> preempt_disable() won't help much here if, for example, the subsequent
> uaccess gets a fault and need to sleep until the accessed page gets
> available. I suspect you'd get some sleeping in atomic warning as well
> with the right debug options enabled.
>
Yes, the user page may be swapped out.
Thank you for the guide.
Regards,
Pingfan
> --
> Catalin
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] arm64: uaccess: disable preempt during uaccess PSTATE.{PAN,TCO}
2021-12-22 13:02 ` [PATCH 2/2] arm64: uaccess: disable preempt during uaccess PSTATE.{PAN,TCO} Catalin Marinas
@ 2021-12-23 9:39 ` Pingfan Liu
0 siblings, 0 replies; 6+ messages in thread
From: Pingfan Liu @ 2021-12-23 9:39 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-arm-kernel, Will Deacon, Mark Rutland, Vincenzo Frascino,
Andrey Konovalov, Arnd Bergmann
On Wed, Dec 22, 2021 at 01:02:44PM +0000, Catalin Marinas wrote:
> On Tue, Dec 21, 2021 at 09:42:42PM +0800, Pingfan Liu wrote:
> > For the pair of
> > uaccess_enable_privileged()/uaccess_disable_privileged(), if preemption
> > happens, the exposure of the access to user space by another code piece
> > is unexpected.
>
> How can this happen? We set PAN on exception entry.
>
Yes, I make the some mistake as [1/2]
> > For the pair of
> > __uaccess_enable_tco_async()/__uaccess_disable_tco_async(), the change
> > of PSTATE.TCO is exposed unexpectedly.
>
> As above, we disable TCO on exception entry.
>
Yes.
> > For the two cases, simply disable preemption to fix the issue.
> >
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> > Cc: Andrey Konovalov <andreyknvl@gmail.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > To: linux-arm-kernel@lists.infradead.org
> > ---
> > arch/arm64/include/asm/uaccess.h | 15 +++++++++++----
> > 1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> > index 406888877bbd..37d5531f210d 100644
> > --- a/arch/arm64/include/asm/uaccess.h
> > +++ b/arch/arm64/include/asm/uaccess.h
> > @@ -193,14 +193,18 @@ static inline void __uaccess_enable_tco(void)
> > */
> > static inline void __uaccess_disable_tco_async(void)
> > {
> > - if (system_uses_mte_async_or_asymm_mode())
> > - __uaccess_disable_tco();
> > + if (system_uses_mte_async_or_asymm_mode()) {
> > + __uaccess_disable_tco();
> > + preempt_enable();
> > + }
> > }
> >
> > static inline void __uaccess_enable_tco_async(void)
> > {
> > - if (system_uses_mte_async_or_asymm_mode())
> > + if (system_uses_mte_async_or_asymm_mode()) {
> > + preempt_disable();
> > __uaccess_enable_tco();
> > + }
> > }
>
> As with the previous patch, disabling preemption prior to a
> might_sleep() is wrong.
>
Right.
I will make clear the kernel_{entry/_exit} part. Appreciate for your
help again.
Thanks,
Pingfan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-23 9:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-21 13:42 [PATCH 1/2] arm64: uaccess: disable preempt during uaccess through ttbr0 Pingfan Liu
2021-12-21 13:42 ` [PATCH 2/2] arm64: uaccess: disable preempt during uaccess PSTATE.{PAN, TCO} Pingfan Liu
2021-12-22 13:02 ` [PATCH 2/2] arm64: uaccess: disable preempt during uaccess PSTATE.{PAN,TCO} Catalin Marinas
2021-12-23 9:39 ` Pingfan Liu
2021-12-22 12:59 ` [PATCH 1/2] arm64: uaccess: disable preempt during uaccess through ttbr0 Catalin Marinas
2021-12-23 9:36 ` Pingfan Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.