[v4.4,V2,25/43] arm64: Move BP hardening to check_and_switch_context

Commit Message

Viresh Kumar July 12, 2019, 5:28 a.m. UTC

From: Marc Zyngier <[e-mail]>

commit a8e4c0a919ae310944ed2c9ace11cf3ccd8a609b upstream.

We call arm64_apply_bp_hardening() from post_ttbr_update_workaround,
which has the unexpected consequence of being triggered on every
exception return to userspace when ARM64_SW_TTBR0_PAN is selected,
even if no context switch actually occured.

This is a bit suboptimal, and it would be more logical to only
invalidate the branch predictor when we actually switch to
a different mm.

In order to solve this, move the call to arm64_apply_bp_hardening()
into check_and_switch_context(), where we're guaranteed to pick
a different mm context.

Acked-by: Will Deacon <[e-mail]>
Signed-off-by: Marc Zyngier <[e-mail]>
Signed-off-by: Catalin Marinas <[e-mail]>
Signed-off-by: Viresh Kumar <[e-mail]>
---
 arch/arm64/mm/context.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Julien Thierry July 31, 2019, 1:09 p.m. UTC | #1

On 12/07/2019 06:28, Viresh Kumar wrote:
> From: Marc Zyngier <[e-mail]>
> 
> commit a8e4c0a919ae310944ed2c9ace11cf3ccd8a609b upstream.
> 
> We call arm64_apply_bp_hardening() from post_ttbr_update_workaround,
> which has the unexpected consequence of being triggered on every
> exception return to userspace when ARM64_SW_TTBR0_PAN is selected,
> even if no context switch actually occured.
> 
> This is a bit suboptimal, and it would be more logical to only
> invalidate the branch predictor when we actually switch to
> a different mm.
> 
> In order to solve this, move the call to arm64_apply_bp_hardening()
> into check_and_switch_context(), where we're guaranteed to pick
> a different mm context.
> 
> Acked-by: Will Deacon <[e-mail]>
> Signed-off-by: Marc Zyngier <[e-mail]>
> Signed-off-by: Catalin Marinas <[e-mail]>
> Signed-off-by: Viresh Kumar <[e-mail]>
> ---
>  arch/arm64/mm/context.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c
> index be42bd3dca5c..de5afc27b4e6 100644
> --- a/arch/arm64/mm/context.c
> +++ b/arch/arm64/mm/context.c
> @@ -183,6 +183,8 @@ void check_and_switch_context(struct mm_struct *mm, unsigned int cpu)
>  	raw_spin_unlock_irqrestore(&cpu_asid_lock, flags);
>  
>  switch_mm_fastpath:
> +	arm64_apply_bp_hardening();
> +
>  	cpu_switch_mm(mm->pgd, mm);
>  }
>  
> @@ -193,8 +195,6 @@ asmlinkage void post_ttbr_update_workaround(void)
>  			"ic iallu; dsb nsh; isb",
>  			ARM64_WORKAROUND_CAVIUM_27456,
>  			CONFIG_CAVIUM_ERRATUM_27456));
> -
> -	arm64_apply_bp_hardening();

Patches 22 and 23 factorize the post_ttbr_update_workaround() and move
it to C code just so we would and a call to arm64_apply_bp_hardening()
in patch 24 that now gets moved elsewhere?

Is it really worth backporting patches 22 and 23?

Cheers,

Viresh Kumar Aug. 1, 2019, 5:09 a.m. UTC | #2

On 31-07-19, 14:09, Julien Thierry wrote:
> 
> 
> On 12/07/2019 06:28, Viresh Kumar wrote:
> > From: Marc Zyngier <[e-mail]>
> > 
> > commit a8e4c0a919ae310944ed2c9ace11cf3ccd8a609b upstream.
> > 
> > We call arm64_apply_bp_hardening() from post_ttbr_update_workaround,
> > which has the unexpected consequence of being triggered on every
> > exception return to userspace when ARM64_SW_TTBR0_PAN is selected,
> > even if no context switch actually occured.
> > 
> > This is a bit suboptimal, and it would be more logical to only
> > invalidate the branch predictor when we actually switch to
> > a different mm.
> > 
> > In order to solve this, move the call to arm64_apply_bp_hardening()
> > into check_and_switch_context(), where we're guaranteed to pick
> > a different mm context.
> > 
> > Acked-by: Will Deacon <[e-mail]>
> > Signed-off-by: Marc Zyngier <[e-mail]>
> > Signed-off-by: Catalin Marinas <[e-mail]>
> > Signed-off-by: Viresh Kumar <[e-mail]>
> > ---
> >  arch/arm64/mm/context.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c
> > index be42bd3dca5c..de5afc27b4e6 100644
> > --- a/arch/arm64/mm/context.c
> > +++ b/arch/arm64/mm/context.c
> > @@ -183,6 +183,8 @@ void check_and_switch_context(struct mm_struct *mm, unsigned int cpu)
> >  	raw_spin_unlock_irqrestore(&cpu_asid_lock, flags);
> >  
> >  switch_mm_fastpath:
> > +	arm64_apply_bp_hardening();
> > +
> >  	cpu_switch_mm(mm->pgd, mm);
> >  }
> >  
> > @@ -193,8 +195,6 @@ asmlinkage void post_ttbr_update_workaround(void)
> >  			"ic iallu; dsb nsh; isb",
> >  			ARM64_WORKAROUND_CAVIUM_27456,
> >  			CONFIG_CAVIUM_ERRATUM_27456));
> > -
> > -	arm64_apply_bp_hardening();
> 
> Patches 22 and 23 factorize the post_ttbr_update_workaround() and move
> it to C code just so we would and a call to arm64_apply_bp_hardening()
> in patch 24 that now gets moved elsewhere?
> 
> Is it really worth backporting patches 22 and 23?

If I can merge patch 24 and 25 into a single patch while backporting,
then patch 22 and 23 won't be required. I am not sure how should the
commit log look like in that case though :)

Is mentioning both the upstream commit ids along with log of the first
patch (which was more important) enough, like this ?

Author: Will Deacon <[e-mail]>
Date:   Wed Jan 3 11:17:58 2018 +0000

    arm64: Add skeleton to harden the branch predictor against aliasing attacks
    
    commit 0f15adbb2861ce6f75ccfc5a92b19eae0ef327d0 upstream.
    commit a8e4c0a919ae310944ed2c9ace11cf3ccd8a609b upstream.
    
    Aliasing attacks against CPU branch predictors can allow an attacker to
    redirect speculative control flow on some CPUs and potentially divulge
    information from one context to another.
    
    This patch adds initial skeleton code behind a new Kconfig option to
    enable implementation-specific mitigations against these attacks for
    CPUs that are affected.
    
    Co-developed-by: Marc Zyngier <[e-mail]>
    Signed-off-by: Will Deacon <[e-mail]>
    Signed-off-by: Catalin Marinas <[e-mail]>
    [ v4.4: Changes made according to 4.4 codebase ]
    Signed-off-by: Viresh Kumar <[e-mail]>

Julien Thierry Aug. 1, 2019, 6:30 a.m. UTC | #3

On 01/08/2019 06:09, Viresh Kumar wrote:
> On 31-07-19, 14:09, Julien Thierry wrote:
>>
>>
>> On 12/07/2019 06:28, Viresh Kumar wrote:
>>> From: Marc Zyngier <[e-mail]>
>>>
>>> commit a8e4c0a919ae310944ed2c9ace11cf3ccd8a609b upstream.
>>>
>>> We call arm64_apply_bp_hardening() from post_ttbr_update_workaround,
>>> which has the unexpected consequence of being triggered on every
>>> exception return to userspace when ARM64_SW_TTBR0_PAN is selected,
>>> even if no context switch actually occured.
>>>
>>> This is a bit suboptimal, and it would be more logical to only
>>> invalidate the branch predictor when we actually switch to
>>> a different mm.
>>>
>>> In order to solve this, move the call to arm64_apply_bp_hardening()
>>> into check_and_switch_context(), where we're guaranteed to pick
>>> a different mm context.
>>>
>>> Acked-by: Will Deacon <[e-mail]>
>>> Signed-off-by: Marc Zyngier <[e-mail]>
>>> Signed-off-by: Catalin Marinas <[e-mail]>
>>> Signed-off-by: Viresh Kumar <[e-mail]>
>>> ---
>>>  arch/arm64/mm/context.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c
>>> index be42bd3dca5c..de5afc27b4e6 100644
>>> --- a/arch/arm64/mm/context.c
>>> +++ b/arch/arm64/mm/context.c
>>> @@ -183,6 +183,8 @@ void check_and_switch_context(struct mm_struct *mm, unsigned int cpu)
>>>  	raw_spin_unlock_irqrestore(&cpu_asid_lock, flags);
>>>  
>>>  switch_mm_fastpath:
>>> +	arm64_apply_bp_hardening();
>>> +
>>>  	cpu_switch_mm(mm->pgd, mm);
>>>  }
>>>  
>>> @@ -193,8 +195,6 @@ asmlinkage void post_ttbr_update_workaround(void)
>>>  			"ic iallu; dsb nsh; isb",
>>>  			ARM64_WORKAROUND_CAVIUM_27456,
>>>  			CONFIG_CAVIUM_ERRATUM_27456));
>>> -
>>> -	arm64_apply_bp_hardening();
>>
>> Patches 22 and 23 factorize the post_ttbr_update_workaround() and move
>> it to C code just so we would and a call to arm64_apply_bp_hardening()
>> in patch 24 that now gets moved elsewhere?
>>
>> Is it really worth backporting patches 22 and 23?
> 
> If I can merge patch 24 and 25 into a single patch while backporting,
> then patch 22 and 23 won't be required. I am not sure how should the
> commit log look like in that case though :)
> 
> Is mentioning both the upstream commit ids along with log of the first
> patch (which was more important) enough, like this ?
> 

I must admit I am not familiar with backport/stable process enough. But
personally I think the your suggestion seems more sensible than
backporting 4 patches.

Or you can maybe ignore patch 25 and say in patch 24 that among the
changes made for the 4.4 codebase, the call arm64_apply_bp_hardening()
was moved from post_ttbr_update_workaround as it doesn't exist and
placed in check_and_switch_context() as it is its final destination.

However, I really don't know what's the best way to proceed according to
existing practices. So input from someone else would be welcome.

Thanks,

Julien

> Author: Will Deacon <[e-mail]>
> Date:   Wed Jan 3 11:17:58 2018 +0000
> 
>     arm64: Add skeleton to harden the branch predictor against aliasing attacks
>     
>     commit 0f15adbb2861ce6f75ccfc5a92b19eae0ef327d0 upstream.
>     commit a8e4c0a919ae310944ed2c9ace11cf3ccd8a609b upstream.
>     
>     Aliasing attacks against CPU branch predictors can allow an attacker to
>     redirect speculative control flow on some CPUs and potentially divulge
>     information from one context to another.
>     
>     This patch adds initial skeleton code behind a new Kconfig option to
>     enable implementation-specific mitigations against these attacks for
>     CPUs that are affected.
>     
>     Co-developed-by: Marc Zyngier <[e-mail]>
>     Signed-off-by: Will Deacon <[e-mail]>
>     Signed-off-by: Catalin Marinas <[e-mail]>
>     [ v4.4: Changes made according to 4.4 codebase ]
>     Signed-off-by: Viresh Kumar <[e-mail]>
>

Viresh Kumar Aug. 1, 2019, 6:35 a.m. UTC | #4

On 01-08-19, 07:30, Julien Thierry wrote:
> I must admit I am not familiar with backport/stable process enough. But
> personally I think the your suggestion seems more sensible than
> backporting 4 patches.
> 
> Or you can maybe ignore patch 25 and say in patch 24 that among the
> changes made for the 4.4 codebase, the call arm64_apply_bp_hardening()
> was moved from post_ttbr_update_workaround as it doesn't exist and
> placed in check_and_switch_context() as it is its final destination.

Done that and dropped the other two patches.

> However, I really don't know what's the best way to proceed according to
> existing practices. So input from someone else would be welcome.

Lets see if someone comes up and ask me to do something else :)

Greg KH Aug. 1, 2019, 6:57 a.m. UTC | #5

On Thu, Aug 01, 2019 at 12:05:44PM +0530, Viresh Kumar wrote:
> On 01-08-19, 07:30, Julien Thierry wrote:
> > I must admit I am not familiar with backport/stable process enough. But
> > personally I think the your suggestion seems more sensible than
> > backporting 4 patches.
> > 
> > Or you can maybe ignore patch 25 and say in patch 24 that among the
> > changes made for the 4.4 codebase, the call arm64_apply_bp_hardening()
> > was moved from post_ttbr_update_workaround as it doesn't exist and
> > placed in check_and_switch_context() as it is its final destination.
> 
> Done that and dropped the other two patches.
> 
> > However, I really don't know what's the best way to proceed according to
> > existing practices. So input from someone else would be welcome.
> 
> Lets see if someone comes up and ask me to do something else :)

Keeping the same patches that upstream has is almost always the better
thing to do in the long-run.

thanks,

greg k-h

Viresh Kumar Aug. 1, 2019, 7:05 a.m. UTC | #6

On 01-08-19, 08:57, Greg KH wrote:
> On Thu, Aug 01, 2019 at 12:05:44PM +0530, Viresh Kumar wrote:
> > On 01-08-19, 07:30, Julien Thierry wrote:
> > > I must admit I am not familiar with backport/stable process enough. But
> > > personally I think the your suggestion seems more sensible than
> > > backporting 4 patches.
> > > 
> > > Or you can maybe ignore patch 25 and say in patch 24 that among the
> > > changes made for the 4.4 codebase, the call arm64_apply_bp_hardening()
> > > was moved from post_ttbr_update_workaround as it doesn't exist and
> > > placed in check_and_switch_context() as it is its final destination.
> > 
> > Done that and dropped the other two patches.
> > 
> > > However, I really don't know what's the best way to proceed according to
> > > existing practices. So input from someone else would be welcome.
> > 
> > Lets see if someone comes up and ask me to do something else :)
> 
> Keeping the same patches that upstream has is almost always the better
> thing to do in the long-run.

That would require two additional patches to be backported, 22 and 23
from this series. From your suggestion it seems that keeping them is
better here ?

Will Deacon Aug. 1, 2019, 7:34 a.m. UTC | #7

On Thu, Aug 01, 2019 at 12:35:41PM +0530, Viresh Kumar wrote:
> On 01-08-19, 08:57, Greg KH wrote:
> > On Thu, Aug 01, 2019 at 12:05:44PM +0530, Viresh Kumar wrote:
> > > On 01-08-19, 07:30, Julien Thierry wrote:
> > > > I must admit I am not familiar with backport/stable process enough. But
> > > > personally I think the your suggestion seems more sensible than
> > > > backporting 4 patches.
> > > > 
> > > > Or you can maybe ignore patch 25 and say in patch 24 that among the
> > > > changes made for the 4.4 codebase, the call arm64_apply_bp_hardening()
> > > > was moved from post_ttbr_update_workaround as it doesn't exist and
> > > > placed in check_and_switch_context() as it is its final destination.
> > > 
> > > Done that and dropped the other two patches.
> > > 
> > > > However, I really don't know what's the best way to proceed according to
> > > > existing practices. So input from someone else would be welcome.
> > > 
> > > Lets see if someone comes up and ask me to do something else :)
> > 
> > Keeping the same patches that upstream has is almost always the better
> > thing to do in the long-run.
> 
> That would require two additional patches to be backported, 22 and 23
> from this series. From your suggestion it seems that keeping them is
> better here ?

Yes. Backporting individual patches as they appear upstream is definitely
the preferred method for -stable. It makes the relationship to mainline
crystal clear, as well as any dependencies between patches that have been
backported. Everytime we tweak something unecessarily in a stable backport,
it just creates the potential for confusion and additional conflicts in
future backports, so it's best to follow the shape of upstream as closely as
possible, even if it results in additional patches.

So I wouldn't worry about total number of patches. I'd worry more about
things like conflicts, deviation from mainline and overall testing coverage.

Will

Viresh Kumar Aug. 1, 2019, 7:41 a.m. UTC | #8

On 01-08-19, 08:34, Will Deacon wrote:
> On Thu, Aug 01, 2019 at 12:35:41PM +0530, Viresh Kumar wrote:
> > On 01-08-19, 08:57, Greg KH wrote:
> > > On Thu, Aug 01, 2019 at 12:05:44PM +0530, Viresh Kumar wrote:
> > > > On 01-08-19, 07:30, Julien Thierry wrote:
> > > > > I must admit I am not familiar with backport/stable process enough. But
> > > > > personally I think the your suggestion seems more sensible than
> > > > > backporting 4 patches.
> > > > > 
> > > > > Or you can maybe ignore patch 25 and say in patch 24 that among the
> > > > > changes made for the 4.4 codebase, the call arm64_apply_bp_hardening()
> > > > > was moved from post_ttbr_update_workaround as it doesn't exist and
> > > > > placed in check_and_switch_context() as it is its final destination.
> > > > 
> > > > Done that and dropped the other two patches.
> > > > 
> > > > > However, I really don't know what's the best way to proceed according to
> > > > > existing practices. So input from someone else would be welcome.
> > > > 
> > > > Lets see if someone comes up and ask me to do something else :)
> > > 
> > > Keeping the same patches that upstream has is almost always the better
> > > thing to do in the long-run.
> > 
> > That would require two additional patches to be backported, 22 and 23
> > from this series. From your suggestion it seems that keeping them is
> > better here ?
> 
> Yes. Backporting individual patches as they appear upstream is definitely
> the preferred method for -stable. It makes the relationship to mainline
> crystal clear, as well as any dependencies between patches that have been
> backported. Everytime we tweak something unecessarily in a stable backport,
> it just creates the potential for confusion and additional conflicts in
> future backports, so it's best to follow the shape of upstream as closely as
> possible, even if it results in additional patches.
> 
> So I wouldn't worry about total number of patches. I'd worry more about
> things like conflicts, deviation from mainline and overall testing coverage.

Okay, I won't make these changes then. Thanks.

Greg KH Aug. 1, 2019, 8:43 a.m. UTC | #9

On Thu, Aug 01, 2019 at 08:34:45AM +0100, Will Deacon wrote:
> On Thu, Aug 01, 2019 at 12:35:41PM +0530, Viresh Kumar wrote:
> > On 01-08-19, 08:57, Greg KH wrote:
> > > On Thu, Aug 01, 2019 at 12:05:44PM +0530, Viresh Kumar wrote:
> > > > On 01-08-19, 07:30, Julien Thierry wrote:
> > > > > I must admit I am not familiar with backport/stable process enough. But
> > > > > personally I think the your suggestion seems more sensible than
> > > > > backporting 4 patches.
> > > > > 
> > > > > Or you can maybe ignore patch 25 and say in patch 24 that among the
> > > > > changes made for the 4.4 codebase, the call arm64_apply_bp_hardening()
> > > > > was moved from post_ttbr_update_workaround as it doesn't exist and
> > > > > placed in check_and_switch_context() as it is its final destination.
> > > > 
> > > > Done that and dropped the other two patches.
> > > > 
> > > > > However, I really don't know what's the best way to proceed according to
> > > > > existing practices. So input from someone else would be welcome.
> > > > 
> > > > Lets see if someone comes up and ask me to do something else :)
> > > 
> > > Keeping the same patches that upstream has is almost always the better
> > > thing to do in the long-run.
> > 
> > That would require two additional patches to be backported, 22 and 23
> > from this series. From your suggestion it seems that keeping them is
> > better here ?
> 
> Yes. Backporting individual patches as they appear upstream is definitely
> the preferred method for -stable. It makes the relationship to mainline
> crystal clear, as well as any dependencies between patches that have been
> backported. Everytime we tweak something unecessarily in a stable backport,
> it just creates the potential for confusion and additional conflicts in
> future backports, so it's best to follow the shape of upstream as closely as
> possible, even if it results in additional patches.
> 
> So I wouldn't worry about total number of patches. I'd worry more about
> things like conflicts, deviation from mainline and overall testing coverage.

That is exactly correct, thanks for saying it better than I could :)

greg k-h

Julien Thierry Aug. 1, 2019, 8:49 a.m. UTC | #10

On 01/08/2019 09:43, Greg KH wrote:
> On Thu, Aug 01, 2019 at 08:34:45AM +0100, Will Deacon wrote:
>> On Thu, Aug 01, 2019 at 12:35:41PM +0530, Viresh Kumar wrote:
>>> On 01-08-19, 08:57, Greg KH wrote:
>>>> On Thu, Aug 01, 2019 at 12:05:44PM +0530, Viresh Kumar wrote:
>>>>> On 01-08-19, 07:30, Julien Thierry wrote:
>>>>>> I must admit I am not familiar with backport/stable process enough. But
>>>>>> personally I think the your suggestion seems more sensible than
>>>>>> backporting 4 patches.
>>>>>>
>>>>>> Or you can maybe ignore patch 25 and say in patch 24 that among the
>>>>>> changes made for the 4.4 codebase, the call arm64_apply_bp_hardening()
>>>>>> was moved from post_ttbr_update_workaround as it doesn't exist and
>>>>>> placed in check_and_switch_context() as it is its final destination.
>>>>>
>>>>> Done that and dropped the other two patches.
>>>>>
>>>>>> However, I really don't know what's the best way to proceed according to
>>>>>> existing practices. So input from someone else would be welcome.
>>>>>
>>>>> Lets see if someone comes up and ask me to do something else :)
>>>>
>>>> Keeping the same patches that upstream has is almost always the better
>>>> thing to do in the long-run.
>>>
>>> That would require two additional patches to be backported, 22 and 23
>>> from this series. From your suggestion it seems that keeping them is
>>> better here ?
>>
>> Yes. Backporting individual patches as they appear upstream is definitely
>> the preferred method for -stable. It makes the relationship to mainline
>> crystal clear, as well as any dependencies between patches that have been
>> backported. Everytime we tweak something unecessarily in a stable backport,
>> it just creates the potential for confusion and additional conflicts in
>> future backports, so it's best to follow the shape of upstream as closely as
>> possible, even if it results in additional patches.
>>
>> So I wouldn't worry about total number of patches. I'd worry more about
>> things like conflicts, deviation from mainline and overall testing coverage.
> 
> That is exactly correct, thanks for saying it better than I could :)
> 

Thanks, I'll try to keep those guidelines in mind for my future comments
on backports.

Cheers,

Patch

diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c
index be42bd3dca5c..de5afc27b4e6 100644
--- a/arch/arm64/mm/context.c
+++ b/arch/arm64/mm/context.c
@@ -183,6 +183,8 @@  void check_and_switch_context(struct mm_struct *mm, unsigned int cpu)
 	raw_spin_unlock_irqrestore(&cpu_asid_lock, flags);
 
 switch_mm_fastpath:
+	arm64_apply_bp_hardening();
+
 	cpu_switch_mm(mm->pgd, mm);
 }
 
@@ -193,8 +195,6 @@  asmlinkage void post_ttbr_update_workaround(void)
 			"ic iallu; dsb nsh; isb",
 			ARM64_WORKAROUND_CAVIUM_27456,
 			CONFIG_CAVIUM_ERRATUM_27456));
-
-	arm64_apply_bp_hardening();
 }
 
 static int asids_init(void)