[PATCH] Remove attempt by intel-spi-pci to turn the SPI flash chip writeable

[PATCH] Remove attempt by intel-spi-pci to turn the SPI flash chip writeable

[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 | 6 ------
 1 file changed, 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..d721ba4e8b41 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
+++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
@@ -41,13 +41,7 @@ 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);
 
 	ispi = intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
-- 
2.25.1

Re: [PATCH] Remove attempt by intel-spi-pci to turn the SPI flash chip writeable

[Arnd Bergmann]

On Mon, Aug 3, 2020 at 3:45 PM Daniel Gutson
<daniel.gutson@eclypsium.com> wrote:

> 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 part of the description sounds wrong: the current behavior is that
the BIOS setting is ignored for PCI devices and it only uses the module
parameter, the same way as it does for the platform driver.

With your patch, both the BIOS setting and the module parameter
have to explicitly allow writing on PCI devices, while at least for Bay
Trail platform devices the BIOS write protection is still ignored.

It sounds like this is what you want, but you should update the description
accordingly.

      Arnd

Re: [PATCH] Remove attempt by intel-spi-pci to turn the SPI flash chip writeable

[Daniel Gutson]

On Mon, Aug 3, 2020 at 10:55 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Aug 3, 2020 at 3:45 PM Daniel Gutson
> <daniel.gutson@eclypsium.com> wrote:
>
> > 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 part of the description sounds wrong: the current behavior is that
> the BIOS setting is ignored for PCI devices and it only uses the module
> parameter, the same way as it does for the platform driver.

Actually, the BIOS setting is not ignored, since it is not bypassable.
There is a lock in the BIOS setting, that, if enabled no matter what the
driver does, it will be still read only. However, if that lock is not set,
the SPI chip will be writable because of the driver. That's why
I say 'attempts'.
The intel-spi-pci driver doesn't have a module parameter, and that's
why it unconditionally attempts to turn the chip writable (it will succeed
if it is not locked).
What I did was just left the intel-spi-pci driver without any module parameter,
as it currently is, but removed the part where it attempts to turn the chip
writable (just in case the BIOS is not locked).

>
> With your patch, both the BIOS setting and the module parameter
> have to explicitly allow writing on PCI devices, while at least for Bay
> Trail platform devices the BIOS write protection is still ignored.
>
> It sounds like this is what you want, but you should update the description
> accordingly.
>
>       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] Remove attempt by intel-spi-pci to turn the SPI flash chip writeable

[Mika Westerberg]

Hi,

On Mon, Aug 03, 2020 at 10:44:49AM -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.

I think you should make the $subject to follow the convention used in
the SPI-NOR subsystem. You can see it running following command:

  $ git log --oneline drivers/mtd/spi-nor/controllers/intel-spi-pci.c

In this case I think it should be:

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

or something like that.

The patch itself looks good, one minor comment below.

> 
> Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> ---
>  drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 6 ------
>  1 file changed, 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..d721ba4e8b41 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> @@ -41,13 +41,7 @@ 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);

Perhaps we should log this in debug level (dev_dbg()) so when debugging
possible issues we can see that the chip is write protected by the BIOS.

>  
>  	ispi = intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
> -- 
> 2.25.1