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

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

[Daniel Gutson]

Currently, the intel-spi-pci driver tries to unconditionally set
the SPI chip writeable. After discussing in the LKML, the original
author decided that it was better  to remove the attempt.

Context, the intel-spi has a module argument that controls
whether the driver attempts to turn the SPI flash chip writeable.
The default value is FALSE (don't try to make it writeable).
However, this flag applies only for a number of devices, coming from the
platform driver, whereas the devices detected through the PCI driver
(intel-spi-pci) are not subject to this check since the configuration
takes place in intel-spi-pci which doesn't have an argument.

This patch removes the code that attempts to turn the SPI chip writeable.

Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
---
 drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
index 81329f680bec..2b071da804e7 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
+++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
@@ -41,14 +41,9 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
 	if (!info)
 		return -ENOMEM;
 
-	/* Try to make the chip read/write */
 	pci_read_config_dword(pdev, BCR, &bcr);
-	if (!(bcr & BCR_WPD)) {
-		bcr |= BCR_WPD;
-		pci_write_config_dword(pdev, BCR, bcr);
-		pci_read_config_dword(pdev, BCR, &bcr);
-	}
 	info->writeable = !!(bcr & BCR_WPD);
+	dev_dbg(&pdev->dev, "device is writeable: %d\n", (int)info->writeable);
 
 	ispi = intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
 	if (IS_ERR(ispi))
-- 
2.25.1

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

[Mika Westerberg]

On Tue, Aug 04, 2020 at 10:58:17AM -0300, Daniel Gutson wrote:
> Currently, the intel-spi-pci driver tries to unconditionally set
> the SPI chip writeable. After discussing in the LKML, the original
> author decided that it was better  to remove the attempt.
> 
> Context, the intel-spi has a module argument that controls
> whether the driver attempts to turn the SPI flash chip writeable.
> The default value is FALSE (don't try to make it writeable).
> However, this flag applies only for a number of devices, coming from the
> platform driver, whereas the devices detected through the PCI driver
> (intel-spi-pci) are not subject to this check since the configuration
> takes place in intel-spi-pci which doesn't have an argument.
> 
> This patch removes the code that attempts to turn the SPI chip writeable.
> 
> Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

[Arnd Bergmann]

On Tue, Aug 4, 2020 at 3:58 PM Daniel Gutson
<daniel.gutson@eclypsium.com> wrote:
>
> Context, the intel-spi has a module argument that controls
> whether the driver attempts to turn the SPI flash chip writeable.
> The default value is FALSE (don't try to make it writeable).
> However, this flag applies only for a number of devices, coming from the
> platform driver, whereas the devices detected through the PCI driver
> (intel-spi-pci) are not subject to this check since the configuration
> takes place in intel-spi-pci which doesn't have an argument.

This is still factually incorrect, as explained at least three times
now.

Please either make the same change for both the Bay Trail
platform driver and the PCI driver, or explain why you want them to
be different rather than incorrectly claiming that you change them to
be the same.

If you want the BIOS setting to always take precedence over the
module argument, logical change would be to also include
the corresponding change for bay trail and do:

--- a/drivers/mtd/spi-nor/controllers/intel-spi.c
+++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
@@ -324,19 +324,6 @@ static int intel_spi_init(struct intel_spi *ispi)
                ispi->nregions = BYT_FREG_NUM;
                ispi->pr_num = BYT_PR_NUM;
                ispi->swseq_reg = true;
-
-               if (writeable) {
-                       /* Disable write protection */
-                       val = readl(ispi->base + BYT_BCR);
-                       if (!(val & BYT_BCR_WPD)) {
-                               val |= BYT_BCR_WPD;
-                               writel(val, ispi->base + BYT_BCR);
-                               val = readl(ispi->base + BYT_BCR);
-                       }
-
-                       ispi->writeable = !!(val & BYT_BCR_WPD);
-               }
-
                break;

        case INTEL_SPI_LPT:

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

[Arnd Bergmann]

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:
>> >
>> > Context, the intel-spi has a module argument that controls
>> > whether the driver attempts to turn the SPI flash chip writeable.
>> > The default value is FALSE (don't try to make it writeable).
>> > However, this flag applies only for a number of devices, coming from the
>> > platform driver, whereas the devices detected through the PCI driver
>> > (intel-spi-pci) are not subject to this check since the configuration
>> > takes place in intel-spi-pci which doesn't have an argument.
>>
>> This is still factually incorrect, as explained at least three times
>> now.
>>
>> Please either make the same change for both the Bay Trail
>> platform driver and the PCI driver, or explain why you want them to
>> be different rather than incorrectly claiming that you change them to
>> be the same.
>
>
> 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."

(note also: imperative form in the patch description rather than
"this patch ...").

      Arnd

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

[Daniel Gutson]

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:
> >> >
> >> > Context, the intel-spi has a module argument that controls
> >> > whether the driver attempts to turn the SPI flash chip writeable.
> >> > The default value is FALSE (don't try to make it writeable).
> >> > However, this flag applies only for a number of devices, coming from the
> >> > platform driver, whereas the devices detected through the PCI driver
> >> > (intel-spi-pci) are not subject to this check since the configuration
> >> > takes place in intel-spi-pci which doesn't have an argument.
> >>
> >> This is still factually incorrect, as explained at least three times
> >> now.
> >>
> >> Please either make the same change for both the Bay Trail
> >> platform driver and the PCI driver, or explain why you want them to
> >> be different rather than incorrectly claiming that you change them to
> >> be the same.
> >
> >
> > 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.
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?


>
> (note also: imperative form in the patch description rather than
> "this patch ...").
>
>       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

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

[Arnd Bergmann]

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), 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?

Maybe you should start by explaining what scenario you
actually want to prevent here. Is it

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

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

[Daniel Gutson]

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

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

[Daniel Gutson]

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

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

[Greg Kroah-Hartman]

On Wed, Aug 12, 2020 at 12:41:35PM -0300, Daniel Gutson wrote:
> ping

What does that mean here?

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

[Greg Kroah-Hartman]

On Thu, Aug 13, 2020 at 08:42:57AM -0300, Daniel Gutson wrote:
> El jue., 13 ago. 2020 6:00 a. m., Greg Kroah-Hartman <
> gregkh@linuxfoundation.org> escribió:
> 
> > On Wed, Aug 12, 2020 at 12:41:35PM -0300, Daniel Gutson wrote:
> > > ping
> >
> > What does that mean here?
> >
> 
> I didn't get an answer from Arnd Bergman who expressed concerns. I want to
> know if my answer was enough or there are more potential changes I have to
> do.

No need to wait for anyone, it's not our job to always respond :)

Take what you feel is correct, fix up your patch along those lines, and
resubmit, that's all you can do.

good luck!

greg k-h

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

[Arnd Bergmann]

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. 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

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

[Daniel Gutson]

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

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

[Arnd Bergmann]

On Thu, Aug 13, 2020 at 11:40 PM Daniel Gutson <daniel@eclypsium.com> wrote:
>
> 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 module parameter is definitely honored on all hardware, but the driver
only cares about the functionality it provides through the mtd interface:

https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/controllers/intel-spi.c#L941

If you care about other (malicious) code writing to the driver, please explain
what the specific attack scenario is that you are worried about, and
why you think
this is not sufficient. What code would be able to write to the device
if not the
device driver itself?

    Arnd

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

[Daniel Gutson]

On Sun, Aug 16, 2020 at 5:42 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Aug 13, 2020 at 11:40 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> >
> > 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 module parameter is definitely honored on all hardware, but the driver
> only cares about the functionality it provides through the mtd interface:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/controllers/intel-spi.c#L941

That is a logical constraint which doesn't impact in the hardware, which already
was changed before in
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/controllers/intel-spi.c#L924

>
> If you care about other (malicious) code writing to the driver, please explain
> what the specific attack scenario is that you are worried about, and
> why you think
> this is not sufficient. What code would be able to write to the device
> if not the
> device driver itself?

Maybe Mika can answer this better, but what I'm trying to do is to
limit the possibility of
damage, as explained in the Kconfig:
"Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
"Say N here unless you know what you are doing. Overwriting the
  SPI flash may render the system unbootable."


>
>     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

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

[Mika Westerberg]

On Tue, Aug 18, 2020 at 12:55:59PM -0300, Daniel Gutson wrote:
> > If you care about other (malicious) code writing to the driver, please explain
> > what the specific attack scenario is that you are worried about, and
> > why you think
> > this is not sufficient. What code would be able to write to the device
> > if not the
> > device driver itself?
> 
> Maybe Mika can answer this better, but what I'm trying to do is to
> limit the possibility of
> damage, as explained in the Kconfig:
> "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> "Say N here unless you know what you are doing. Overwriting the
>   SPI flash may render the system unbootable."

Right, the PCI part of the driver unconditionally (and wrongly) tried to
set the chip writeable.

What this whole thing tries to protect is that the user does not
accidentally write to the flash chip. It contains BIOS and other
important firmware so touching it (if it is not locked in the BIOS side)
may potentially brick the system. That's why we also require that
command line parameter so the user who knows what he or she is doing can
enable it for writing.

Actually thinking about this bit more, to make PCI and the platform
parts consistent we can make the "writeable" control this for the PCI
part as well. So what if we add a callback to struct intel_spi_boardinfo
that the PCI driver populates and then the "core" driver uses to enable
writing when "writeable" is set to 1.

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

[Arnd Bergmann]

On Wed, Aug 19, 2020 at 8:57 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Tue, Aug 18, 2020 at 12:55:59PM -0300, Daniel Gutson wrote:
> > > If you care about other (malicious) code writing to the driver, please explain
> > > what the specific attack scenario is that you are worried about, and
> > > why you think
> > > this is not sufficient. What code would be able to write to the device
> > > if not the
> > > device driver itself?
> >
> > Maybe Mika can answer this better, but what I'm trying to do is to
> > limit the possibility of
> > damage, as explained in the Kconfig:
> > "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> > "Say N here unless you know what you are doing. Overwriting the
> >   SPI flash may render the system unbootable."
>
> Right, the PCI part of the driver unconditionally (and wrongly) tried to
> set the chip writeable.
>
> What this whole thing tries to protect is that the user does not
> accidentally write to the flash chip. It contains BIOS and other
> important firmware so touching it (if it is not locked in the BIOS side)
> may potentially brick the system. That's why we also require that
> command line parameter so the user who knows what he or she is doing can
> enable it for writing.

The same thing can happen with the platform driver if you load it
once with 'writeable=1' and then unload, leaving the chip in writeable
state. If you load it a second time without the module parameter, it
will be in the same state as the PCI driver: the hardware bit allows
writing, but the MTD layer prevents writes from being issued to the
device.

> Actually thinking about this bit more, to make PCI and the platform
> parts consistent we can make the "writeable" control this for the PCI
> part as well. So what if we add a callback to struct intel_spi_boardinfo
> that the PCI driver populates and then the "core" driver uses to enable
> writing when "writeable" is set to 1.

If you are really worried about the write protection being bypassed by
a different driver or code injection, the best way would seem to be to
only enable writing in the mtd write callback and disable it immediately
after the write is complete. I still don't see why this hardware would
be more susceptible to this kind of attack than other drivers though,
as it already has the safeguard against writing through the MTD layer
without the module parameter.

       Arnd

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

[Mika Westerberg]

On Wed, Aug 19, 2020 at 10:38:24AM +0200, Arnd Bergmann wrote:
> On Wed, Aug 19, 2020 at 8:57 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > On Tue, Aug 18, 2020 at 12:55:59PM -0300, Daniel Gutson wrote:
> > > > If you care about other (malicious) code writing to the driver, please explain
> > > > what the specific attack scenario is that you are worried about, and
> > > > why you think
> > > > this is not sufficient. What code would be able to write to the device
> > > > if not the
> > > > device driver itself?
> > >
> > > Maybe Mika can answer this better, but what I'm trying to do is to
> > > limit the possibility of
> > > damage, as explained in the Kconfig:
> > > "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> > > "Say N here unless you know what you are doing. Overwriting the
> > >   SPI flash may render the system unbootable."
> >
> > Right, the PCI part of the driver unconditionally (and wrongly) tried to
> > set the chip writeable.
> >
> > What this whole thing tries to protect is that the user does not
> > accidentally write to the flash chip. It contains BIOS and other
> > important firmware so touching it (if it is not locked in the BIOS side)
> > may potentially brick the system. That's why we also require that
> > command line parameter so the user who knows what he or she is doing can
> > enable it for writing.
> 
> The same thing can happen with the platform driver if you load it
> once with 'writeable=1' and then unload, leaving the chip in writeable
> state. If you load it a second time without the module parameter, it
> will be in the same state as the PCI driver: the hardware bit allows
> writing, but the MTD layer prevents writes from being issued to the
> device.

Right.

> > Actually thinking about this bit more, to make PCI and the platform
> > parts consistent we can make the "writeable" control this for the PCI
> > part as well. So what if we add a callback to struct intel_spi_boardinfo
> > that the PCI driver populates and then the "core" driver uses to enable
> > writing when "writeable" is set to 1.
> 
> If you are really worried about the write protection being bypassed by
> a different driver or code injection, the best way would seem to be to
> only enable writing in the mtd write callback and disable it immediately
> after the write is complete. I still don't see why this hardware would
> be more susceptible to this kind of attack than other drivers though,
> as it already has the safeguard against writing through the MTD layer
> without the module parameter.

Hmm, is there already a mechanism at the MTD level to prevent writes? If
that's the case then sure we can use that instead.

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

[Arnd Bergmann]

On Wed, Aug 19, 2020 at 11:11 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Aug 19, 2020 at 10:38:24AM +0200, Arnd Bergmann wrote:
> > On Wed, Aug 19, 2020 at 8:57 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
>
> > > Actually thinking about this bit more, to make PCI and the platform
> > > parts consistent we can make the "writeable" control this for the PCI
> > > part as well. So what if we add a callback to struct intel_spi_boardinfo
> > > that the PCI driver populates and then the "core" driver uses to enable
> > > writing when "writeable" is set to 1.
> >
> > If you are really worried about the write protection being bypassed by
> > a different driver or code injection, the best way would seem to be to
> > only enable writing in the mtd write callback and disable it immediately
> > after the write is complete. I still don't see why this hardware would
> > be more susceptible to this kind of attack than other drivers though,
> > as it already has the safeguard against writing through the MTD layer
> > without the module parameter.
>
> Hmm, is there already a mechanism at the MTD level to prevent writes? If
> that's the case then sure we can use that instead.

The mtd core just checks both the permissions on the device node (which
default to 0600 without any special udev rules) and the MTD_WRITEABLE
on the underlying device that is controlled by the module parameter
in case of intel-spi{,-platform,-pci}.c.

     Arnd

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

[Mika Westerberg]

On Sat, Aug 22, 2020 at 06:06:03PM +0200, Arnd Bergmann wrote:
> On Wed, Aug 19, 2020 at 11:11 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Wed, Aug 19, 2020 at 10:38:24AM +0200, Arnd Bergmann wrote:
> > > On Wed, Aug 19, 2020 at 8:57 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> >
> > > > Actually thinking about this bit more, to make PCI and the platform
> > > > parts consistent we can make the "writeable" control this for the PCI
> > > > part as well. So what if we add a callback to struct intel_spi_boardinfo
> > > > that the PCI driver populates and then the "core" driver uses to enable
> > > > writing when "writeable" is set to 1.
> > >
> > > If you are really worried about the write protection being bypassed by
> > > a different driver or code injection, the best way would seem to be to
> > > only enable writing in the mtd write callback and disable it immediately
> > > after the write is complete. I still don't see why this hardware would
> > > be more susceptible to this kind of attack than other drivers though,
> > > as it already has the safeguard against writing through the MTD layer
> > > without the module parameter.
> >
> > Hmm, is there already a mechanism at the MTD level to prevent writes? If
> > that's the case then sure we can use that instead.
> 
> The mtd core just checks both the permissions on the device node (which
> default to 0600 without any special udev rules) and the MTD_WRITEABLE
> on the underlying device that is controlled by the module parameter
> in case of intel-spi{,-platform,-pci}.c.

OK, thanks.

Since we cannot really get rid of the module parameter (AFAIK there are
users for it), I still think we should just make the "writeable" to
apply to the PCI part as well. That should at least make it consistent,
and it also solves Daniel's case.

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

[Arnd Bergmann]

On Mon, Aug 24, 2020 at 10:22 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Sat, Aug 22, 2020 at 06:06:03PM +0200, Arnd Bergmann wrote:
> > On Wed, Aug 19, 2020 at 11:11 AM Mika Westerberg
> >
> > The mtd core just checks both the permissions on the device node (which
> > default to 0600 without any special udev rules) and the MTD_WRITEABLE
> > on the underlying device that is controlled by the module parameter
> > in case of intel-spi{,-platform,-pci}.c.
>
> OK, thanks.
>
> Since we cannot really get rid of the module parameter (AFAIK there are
> users for it), I still think we should just make the "writeable" to
> apply to the PCI part as well. That should at least make it consistent,
> and it also solves Daniel's case.

Can you explain Daniel's case then? I still don't understand what he
actually wants.

As I keep repeating, the module parameter *does* apply to the pci
driver front-end since it determines whether the driver will disallow
writes to the mtd device without it. The only difference is that the pci
driver will attempt to set the hardware bit without checking the
module parameter first, while the platform driver does not. If the
module parameter is not set however, the state of the hardware
bit is never checked again.

    Arnd

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

[Mika Westerberg]

On Mon, Aug 24, 2020 at 11:08:33AM +0200, Arnd Bergmann wrote:
> On Mon, Aug 24, 2020 at 10:22 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Sat, Aug 22, 2020 at 06:06:03PM +0200, Arnd Bergmann wrote:
> > > On Wed, Aug 19, 2020 at 11:11 AM Mika Westerberg
> > >
> > > The mtd core just checks both the permissions on the device node (which
> > > default to 0600 without any special udev rules) and the MTD_WRITEABLE
> > > on the underlying device that is controlled by the module parameter
> > > in case of intel-spi{,-platform,-pci}.c.
> >
> > OK, thanks.
> >
> > Since we cannot really get rid of the module parameter (AFAIK there are
> > users for it), I still think we should just make the "writeable" to
> > apply to the PCI part as well. That should at least make it consistent,
> > and it also solves Daniel's case.
> 
> Can you explain Daniel's case then? I still don't understand what he
> actually wants.
> 
> As I keep repeating, the module parameter *does* apply to the pci
> driver front-end since it determines whether the driver will disallow
> writes to the mtd device without it. The only difference is that the pci
> driver will attempt to set the hardware bit without checking the
> module parameter first, while the platform driver does not. If the
> module parameter is not set however, the state of the hardware
> bit is never checked again.

I think Daniel wants the PCI driver not to set the hardware bit by
default (same as the platform driver).

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

[Arnd Bergmann]

On Mon, Aug 24, 2020 at 11:15 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Aug 24, 2020 at 11:08:33AM +0200, Arnd Bergmann wrote:
> > On Mon, Aug 24, 2020 at 10:22 AM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > On Sat, Aug 22, 2020 at 06:06:03PM +0200, Arnd Bergmann wrote:
> > > > On Wed, Aug 19, 2020 at 11:11 AM Mika Westerberg
> > > >
> > > > The mtd core just checks both the permissions on the device node (which
> > > > default to 0600 without any special udev rules) and the MTD_WRITEABLE
> > > > on the underlying device that is controlled by the module parameter
> > > > in case of intel-spi{,-platform,-pci}.c.
> > >
> > > OK, thanks.
> > >
> > > Since we cannot really get rid of the module parameter (AFAIK there are
> > > users for it), I still think we should just make the "writeable" to
> > > apply to the PCI part as well. That should at least make it consistent,
> > > and it also solves Daniel's case.
> >
> > Can you explain Daniel's case then? I still don't understand what he
> > actually wants.
> >
> > As I keep repeating, the module parameter *does* apply to the pci
> > driver front-end since it determines whether the driver will disallow
> > writes to the mtd device without it. The only difference is that the pci
> > driver will attempt to set the hardware bit without checking the
> > module parameter first, while the platform driver does not. If the
> > module parameter is not set however, the state of the hardware
> > bit is never checked again.
>
> I think Daniel wants the PCI driver not to set the hardware bit by
> default (same as the platform driver).

Sure, but *why*?

    Arnd

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

[Mika Westerberg]

On Mon, Aug 24, 2020 at 11:31:40AM +0200, Arnd Bergmann wrote:
> On Mon, Aug 24, 2020 at 11:15 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Mon, Aug 24, 2020 at 11:08:33AM +0200, Arnd Bergmann wrote:
> > > On Mon, Aug 24, 2020 at 10:22 AM Mika Westerberg
> > > <mika.westerberg@linux.intel.com> wrote:
> > > > On Sat, Aug 22, 2020 at 06:06:03PM +0200, Arnd Bergmann wrote:
> > > > > On Wed, Aug 19, 2020 at 11:11 AM Mika Westerberg
> > > > >
> > > > > The mtd core just checks both the permissions on the device node (which
> > > > > default to 0600 without any special udev rules) and the MTD_WRITEABLE
> > > > > on the underlying device that is controlled by the module parameter
> > > > > in case of intel-spi{,-platform,-pci}.c.
> > > >
> > > > OK, thanks.
> > > >
> > > > Since we cannot really get rid of the module parameter (AFAIK there are
> > > > users for it), I still think we should just make the "writeable" to
> > > > apply to the PCI part as well. That should at least make it consistent,
> > > > and it also solves Daniel's case.
> > >
> > > Can you explain Daniel's case then? I still don't understand what he
> > > actually wants.
> > >
> > > As I keep repeating, the module parameter *does* apply to the pci
> > > driver front-end since it determines whether the driver will disallow
> > > writes to the mtd device without it. The only difference is that the pci
> > > driver will attempt to set the hardware bit without checking the
> > > module parameter first, while the platform driver does not. If the
> > > module parameter is not set however, the state of the hardware
> > > bit is never checked again.
> >
> > I think Daniel wants the PCI driver not to set the hardware bit by
> > default (same as the platform driver).
> 
> Sure, but *why*?

Because this is part of the platform firmware security check patch he is
also working on and, I guess making the flash chip writeable by default
is triggering some of the checks in that patch. Or something along those
lines ;-)

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

[Daniel Gutson]

On Mon, Aug 24, 2020 at 6:44 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Mon, Aug 24, 2020 at 11:31:40AM +0200, Arnd Bergmann wrote:
> > On Mon, Aug 24, 2020 at 11:15 AM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > On Mon, Aug 24, 2020 at 11:08:33AM +0200, Arnd Bergmann wrote:
> > > > On Mon, Aug 24, 2020 at 10:22 AM Mika Westerberg
> > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > On Sat, Aug 22, 2020 at 06:06:03PM +0200, Arnd Bergmann wrote:
> > > > > > On Wed, Aug 19, 2020 at 11:11 AM Mika Westerberg
> > > > > >
> > > > > > The mtd core just checks both the permissions on the device node (which
> > > > > > default to 0600 without any special udev rules) and the MTD_WRITEABLE
> > > > > > on the underlying device that is controlled by the module parameter
> > > > > > in case of intel-spi{,-platform,-pci}.c.
> > > > >
> > > > > OK, thanks.
> > > > >
> > > > > Since we cannot really get rid of the module parameter (AFAIK there are
> > > > > users for it), I still think we should just make the "writeable" to
> > > > > apply to the PCI part as well. That should at least make it consistent,
> > > > > and it also solves Daniel's case.
> > > >
> > > > Can you explain Daniel's case then? I still don't understand what he
> > > > actually wants.
> > > >
> > > > As I keep repeating, the module parameter *does* apply to the pci
> > > > driver front-end since it determines whether the driver will disallow
> > > > writes to the mtd device without it. The only difference is that the pci
> > > > driver will attempt to set the hardware bit without checking the
> > > > module parameter first, while the platform driver does not. If the
> > > > module parameter is not set however, the state of the hardware
> > > > bit is never checked again.
> > >
> > > I think Daniel wants the PCI driver not to set the hardware bit by
> > > default (same as the platform driver).
> >
> > Sure, but *why*?
>
> Because this is part of the platform firmware security check patch he is
> also working on and, I guess making the flash chip writeable by default
> is triggering some of the checks in that patch. Or something along those
> lines ;-)

Correct. I need these drivers to be enabled by default, but they are
documented as "DANGEROUS", so I want to also split the driver
in read-only mode and R/W mode. Currently, the driver is R/W,
and this would be a little first step in that direction.

-- 
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

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

[David Laight]

From: Arnd Bergmann
> Sent: 19 August 2020 09:38
...
> If you are really worried about the write protection being bypassed by
> a different driver or code injection, the best way would seem to be to
> only enable writing in the mtd write callback and disable it immediately
> after the write is complete. I still don't see why this hardware would
> be more susceptible to this kind of attack than other drivers though,
> as it already has the safeguard against writing through the MTD layer
> without the module parameter.

It is pretty unlikely that anyone will accidentally do an spi write
(it is all too complicated).
Anything that is being mischievous can send the write enable
command itself.

If you care you need to use the device pin to disable writes.

	David
 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)