[PATCH] arm64: entry: unmask IRQ in el0_sp()

[PATCH] arm64: entry: unmask IRQ in el0_sp()

[Mark Rutland]

Currently, the EL0 SP alignment handler masks IRQs unnecessarily. It
does so due to historic code sharing of the EL0 SP and PC alignment
handlers, and branch predictor hardening applicable to the EL0 SP
handler.

We began masking IRQs in the EL0 SP alignment handler in commit:

  5dfc6ed27710c42c ("arm64: entry: Apply BP hardening for high-priority synchronous exception")

... as this shared code with the EL0 PC alignment handler, and branch
predictor hardening made it necessary to disable IRQs for early parts of
the EL0 PC alignment handler. It was not necessary to mask IRQs during
EL0 SP alignment exceptions, but it was not considered harmful to do so.

This masking was carried forward into C code in commit:

  582f95835a8fc812 ("arm64: entry: convert el0_sync to C")

... where the SP/PC cases were split into separate handlers, and the
masking duplicated.

Subsequently the EL0 PC alignment handler was refactored to perform
branch predictor hardening before unmasking IRQs, in commit:

  bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening")

... but the redundant masking of IRQs was not removed from the EL0 SP
alignment handler.

Let's do so now, and make it interruptible as with most other
synchronous exception handlers.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/entry-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index fde59981445c..c839b5bf1904 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -175,7 +175,7 @@ NOKPROBE_SYMBOL(el0_pc);
 static void notrace el0_sp(struct pt_regs *regs, unsigned long esr)
 {
 	user_exit_irqoff();
-	local_daif_restore(DAIF_PROCCTX_NOIRQ);
+	local_daif_restore(DAIF_PROCCTX);
 	do_sp_pc_abort(regs->sp, esr, regs);
 }
 NOKPROBE_SYMBOL(el0_sp);
-- 
2.11.0


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

Re: [PATCH] arm64: entry: unmask IRQ in el0_sp()

[James Morse]

Hi Mark,

On 28/02/2020 14:59, Mark Rutland wrote:
> Currently, the EL0 SP alignment handler masks IRQs unnecessarily. It
> does so due to historic code sharing of the EL0 SP and PC alignment
> handlers, and branch predictor hardening applicable to the EL0 SP
> handler.
> 
> We began masking IRQs in the EL0 SP alignment handler in commit:
> 
>   5dfc6ed27710c42c ("arm64: entry: Apply BP hardening for high-priority synchronous exception")
> 
> ... as this shared code with the EL0 PC alignment handler, and branch
> predictor hardening made it necessary to disable IRQs for early parts of
> the EL0 PC alignment handler. It was not necessary to mask IRQs during
> EL0 SP alignment exceptions, but it was not considered harmful to do so.
> 
> This masking was carried forward into C code in commit:
> 
>   582f95835a8fc812 ("arm64: entry: convert el0_sync to C")
> 
> ... where the SP/PC cases were split into separate handlers, and the
> masking duplicated.
> 
> Subsequently the EL0 PC alignment handler was refactored to perform
> branch predictor hardening before unmasking IRQs, in commit:
> 
>   bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening")
> 
> ... but the redundant masking of IRQs was not removed from the EL0 SP
> alignment handler.

Bother.


> Let's do so now, and make it interruptible as with most other
> synchronous exception handlers.

I think you want:
Fixes: bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening")

on this as, bfe298745afc9548 changed the behaviour: local_daif_restore(DAIF_PROCCTX) was
called before arm64_notify_die(), now its not.

With that,
Reviewed-by: James Morse <james.morse@arm.com>


Thanks!

James

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

Re: [PATCH] arm64: entry: unmask IRQ in el0_sp()

[Mark Rutland]

On Fri, Feb 28, 2020 at 03:37:46PM +0000, James Morse wrote:
> Hi Mark,
> 
> On 28/02/2020 14:59, Mark Rutland wrote:
> > Currently, the EL0 SP alignment handler masks IRQs unnecessarily. It
> > does so due to historic code sharing of the EL0 SP and PC alignment
> > handlers, and branch predictor hardening applicable to the EL0 SP
> > handler.
> > 
> > We began masking IRQs in the EL0 SP alignment handler in commit:
> > 
> >   5dfc6ed27710c42c ("arm64: entry: Apply BP hardening for high-priority synchronous exception")
> > 
> > ... as this shared code with the EL0 PC alignment handler, and branch
> > predictor hardening made it necessary to disable IRQs for early parts of
> > the EL0 PC alignment handler. It was not necessary to mask IRQs during
> > EL0 SP alignment exceptions, but it was not considered harmful to do so.
> > 
> > This masking was carried forward into C code in commit:
> > 
> >   582f95835a8fc812 ("arm64: entry: convert el0_sync to C")
> > 
> > ... where the SP/PC cases were split into separate handlers, and the
> > masking duplicated.
> > 
> > Subsequently the EL0 PC alignment handler was refactored to perform
> > branch predictor hardening before unmasking IRQs, in commit:
> > 
> >   bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening")
> > 
> > ... but the redundant masking of IRQs was not removed from the EL0 SP
> > alignment handler.
> 
> Bother.
> 
> 
> > Let's do so now, and make it interruptible as with most other
> > synchronous exception handlers.
> 
> I think you want:
> Fixes: bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening")
> 
> on this as, bfe298745afc9548 changed the behaviour: local_daif_restore(DAIF_PROCCTX) was
> called before arm64_notify_die(), now its not.
> 
> With that,
> Reviewed-by: James Morse <james.morse@arm.com>

Ah; I missed that subtlety.

I assume that Catalin can fold those in when applying. Otherwise I'll
add them for a v2.

Thanks,
Mark.

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

Re: [PATCH] arm64: entry: unmask IRQ in el0_sp()

[Will Deacon]

On Fri, Feb 28, 2020 at 04:08:09PM +0000, Mark Rutland wrote:
> On Fri, Feb 28, 2020 at 03:37:46PM +0000, James Morse wrote:
> > On 28/02/2020 14:59, Mark Rutland wrote:
> > > Let's do so now, and make it interruptible as with most other
> > > synchronous exception handlers.
> > 
> > I think you want:
> > Fixes: bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening")
> > 
> > on this as, bfe298745afc9548 changed the behaviour: local_daif_restore(DAIF_PROCCTX) was
> > called before arm64_notify_die(), now its not.
> > 
> > With that,
> > Reviewed-by: James Morse <james.morse@arm.com>
> 
> Ah; I missed that subtlety.
> 
> I assume that Catalin can fold those in when applying. Otherwise I'll
> add them for a v2.

If you want v2 to go in as a fix, please can you explain why this is a
problem (beyond disabling interrupts for longer than necessary) in the
commit message?

Cheers,

Will

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

Re: [PATCH] arm64: entry: unmask IRQ in el0_sp()

[James Morse]

Hi Mark, Will,

On 05/03/2020 20:30, Will Deacon wrote:
> On Fri, Feb 28, 2020 at 04:08:09PM +0000, Mark Rutland wrote:
>> On Fri, Feb 28, 2020 at 03:37:46PM +0000, James Morse wrote:
>>> On 28/02/2020 14:59, Mark Rutland wrote:
>>>> Let's do so now, and make it interruptible as with most other
>>>> synchronous exception handlers.
>>>
>>> I think you want:
>>> Fixes: bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening")
>>>
>>> on this as, bfe298745afc9548 changed the behaviour: local_daif_restore(DAIF_PROCCTX) was
>>> called before arm64_notify_die(), now its not.
>>>
>>> With that,
>>> Reviewed-by: James Morse <james.morse@arm.com>
>>
>> Ah; I missed that subtlety.
>>
>> I assume that Catalin can fold those in when applying. Otherwise I'll
>> add them for a v2.
> 
> If you want v2 to go in as a fix, please can you explain why this is a
> problem (beyond disabling interrupts for longer than necessary) in the
> commit message?

Good news, its not broken. I wrongly-assumed calling arm64_notify_die() and then
force_sig_fault() with both IRQ-unmasked and IRQ-masked would lead to problems, but the
guts of force_sig_fault() is all wrapped in spin_lock_irqsave().


Thanks,

James

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

Re: [PATCH] arm64: entry: unmask IRQ in el0_sp()

[Mark Rutland]

On Mon, Mar 09, 2020 at 04:04:58PM +0000, James Morse wrote:
> Hi Mark, Will,
> 
> On 05/03/2020 20:30, Will Deacon wrote:
> > On Fri, Feb 28, 2020 at 04:08:09PM +0000, Mark Rutland wrote:
> >> On Fri, Feb 28, 2020 at 03:37:46PM +0000, James Morse wrote:
> >>> On 28/02/2020 14:59, Mark Rutland wrote:
> >>>> Let's do so now, and make it interruptible as with most other
> >>>> synchronous exception handlers.
> >>>
> >>> I think you want:
> >>> Fixes: bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening")
> >>>
> >>> on this as, bfe298745afc9548 changed the behaviour: local_daif_restore(DAIF_PROCCTX) was
> >>> called before arm64_notify_die(), now its not.
> >>>
> >>> With that,
> >>> Reviewed-by: James Morse <james.morse@arm.com>
> >>
> >> Ah; I missed that subtlety.
> >>
> >> I assume that Catalin can fold those in when applying. Otherwise I'll
> >> add them for a v2.
> > 
> > If you want v2 to go in as a fix, please can you explain why this is a
> > problem (beyond disabling interrupts for longer than necessary) in the
> > commit message?
> 
> Good news, its not broken. I wrongly-assumed calling arm64_notify_die() and then
> force_sig_fault() with both IRQ-unmasked and IRQ-masked would lead to problems, but the
> guts of force_sig_fault() is all wrapped in spin_lock_irqsave().

Thanks for delving into that.

Catalin, are you happy to queue the patch for v5.7 with James' R-b (but
not the Fixes tag), and/or would you like me to send a v2 with that?

Thanks,
Mark.

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

Re: [PATCH] arm64: entry: unmask IRQ in el0_sp()

[Catalin Marinas]

On Tue, Mar 10, 2020 at 10:51:52AM +0000, Mark Rutland wrote:
> On Mon, Mar 09, 2020 at 04:04:58PM +0000, James Morse wrote:
> > Hi Mark, Will,
> > 
> > On 05/03/2020 20:30, Will Deacon wrote:
> > > On Fri, Feb 28, 2020 at 04:08:09PM +0000, Mark Rutland wrote:
> > >> On Fri, Feb 28, 2020 at 03:37:46PM +0000, James Morse wrote:
> > >>> On 28/02/2020 14:59, Mark Rutland wrote:
> > >>>> Let's do so now, and make it interruptible as with most other
> > >>>> synchronous exception handlers.
> > >>>
> > >>> I think you want:
> > >>> Fixes: bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening")
> > >>>
> > >>> on this as, bfe298745afc9548 changed the behaviour: local_daif_restore(DAIF_PROCCTX) was
> > >>> called before arm64_notify_die(), now its not.
> > >>>
> > >>> With that,
> > >>> Reviewed-by: James Morse <james.morse@arm.com>
> > >>
> > >> Ah; I missed that subtlety.
> > >>
> > >> I assume that Catalin can fold those in when applying. Otherwise I'll
> > >> add them for a v2.
> > > 
> > > If you want v2 to go in as a fix, please can you explain why this is a
> > > problem (beyond disabling interrupts for longer than necessary) in the
> > > commit message?
> > 
> > Good news, its not broken. I wrongly-assumed calling arm64_notify_die() and then
> > force_sig_fault() with both IRQ-unmasked and IRQ-masked would lead to problems, but the
> > guts of force_sig_fault() is all wrapped in spin_lock_irqsave().
> 
> Thanks for delving into that.
> 
> Catalin, are you happy to queue the patch for v5.7 with James' R-b (but
> not the Fixes tag), and/or would you like me to send a v2 with that?

No need to send a v2. Queued for 5.7. Thanks.

-- 
Catalin

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