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> <CAK8P3a13N_wNORz_3cYHTN8t29pPrY+dJ+g+1Ga_MmG1TmrUQw@mail.gmail.com>
In-Reply-To: <CAK8P3a13N_wNORz_3cYHTN8t29pPrY+dJ+g+1Ga_MmG1TmrUQw@mail.gmail.com>
From:   Daniel Gutson <daniel@eclypsium.com>
Date:   Thu, 13 Aug 2020 18:40:34 -0300
Message-ID: <CAFmMkTGm3pMsBzEenYOsRbhOZKFhbHiZ5LxPyVmTdYTTRpKzVQ@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

On Thu, Aug 13, 2020 at 12:41 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Aug 4, 2020 at 11:26 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> > On Tue, Aug 4, 2020 at 5:46 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > 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
>
> Neither of these matches what I see in the source code. Please
> check again.
>
> Once more: intel-spi.c has a module parameter that controls writing
> to the device regardless of the back-end (platform or pci), purely
> in software.

If I understand you correctly, this is not what I see:
If the deviceID is listed in intel-spi-pci.c
(https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/controllers/intel-spi-pci.c#L66)
then intel_spi_pci_probe will be called, where it unconditionally will
try to make the chip writable
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/controllers/intel-spi-pci.c#L44
These devices correspond to the BXT and CNL devices (lines 19 and 23 resp.).

Lines later (53), it will call intel-spi.c 's intel_spi_probe
function, which ends up calling intel_spi_init,
which checks for the type
(https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/controllers/intel-spi.c#L313)
It is in this switch where the module parameter is checked, but only
in the BYT case; however,
flow coming from intel-spi-pci is BXT and CNL as mentioned before,
landing in their case labels (lines 343 and 351 respectively)
where the module parameter is not checked.

Therefore, for BXT and CNL probed in intel-spi-pci, the chip is turned
writable and later the module parameter is not honored.

> The hardware write-protect setting where available
> works in addition that and prevents writing even if the module
> parameter is set to writeable.
>
> > > 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.
>
> My question was why you even care whether the hardware
> bit is set to writeable if the driver disallows writing. I think the
> answer is that you misread the driver.
>
>        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