[PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Daniel Gutson]

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

That's why I propose this patch to add such argument to intel-spi-pci,
so the user can control whether the driver tries to make the chip
writeable or not, being the default FALSE as is the argument of
intel-spi.

Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
---
 drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 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..77e57450f166 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
+++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
@@ -24,6 +24,10 @@ static const struct intel_spi_boardinfo cnl_info = {
 	.type = INTEL_SPI_CNL,
 };
 
+static bool writeable;
+module_param(writeable, bool, 0);
+MODULE_PARM_DESC(writeable, "Enable write access to SPI flash chip (default=0)");
+
 static int intel_spi_pci_probe(struct pci_dev *pdev,
 			       const struct pci_device_id *id)
 {
@@ -41,12 +45,14 @@ 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);
+	if (writeable) {
+		/* 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);
 
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Greg Kroah-Hartman]

On Fri, Jul 24, 2020 at 06:28:53PM -0300, Daniel Gutson wrote:
> Currently, 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.
> 
> That's why I propose this patch to add such argument to intel-spi-pci,
> so the user can control whether the driver tries to make the chip
> writeable or not, being the default FALSE as is the argument of
> intel-spi.
> 
> Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> ---
>  drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 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..77e57450f166 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> @@ -24,6 +24,10 @@ static const struct intel_spi_boardinfo cnl_info = {
>  	.type = INTEL_SPI_CNL,
>  };
>  
> +static bool writeable;
> +module_param(writeable, bool, 0);
> +MODULE_PARM_DESC(writeable, "Enable write access to SPI flash chip (default=0)");

Ick, this isn't the 1990's, please do not add new module parameters,
they are a major pain to work with and only work on a global basis, not
on a per-device basis.

No user will remember how to use this, as it isn't documented anywhere
either.  Can you make this a sysfs attribute or something, or better
yet, make it "just work" depending on the device type?

thanks,

greg k-h

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Greg Kroah-Hartman]

On Sat, Jul 25, 2020 at 02:20:03PM -0300, Daniel Gutson wrote:
> El sáb., 25 jul. 2020 2:56 a. m., Greg Kroah-Hartman <
> gregkh@linuxfoundation.org> escribió:
> 
> > On Fri, Jul 24, 2020 at 06:28:53PM -0300, Daniel Gutson wrote:
> > > Currently, 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.
> > >
> > > That's why I propose this patch to add such argument to intel-spi-pci,
> > > so the user can control whether the driver tries to make the chip
> > > writeable or not, being the default FALSE as is the argument of
> > > intel-spi.
> > >
> > > Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> > > ---
> > >  drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 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..77e57450f166 100644
> > > --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> > > +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> > > @@ -24,6 +24,10 @@ static const struct intel_spi_boardinfo cnl_info = {
> > >       .type = INTEL_SPI_CNL,
> > >  };
> > >
> > > +static bool writeable;
> > > +module_param(writeable, bool, 0);
> > > +MODULE_PARM_DESC(writeable, "Enable write access to SPI flash chip
> > (default=0)");
> >
> > Ick, this isn't the 1990's, please do not add new module parameters,
> > they are a major pain to work with and only work on a global basis, not
> > on a per-device basis.
> >
> > No user will remember how to use this, as it isn't documented anywhere
> > either.  Can you make this a sysfs attribute or something, or better
> > yet, make it "just work" depending on the device type?
> >
> 
> 1) I just did the same that intel-spi.c does.

No need to copy bad examples :)

> You need to understand that there's a set of DIDs coming from the
> lpc_ich (and then the platform) driver, and another set from
> intel-spi-pci. The first set is subject to the check, the second
> doesn't. So there is no "just work" as I understand it.

I have no idea what "DID" is here, sorry.

But why do you want to write it?

> 2) this is an initialization argument, I could make it always NOT attempt
> to make the chip writable, and a system attribute to turn it writable
> post-initialization but the behavior is not the same. In any ways, as I
> mentioned before, some DIDs will be covered by the existing argument of
> intel-spi and other DIDs by this sysfs attribute, becoming IMHO
> inconsistent from the user POV.

What sysfs attribute?  This is a module parameter :(

totally confused,

greg k-h

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Arnd Bergmann]

On Mon, Jul 27, 2020 at 5:05 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> On Sun, Jul 26, 2020 at 4:17 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>>
>> On Sat, Jul 25, 2020 at 02:20:03PM -0300, Daniel Gutson wrote:
>> > El sáb., 25 jul. 2020 2:56 a. m., Greg Kroah-Hartman <
>> > gregkh@linuxfoundation.org> escribió:
>> >
>> >
>> > 1) I just did the same that intel-spi.c does.
>>
>> No need to copy bad examples :)
>
>
> Didn't know it was a bad example. What's is the current modern mechanism that replaces initialization-time configuration?

I'd say you'd generally want this to be a per-instance setting, which
could be a sysfs attribute of the physical device, or an ioctl for an
existing user space abstraction.

In the changelog, you should also explain what this is used for. Do
you actually want to write to a device that is marked read-only, or
are you just trying to make the interface more consistent between the
two drivers?

     Arnd

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Daniel Gutson]

On Mon, Jul 27, 2020 at 12:15 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Jul 27, 2020 at 5:05 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> > On Sun, Jul 26, 2020 at 4:17 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >>
> >> On Sat, Jul 25, 2020 at 02:20:03PM -0300, Daniel Gutson wrote:
> >> > El sáb., 25 jul. 2020 2:56 a. m., Greg Kroah-Hartman <
> >> > gregkh@linuxfoundation.org> escribió:
> >> >
> >> >
> >> > 1) I just did the same that intel-spi.c does.
> >>
> >> No need to copy bad examples :)
> >
> >
> > Didn't know it was a bad example. What's is the current modern mechanism that replaces initialization-time configuration?
>
> I'd say you'd generally want this to be a per-instance setting, which
> could be a sysfs attribute of the physical device, or an ioctl for an
> existing user space abstraction.

But still, they are not equivalent. The initial configuration remains
constant for the rest of the life of the driver, whereas the
sysfs attribute is set after the initialization and can be re-set over
time. I'm not asking module parameters "to come back" if
they are now considered obsolete, I'm just trying to understand.

>
> In the changelog, you should also explain what this is used for. Do
> you actually want to write to a device that is marked read-only, or
> are you just trying to make the interface more consistent between the
> two drivers?

The device can either be locked or unlocked. If it is unlocked, it can
be set writable depending on the module
argument in intel-spi, or straight writable in intel-spi-pci. Which is
dangerous.
I wanted to make, as you say, the interface consistent.
Exposing an interface to the user (via a sysfs attribute) to try to
make the driver writable is definitively a bad idea.
I'd rather do nothing (no module arg) rather than open that
here-be-dragons door.
>
>      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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Daniel Gutson]

On Mon, Jul 27, 2020 at 12:31 PM Daniel Gutson <daniel@eclypsium.com> wrote:
>
> On Mon, Jul 27, 2020 at 12:15 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Mon, Jul 27, 2020 at 5:05 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> > > On Sun, Jul 26, 2020 at 4:17 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > >>
> > >> On Sat, Jul 25, 2020 at 02:20:03PM -0300, Daniel Gutson wrote:
> > >> > El sáb., 25 jul. 2020 2:56 a. m., Greg Kroah-Hartman <
> > >> > gregkh@linuxfoundation.org> escribió:
> > >> >
> > >> >
> > >> > 1) I just did the same that intel-spi.c does.
> > >>
> > >> No need to copy bad examples :)
> > >
> > >
> > > Didn't know it was a bad example. What's is the current modern mechanism that replaces initialization-time configuration?
> >
> > I'd say you'd generally want this to be a per-instance setting, which
> > could be a sysfs attribute of the physical device, or an ioctl for an
> > existing user space abstraction.
>
> But still, they are not equivalent. The initial configuration remains
> constant for the rest of the life of the driver, whereas the
> sysfs attribute is set after the initialization and can be re-set over
> time. I'm not asking module parameters "to come back" if
> they are now considered obsolete, I'm just trying to understand.
>
> >
> > In the changelog, you should also explain what this is used for. Do
> > you actually want to write to a device that is marked read-only, or
> > are you just trying to make the interface more consistent between the
> > two drivers?
>
> The device can either be locked or unlocked. If it is unlocked, it can
> be set writable depending on the module
> argument in intel-spi, or straight writable in intel-spi-pci. Which is
> dangerous.
> I wanted to make, as you say, the interface consistent.
> Exposing an interface to the user (via a sysfs attribute) to try to
> make the driver writable is definitively a bad idea.
> I'd rather do nothing (no module arg) rather than open that
> here-be-dragons door.

ping.
This is a real existing problem that I'm trying to address.
There's currently no way to prevent the kernel to try to turn
the SPI flash chip writable for the device IDs handled by
intel-spi-pci. And allowing userspace to switch it between
writable/non-writable is not an option.
What other mechanism can I use other than the module parameter,
so
 - not accessible through user space
 - ideally set once, ideally at initialization time

Thanks,

    Daniel.


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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Greg Kroah-Hartman]

On Wed, Jul 29, 2020 at 05:54:35PM -0300, Daniel Gutson wrote:
> On Mon, Jul 27, 2020 at 12:31 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> >
> > On Mon, Jul 27, 2020 at 12:15 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > On Mon, Jul 27, 2020 at 5:05 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> > > > On Sun, Jul 26, 2020 at 4:17 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > >>
> > > >> On Sat, Jul 25, 2020 at 02:20:03PM -0300, Daniel Gutson wrote:
> > > >> > El sáb., 25 jul. 2020 2:56 a. m., Greg Kroah-Hartman <
> > > >> > gregkh@linuxfoundation.org> escribió:
> > > >> >
> > > >> >
> > > >> > 1) I just did the same that intel-spi.c does.
> > > >>
> > > >> No need to copy bad examples :)
> > > >
> > > >
> > > > Didn't know it was a bad example. What's is the current modern mechanism that replaces initialization-time configuration?
> > >
> > > I'd say you'd generally want this to be a per-instance setting, which
> > > could be a sysfs attribute of the physical device, or an ioctl for an
> > > existing user space abstraction.
> >
> > But still, they are not equivalent. The initial configuration remains
> > constant for the rest of the life of the driver, whereas the
> > sysfs attribute is set after the initialization and can be re-set over
> > time. I'm not asking module parameters "to come back" if
> > they are now considered obsolete, I'm just trying to understand.
> >
> > >
> > > In the changelog, you should also explain what this is used for. Do
> > > you actually want to write to a device that is marked read-only, or
> > > are you just trying to make the interface more consistent between the
> > > two drivers?
> >
> > The device can either be locked or unlocked. If it is unlocked, it can
> > be set writable depending on the module
> > argument in intel-spi, or straight writable in intel-spi-pci. Which is
> > dangerous.
> > I wanted to make, as you say, the interface consistent.
> > Exposing an interface to the user (via a sysfs attribute) to try to
> > make the driver writable is definitively a bad idea.
> > I'd rather do nothing (no module arg) rather than open that
> > here-be-dragons door.
> 
> ping.
> This is a real existing problem that I'm trying to address.
> There's currently no way to prevent the kernel to try to turn
> the SPI flash chip writable for the device IDs handled by
> intel-spi-pci. And allowing userspace to switch it between
> writable/non-writable is not an option.
> What other mechanism can I use other than the module parameter,

Again, module parameters are working on a per-chunk-of-code basis, while
you want to work on a per-device basis, which is why you should not use
a module parameter.

good luck!

greg k-h

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Arnd Bergmann]

On Thu, Jul 30, 2020 at 2:21 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> El jue., 30 jul. 2020 2:31 a. m., Greg Kroah-Hartman <gregkh@linuxfoundation.org> escribió:
>>
>> Again, module parameters are working on a per-chunk-of-code basis, while
>> you want to work on a per-device basis,
>
>
> I think there is a misunderstanding.  What I want is to control (turn on or off) is a very specific code snippet that provides the "functionality" of trying to turn the chip writable. The rest of the device driver is fine.
> I assume that the one that doesn't understand is me.
>

I looked at the source code again and found that the existing module
parameter applies to both the platform and pci device front-ends, both
of which go through

        /* Prevent writes if not explicitly enabled */
        if (!ispi->writeable || !writeable)
                ispi->nor.mtd.flags &= ~MTD_WRITEABLE;

Setting the PCI device writable in hardware makes it possible to
actually write to it *only* if the module parameter is also set to '1'.
One might disagree with that design, but I don't think your patch
would make it any better, it just means one would have to set
two module parameters instead of one.

     Arnd

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Daniel Gutson]

On Thu, Jul 30, 2020 at 11:09 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Jul 30, 2020 at 2:21 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> > El jue., 30 jul. 2020 2:31 a. m., Greg Kroah-Hartman <gregkh@linuxfoundation.org> escribió:
> >>
> >> Again, module parameters are working on a per-chunk-of-code basis, while
> >> you want to work on a per-device basis,
> >
> >
> > I think there is a misunderstanding.  What I want is to control (turn on or off) is a very specific code snippet that provides the "functionality" of trying to turn the chip writable. The rest of the device driver is fine.
> > I assume that the one that doesn't understand is me.
> >
>
> I looked at the source code again and found that the existing module
> parameter applies to both the platform and pci device front-ends, both
> of which go through
>
>         /* Prevent writes if not explicitly enabled */
>         if (!ispi->writeable || !writeable)
>                 ispi->nor.mtd.flags &= ~MTD_WRITEABLE;
>

I think you missed
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/controllers/intel-spi-pci.c#L44

    /* 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);
    }

in the probe function, and is executed always and unconditionally.

/* 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);
}

> Setting the PCI device writable in hardware makes it possible to
> actually write to it *only* if the module parameter is also set to '1'.
> One might disagree with that design, but I don't think your patch
> would make it any better, it just means one would have to set
> two module parameters instead of one.
>
>      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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Daniel Gutson]

On Thu, Jul 30, 2020 at 11:18 AM Daniel Gutson <daniel@eclypsium.com> wrote:
>
> On Thu, Jul 30, 2020 at 11:09 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Thu, Jul 30, 2020 at 2:21 PM Daniel Gutson <daniel@eclypsium.com> wrote:
> > > El jue., 30 jul. 2020 2:31 a. m., Greg Kroah-Hartman <gregkh@linuxfoundation.org> escribió:
> > >>
> > >> Again, module parameters are working on a per-chunk-of-code basis, while
> > >> you want to work on a per-device basis,
> > >
> > >
> > > I think there is a misunderstanding.  What I want is to control (turn on or off) is a very specific code snippet that provides the "functionality" of trying to turn the chip writable. The rest of the device driver is fine.
> > > I assume that the one that doesn't understand is me.
> > >
> >
> > I looked at the source code again and found that the existing module
> > parameter applies to both the platform and pci device front-ends, both
> > of which go through
> >
> >         /* Prevent writes if not explicitly enabled */
> >         if (!ispi->writeable || !writeable)
> >                 ispi->nor.mtd.flags &= ~MTD_WRITEABLE;
> >
>
> I think you missed
> https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/controllers/intel-spi-pci.c#L44
>
>     /* 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);
>     }
>
> in the probe function, and is executed always and unconditionally.

To clarify, this is executed before intel-spi code.

>
> /* 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);
> }
>
> > Setting the PCI device writable in hardware makes it possible to
> > actually write to it *only* if the module parameter is also set to '1'.
> > One might disagree with that design, but I don't think your patch
> > would make it any better, it just means one would have to set
> > two module parameters instead of one.
> >
> >      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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Mika Westerberg]

Hi,

Sorry for the delay, I was on vacation.

On Fri, Jul 24, 2020 at 06:28:53PM -0300, Daniel Gutson wrote:
> Currently, 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.
> 
> That's why I propose this patch to add such argument to intel-spi-pci,
> so the user can control whether the driver tries to make the chip
> writeable or not, being the default FALSE as is the argument of
> intel-spi.
> 
> Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> ---
>  drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 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..77e57450f166 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> @@ -24,6 +24,10 @@ static const struct intel_spi_boardinfo cnl_info = {
>  	.type = INTEL_SPI_CNL,
>  };
>  
> +static bool writeable;
> +module_param(writeable, bool, 0);
> +MODULE_PARM_DESC(writeable, "Enable write access to SPI flash chip (default=0)");

I think instead of this we should simply make it so that the driver
never tries to make the chip writable.

> +
>  static int intel_spi_pci_probe(struct pci_dev *pdev,
>  			       const struct pci_device_id *id)
>  {
> @@ -41,12 +45,14 @@ 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);
> +	if (writeable) {
> +		/* 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);

So here we just read the BCR register and then set info->writeable based
on its value.

Then it is up to the BIOS to enable this if it allows writing the flash
chip from the OS side.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Richard Hughes]

On Mon, 3 Aug 2020 at 10:57, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> I think instead of this we should simply make it so that the driver
> never tries to make the chip writable.

I think this is a good idea, but I wasn't sure if it was an acceptable
behaviour change. Should the driver still try to set BCR_WPD when
writing an image (i.e. defer the setting of write enable until later),
or just not set the BCR register at all? I think your last comment was
the latter, but wanted to check.

Richard.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Mika Westerberg]

On Mon, Aug 03, 2020 at 11:18:12AM +0100, Richard Hughes wrote:
> On Mon, 3 Aug 2020 at 10:57, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > I think instead of this we should simply make it so that the driver
> > never tries to make the chip writable.
> 
> I think this is a good idea, but I wasn't sure if it was an acceptable
> behaviour change. Should the driver still try to set BCR_WPD when
> writing an image (i.e. defer the setting of write enable until later),
> or just not set the BCR register at all? I think your last comment was
> the latter, but wanted to check.

I would say not set it at all. I think it was (my) mistake to set it in
the first place.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Daniel Gutson]

On Mon, Aug 3, 2020 at 7:27 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Mon, Aug 03, 2020 at 11:18:12AM +0100, Richard Hughes wrote:
> > On Mon, 3 Aug 2020 at 10:57, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > I think instead of this we should simply make it so that the driver
> > > never tries to make the chip writable.
> >
> > I think this is a good idea, but I wasn't sure if it was an acceptable
> > behaviour change. Should the driver still try to set BCR_WPD when
> > writing an image (i.e. defer the setting of write enable until later),
> > or just not set the BCR register at all? I think your last comment was
> > the latter, but wanted to check.
>
> I would say not set it at all. I think it was (my) mistake to set it in
> the first place.

Do you want me to remove the module parameter from intel-spi too and
do the same?



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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Mika Westerberg]

On Mon, Aug 03, 2020 at 09:58:23AM -0300, Daniel Gutson wrote:
> On Mon, Aug 3, 2020 at 7:27 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > On Mon, Aug 03, 2020 at 11:18:12AM +0100, Richard Hughes wrote:
> > > On Mon, 3 Aug 2020 at 10:57, Mika Westerberg
> > > <mika.westerberg@linux.intel.com> wrote:
> > > > I think instead of this we should simply make it so that the driver
> > > > never tries to make the chip writable.
> > >
> > > I think this is a good idea, but I wasn't sure if it was an acceptable
> > > behaviour change. Should the driver still try to set BCR_WPD when
> > > writing an image (i.e. defer the setting of write enable until later),
> > > or just not set the BCR register at all? I think your last comment was
> > > the latter, but wanted to check.
> >
> > I would say not set it at all. I think it was (my) mistake to set it in
> > the first place.
> 
> Do you want me to remove the module parameter from intel-spi too and
> do the same?

No, I think that should still be left there. Then by default it is
read-only and you can only enable writing if the BIOS allows it and that
the user actually requested it.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable

[Daniel Gutson]

On Mon, Aug 3, 2020 at 10:06 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Mon, Aug 03, 2020 at 09:58:23AM -0300, Daniel Gutson wrote:
> > On Mon, Aug 3, 2020 at 7:27 AM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > On Mon, Aug 03, 2020 at 11:18:12AM +0100, Richard Hughes wrote:
> > > > On Mon, 3 Aug 2020 at 10:57, Mika Westerberg
> > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > I think instead of this we should simply make it so that the driver
> > > > > never tries to make the chip writable.
> > > >
> > > > I think this is a good idea, but I wasn't sure if it was an acceptable
> > > > behaviour change. Should the driver still try to set BCR_WPD when
> > > > writing an image (i.e. defer the setting of write enable until later),
> > > > or just not set the BCR register at all? I think your last comment was
> > > > the latter, but wanted to check.
> > >
> > > I would say not set it at all. I think it was (my) mistake to set it in
> > > the first place.
> >
> > Do you want me to remove the module parameter from intel-spi too and
> > do the same?
>
> No, I think that should still be left there. Then by default it is
> read-only and you can only enable writing if the BIOS allows it and that
> the user actually requested it.

OK. Patch heading your way in 1h.



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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/