Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200804135817.5495-1-daniel.gutson@eclypsium.com>
 <CAK8P3a0_fJ0BfD5Qvxdo0s7CbjPWaGA8QTgYhbXR=omafOHH4Q@mail.gmail.com>
 <CAFmMkTHEm8k+5GZkVJbDZMEhMwpsqVKRb-hGskSpBstdLRuFyA@mail.gmail.com>
 <CAK8P3a27bTYyn3N+tX=i_6f4KrQkOmkUA1zUQfvCW7qw6smSkQ@mail.gmail.com>
 <CAFmMkTF9eVm0tpOKEy2rzdX=Scr3RwqHDFy_i24R3F5ok-4=eA@mail.gmail.com>
 <CAK8P3a3mf8_Y4DWe3WuBO-Xo0N4Jj=-rrtFzD6w0TriGZPu1_g@mail.gmail.com> <CAFmMkTFzmC=aY0gR6urLu-8Oq8aeHBUWi-TodG8XhXKCcC057A@mail.gmail.com>
In-Reply-To: <CAFmMkTFzmC=aY0gR6urLu-8Oq8aeHBUWi-TodG8XhXKCcC057A@mail.gmail.com>
From:   Daniel Gutson <daniel@eclypsium.com>
Date:   Wed, 12 Aug 2020 12:41:35 -0300
Message-ID: <CAFmMkTE+2Qxo43bZkwCszEYbXFV22YdpLJD40gB6LgvnPbvdSA@mail.gmail.com>
Subject: Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash
 chip writable
To:     Arnd Bergmann <arnd@arndb.de>
Cc:     Tudor Ambarus <tudor.ambarus@microchip.com>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        Boris Brezillon <bbrezillon@kernel.org>,
        linux-mtd <linux-mtd@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Alex Bazhaniuk <alex@eclypsium.com>,
        Richard Hughes <hughsient@gmail.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

ping

On Tue, Aug 4, 2020 at 6:26 PM Daniel Gutson <daniel@eclypsium.com> wrote:
>
> On Tue, Aug 4, 2020 at 5:46 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, Aug 4, 2020 at 9:57 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> > > On Tue, Aug 4, 2020 at 4:06 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > On Tue, Aug 4, 2020 at 5:49 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> > > > > On Tue, Aug 4, 2020 at 12:21 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > >> On Tue, Aug 4, 2020 at 3:58 PM Daniel Gutson
> > > > >> <daniel.gutson@eclypsium.com> wrote:
> > > > >
> > > > > What about just saying
> > > > >
> > > > > "This patch removes the attempt by the intel-spi-pci driver to
> > > > > make the chip always writable."
> > > >
> > > > Yes, that is much better, though it still sounds like it would at the
> > > > moment allow writing to the device from software without also
> > > > setting the module parameter. I would say something like
> > > >
> > > > "Disallow overriding the write protection in the PCI driver
> > > > with a module parameter and instead honor the current
> > > > state of the write protection as set by the firmware."
> > >
> > > But wait, Mika, the author of the file, asked earlier not to remove
> > > the module parameter of intel-spi, and just remove the unconditional
> > > attempt to turn the chip writable in intle-spi-pci.
> >
> > Yes, and I think that is fine (aside from the inconsistency with bay trail
> > that you have not commented on),
>
> There are two inconsistencies before any of my patches:
> 1) in intel-spi.c: uses the module parameter only for bay trail.
> 2) intel-spi.c uses a module parameter whereas intel-spi-pci doesn't
>
> My initial patch addressed #2 by also adding a module parameter to
> intel-spi-pci,
> but then some of you discouraged me to use module parameters.
> Mika showed up and suggested to leave intel-spi.c as is (with its
> module parameter),
> and remove the code in intel-spi-pci that tried to turn the SPI chip
> writable if the BIOS
> was unlocked.
>
> > but that only touches the hardware
> > write-protection, which doesn't really have any effect unless user
> > space also configures the driver module to allow writing to the
> > mtd device.
> >
> > > So I'm not touching intel-pci, just removing that code from
> > > intel-spi-pci without adding a new module parameter.
> > >
> > > Are you aligned on this?
> >
> > One of us is still very confused about what the driver does.
> > You seem to have gone back to saying that without the
> > change a user could just write to the device even without
> > passing the module parameter to intel-spi.ko?
>
> What I'm trying to say is that, if the BIOS is unlocked
> (no driver involvement here), the intel-spi-pci turns the
> chip writable even without changing the module parameter of intel-spi.
> This is because the attempt to turn the chip writable occurs in
> the probing of intel-spi-pci, that is, earlier than the intel-spi
> initialization.
>
> >
> > Maybe you should start by explaining what scenario you
> > actually want to prevent here. Is it
>
> Was it clear from above?
>
> Before commenting below, it's important to note again that
> the driver will succeed in turning the chip writable only if the
> BIOS is unlocked by its build time specification.
> The WPD field (Write Protect Disable) bit only has effect if
> the BIOS is not locked. This WPD bit is the one that the intel-spi-pci
> driver tries to set unconditionally. If the BIOS is locked, it will cause
> no effect. But if the BIOS is not locked, the chip will
> end up in Write Protect Disabled state.
> My latest patch simply leaves alone the WPD bit in intel-spi-pci,
> not trying to set it to 1.
>
> I'm not sure the options below are now fully compatible
> with my explanation above.
>
> >
> > a) the hardware write-protect bit getting changed, which
> >     introduces the possibility of corrupting the flash even
> >     if nothing tries to write to it,
> >
> > b) root users setting the device writable with the intention
> >    of writing to it even though firmware has politely asked
> >    for this not to be done (by setting the write-protect bit
> >    but not preventing it from being disabled again), or
> >
> > c) a writeable mtd device showing up even without
> >     the module parameter being set at all?
> >
> > I thought the initial patch was about c) which turned out
> > to be a non-issue, and then the later patch being about b).
> >
> >      Arnd
>
>
>
> --
> Daniel Gutson
> Argentina Site Director
> Enginieering Director
> Eclypsium
>
> Below The Surface: Get the latest threat research and insights on
> firmware and supply chain threats from the research team at Eclypsium.
> https://eclypsium.com/research/#threatreport


-- 
Daniel Gutson
Argentina Site Director
Enginieering Director
Eclypsium

Below The Surface: Get the latest threat research and insights on
firmware and supply chain threats from the research team at Eclypsium.
https://eclypsium.com/research/#threatreport