linux-kernel - Re: [PATCH v2] i2c: designware: Fix corrupted memory seen in the ISR

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZQlwC9TCSwWJpuxy@arm.com>
Date:   Tue, 19 Sep 2023 10:55:23 +0100
From:   Catalin Marinas <catalin.marinas@....com>
To:     Jan Bottorff <janb@...amperecomputing.com>
Cc:     Serge Semin <fancer.lancer@...il.com>,
        Yann Sionneau <yann@...nneau.net>,
        Will Deacon <will@...nel.org>,
        Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Jan Dabros <jsd@...ihalf.com>,
        Andi Shyti <andi.shyti@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        Yann Sionneau <ysionneau@...rayinc.com>
Subject: Re: [PATCH v2] i2c: designware: Fix corrupted memory seen in the ISR

On Mon, Sep 18, 2023 at 08:45:42PM -0700, Jan Bottorff wrote:
> > > > > > > diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> > > > > > > index ca1035e010c7..1694ac6bb592 100644
> > > > > > > --- a/drivers/i2c/busses/i2c-designware-master.c
> > > > > > > +++ b/drivers/i2c/busses/i2c-designware-master.c
> > > > > > > @@ -248,6 +248,14 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
> > > > > > >         /* Dummy read to avoid the register getting stuck on Bay Trail */
> > > > > > >         regmap_read(dev->map, DW_IC_ENABLE_STATUS, &dummy);
> > > > > > > 
> > > > > > > +     /*
> > > > > > > +      * To guarantee data written by the current core is visible to
> > > > > > > +      * all cores, a write barrier is required. This needs to be
> > > > > > > +      * before an interrupt causes execution on another core.
> > > > > > > +      * For ARM processors, this needs to be a DSB barrier.
> > > > > > > +      */
> > > > > > > +     wmb();
[...]
> I did find the below text in the Arm Architectural Reference Manual (DDI
> 0487I.a) section K13.4 "Using a mailbox to send an interrupt". It was nearly
> the same wording as the ARM barrier document I previously referenced at
> https://developer.arm.com/documentation/genc007826/latest/ This too says a
> DSB barrier is required for memory updates to be observable in the ISR.
> 
> "
> K13.4 Using a mailbox to send an interrupt
>   In some message passing systems, it is common for one observer to update
> memory and then notify a second observer of the update by sending an
> interrupt, using a mailbox. Although a memory access might be made to
> initiate the sending of the mailbox interrupt, a DSB instruction is
> required to ensure the completion of previous memory accesses.
> 
> Therefore, the following sequence is required to ensure that P2 observes the
> updated value:
> 
> AArch32
> P1
>   STR R5, [R1] ; message stored to shared memory location
>   DSB ST
>   STR R0, [R4] ; R4 contains the address of a mailbox
> P2
>   ; interrupt service routine
>   LDR R5, [R1]
> 
> AArch64
> P1
>   STR W5, [X1] ; message stored to shared memory location
>   DSB ST
>   STR W0, [X4] ; R4 contains the address of a mailbox
> P2
>   ; interrupt service routine
>   LDR W5, [X1]
> "

Will convinced me in the past that a DMB is sufficient here unless the
peripheral is CPU-local. The Arm ARM is not entirely clear here.

> I hear your concern about how this barrier in platform portable code may
> impact platforms other than the one I'm trying to fix. It almost seems like
> there is some missing type of barrier macro that on ARM64 does what is
> required for cases like this and on other platforms does whatever is
> appropriate for that platform, often nothing.

I also agree that a wmb() in the i2c driver is not the more elegant fix.
For similar reasons, we hid barriers in the write*() macros, drivers
need to stay architecture-agnostic as much as possible.

Where does the regmap_write() end up? I think the barrier should be
somewhere down this path.

-- 
Catalin