Messages in this thread | | | From | Daniel Gutson <> | Date | Wed, 12 Aug 2020 12:41:35 -0300 | Subject | Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable |
| |
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
| |