linux-kernel - Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAPYmKFu8_72K21GL15W6fwyUv36FfEOcQKCv7dKgoT29gOwhSg@mail.gmail.com>
Date: Wed, 19 Jun 2024 11:53:14 +0800
From: Xu Lu <luxu.kernel@...edance.com>
To: Alexandre Ghiti <alex@...ti.fr>
Cc: Andrea Parri <parri.andrea@...il.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Paul Walmsley <paul.walmsley@...ive.com>, aou@...s.berkeley.edu, 
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags
 indicates irq disabled

Hi Alex,

On Tue, Jun 18, 2024 at 7:52 PM Alexandre Ghiti <alex@...ti.fr> wrote:
>
> Hi Andrea,
>
> On 18/06/2024 13:05, Andrea Parri wrote:
> > (merging replies)
> >
> >>>> However, if local_irq_save() is called when irq is disabled. The SR_IE bit of
> >>>> flag returned is clear. If some code between local_irq_save() and
> >>>> local_irq_restore() reenables irq, causing the SR_IE bit of CSR_STATUS
> >>>> back to 1, then local_irq_restore() can not restore irq status back to disabled.
> > But doesn't that represent some bogus manipulation of the irq flags? cf.
> >
> > config DEBUG_IRQFLAGS
> >       bool "Debug IRQ flag manipulation"
> >       help
> >         Enables checks for potentially unsafe enabling or disabling of
> >         interrupts, such as calling raw_local_irq_restore() when interrupts
> >         are enabled.
> >
> > in particular, raw_check_bogus_irq_restore() in raw_local_irq_restore().
> >
> >
> >> This got lost but this is still correct and needed.
> > You mean because of the mentioned rtl8723e example? are there other such
> > instances?  IOW, why do you say that the changes in question are needed?
>
>
> Simply because the scenario in this driver and I looked at the arm64
> implementation which restores flags unconditionally.

Indeed, ARM64 implementation restores flags unconditionally.

Actually in the beginning I found several drivers use
local_irq_restore after local_irq_enable.
Such drivers include 'scsi/arm/acornscsi.c',
'net/wireless/realtek/rtlwifi/rtlxxx/hw.c', etc.
I wonder whether the semantics of local_irq_restore should be
restoring irq flags unconditionally and thus I submitted this patch.

>
> But if that's considered bogus, let's drop this. Sorry Xu for the noise,
> and thanks Andrea for pointing this.

It's OK if we think this is not a problem. Just ignore this patch.

Thanks!

Xu Lu

>
> Alex
>
>
> >
> >    Andrea