Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable

[Daniel Gutson <daniel@eclypsium.com>]

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