* [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable @ 2020-08-04 13:58 Daniel Gutson 2020-08-04 14:08 ` Mika Westerberg 2020-08-04 15:21 ` Arnd Bergmann 0 siblings, 2 replies; 25+ messages in thread From: Daniel Gutson @ 2020-08-04 13:58 UTC (permalink / raw) To: Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Mika Westerberg, Boris Brezillon, Daniel Gutson, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes Cc: Arnd Bergmann, Greg Kroah-Hartman 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-04 13:58 [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable Daniel Gutson @ 2020-08-04 14:08 ` Mika Westerberg 2020-08-04 15:21 ` Arnd Bergmann 1 sibling, 0 replies; 25+ messages in thread From: Mika Westerberg @ 2020-08-04 14:08 UTC (permalink / raw) To: Daniel Gutson Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Arnd Bergmann, Greg Kroah-Hartman 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-04 13:58 [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable Daniel Gutson 2020-08-04 14:08 ` Mika Westerberg @ 2020-08-04 15:21 ` Arnd Bergmann [not found] ` <CAFmMkTHEm8k+5GZkVJbDZMEhMwpsqVKRb-hGskSpBstdLRuFyA@mail.gmail.com> 1 sibling, 1 reply; 25+ messages in thread From: Arnd Bergmann @ 2020-08-04 15:21 UTC (permalink / raw) To: Daniel Gutson Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Mika Westerberg, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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: ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <CAFmMkTHEm8k+5GZkVJbDZMEhMwpsqVKRb-hGskSpBstdLRuFyA@mail.gmail.com>]
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable [not found] ` <CAFmMkTHEm8k+5GZkVJbDZMEhMwpsqVKRb-hGskSpBstdLRuFyA@mail.gmail.com> @ 2020-08-04 19:06 ` Arnd Bergmann 2020-08-04 19:57 ` Daniel Gutson 0 siblings, 1 reply; 25+ messages in thread From: Arnd Bergmann @ 2020-08-04 19:06 UTC (permalink / raw) To: Daniel Gutson Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Mika Westerberg, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-04 19:06 ` Arnd Bergmann @ 2020-08-04 19:57 ` Daniel Gutson 2020-08-04 20:46 ` Arnd Bergmann 0 siblings, 1 reply; 25+ messages in thread From: Daniel Gutson @ 2020-08-04 19:57 UTC (permalink / raw) To: Arnd Bergmann Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Mika Westerberg, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-04 19:57 ` Daniel Gutson @ 2020-08-04 20:46 ` Arnd Bergmann 2020-08-04 21:26 ` Daniel Gutson 0 siblings, 1 reply; 25+ messages in thread From: Arnd Bergmann @ 2020-08-04 20:46 UTC (permalink / raw) To: Daniel Gutson Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Mika Westerberg, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-04 20:46 ` Arnd Bergmann @ 2020-08-04 21:26 ` Daniel Gutson 2020-08-12 15:41 ` Daniel Gutson 2020-08-13 15:41 ` Arnd Bergmann 0 siblings, 2 replies; 25+ messages in thread From: Daniel Gutson @ 2020-08-04 21:26 UTC (permalink / raw) To: Arnd Bergmann Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Mika Westerberg, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-04 21:26 ` Daniel Gutson @ 2020-08-12 15:41 ` Daniel Gutson 2020-08-13 9:01 ` Greg Kroah-Hartman 2020-08-13 15:41 ` Arnd Bergmann 1 sibling, 1 reply; 25+ messages in thread From: Daniel Gutson @ 2020-08-12 15:41 UTC (permalink / raw) To: Arnd Bergmann Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Mika Westerberg, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-12 15:41 ` Daniel Gutson @ 2020-08-13 9:01 ` Greg Kroah-Hartman [not found] ` <CAFmMkTFgjW+9gNfx=2SU7B0foww=SLiiyVi+P-hZpEFDbMTf2Q@mail.gmail.com> 0 siblings, 1 reply; 25+ messages in thread From: Greg Kroah-Hartman @ 2020-08-13 9:01 UTC (permalink / raw) To: Daniel Gutson Cc: Arnd Bergmann, Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Mika Westerberg, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes On Wed, Aug 12, 2020 at 12:41:35PM -0300, Daniel Gutson wrote: > ping What does that mean here? ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <CAFmMkTFgjW+9gNfx=2SU7B0foww=SLiiyVi+P-hZpEFDbMTf2Q@mail.gmail.com>]
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable [not found] ` <CAFmMkTFgjW+9gNfx=2SU7B0foww=SLiiyVi+P-hZpEFDbMTf2Q@mail.gmail.com> @ 2020-08-13 11:47 ` Greg Kroah-Hartman 0 siblings, 0 replies; 25+ messages in thread From: Greg Kroah-Hartman @ 2020-08-13 11:47 UTC (permalink / raw) To: Daniel Gutson Cc: Arnd Bergmann, Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Mika Westerberg, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-04 21:26 ` Daniel Gutson 2020-08-12 15:41 ` Daniel Gutson @ 2020-08-13 15:41 ` Arnd Bergmann 2020-08-13 21:40 ` Daniel Gutson 1 sibling, 1 reply; 25+ messages in thread From: Arnd Bergmann @ 2020-08-13 15:41 UTC (permalink / raw) To: Daniel Gutson Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Mika Westerberg, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-13 15:41 ` Arnd Bergmann @ 2020-08-13 21:40 ` Daniel Gutson 2020-08-16 8:41 ` Arnd Bergmann 0 siblings, 1 reply; 25+ messages in thread From: Daniel Gutson @ 2020-08-13 21:40 UTC (permalink / raw) To: Arnd Bergmann Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Mika Westerberg, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-13 21:40 ` Daniel Gutson @ 2020-08-16 8:41 ` Arnd Bergmann 2020-08-18 15:55 ` Daniel Gutson 0 siblings, 1 reply; 25+ messages in thread From: Arnd Bergmann @ 2020-08-16 8:41 UTC (permalink / raw) To: Daniel Gutson Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Mika Westerberg, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-16 8:41 ` Arnd Bergmann @ 2020-08-18 15:55 ` Daniel Gutson 2020-08-19 6:57 ` Mika Westerberg 0 siblings, 1 reply; 25+ messages in thread From: Daniel Gutson @ 2020-08-18 15:55 UTC (permalink / raw) To: Arnd Bergmann Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Mika Westerberg, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-18 15:55 ` Daniel Gutson @ 2020-08-19 6:57 ` Mika Westerberg 2020-08-19 8:38 ` Arnd Bergmann 0 siblings, 1 reply; 25+ messages in thread From: Mika Westerberg @ 2020-08-19 6:57 UTC (permalink / raw) To: Daniel Gutson Cc: Arnd Bergmann, Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-19 6:57 ` Mika Westerberg @ 2020-08-19 8:38 ` Arnd Bergmann 2020-08-19 9:11 ` Mika Westerberg 2020-08-19 9:19 ` David Laight 0 siblings, 2 replies; 25+ messages in thread From: Arnd Bergmann @ 2020-08-19 8:38 UTC (permalink / raw) To: Mika Westerberg Cc: Daniel Gutson, Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-19 8:38 ` Arnd Bergmann @ 2020-08-19 9:11 ` Mika Westerberg 2020-08-22 16:06 ` Arnd Bergmann 2020-08-19 9:19 ` David Laight 1 sibling, 1 reply; 25+ messages in thread From: Mika Westerberg @ 2020-08-19 9:11 UTC (permalink / raw) To: Arnd Bergmann Cc: Daniel Gutson, Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-19 9:11 ` Mika Westerberg @ 2020-08-22 16:06 ` Arnd Bergmann 2020-08-24 8:22 ` Mika Westerberg 0 siblings, 1 reply; 25+ messages in thread From: Arnd Bergmann @ 2020-08-22 16:06 UTC (permalink / raw) To: Mika Westerberg Cc: Daniel Gutson, Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-22 16:06 ` Arnd Bergmann @ 2020-08-24 8:22 ` Mika Westerberg 2020-08-24 9:08 ` Arnd Bergmann 0 siblings, 1 reply; 25+ messages in thread From: Mika Westerberg @ 2020-08-24 8:22 UTC (permalink / raw) To: Arnd Bergmann Cc: Daniel Gutson, Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-24 8:22 ` Mika Westerberg @ 2020-08-24 9:08 ` Arnd Bergmann 2020-08-24 9:15 ` Mika Westerberg 0 siblings, 1 reply; 25+ messages in thread From: Arnd Bergmann @ 2020-08-24 9:08 UTC (permalink / raw) To: Mika Westerberg Cc: Daniel Gutson, Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-24 9:08 ` Arnd Bergmann @ 2020-08-24 9:15 ` Mika Westerberg 2020-08-24 9:31 ` Arnd Bergmann 0 siblings, 1 reply; 25+ messages in thread From: Mika Westerberg @ 2020-08-24 9:15 UTC (permalink / raw) To: Arnd Bergmann Cc: Daniel Gutson, Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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). ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-24 9:15 ` Mika Westerberg @ 2020-08-24 9:31 ` Arnd Bergmann 2020-08-24 9:44 ` Mika Westerberg 0 siblings, 1 reply; 25+ messages in thread From: Arnd Bergmann @ 2020-08-24 9:31 UTC (permalink / raw) To: Mika Westerberg Cc: Daniel Gutson, Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-24 9:31 ` Arnd Bergmann @ 2020-08-24 9:44 ` Mika Westerberg 2020-08-24 16:00 ` Daniel Gutson 0 siblings, 1 reply; 25+ messages in thread From: Mika Westerberg @ 2020-08-24 9:44 UTC (permalink / raw) To: Arnd Bergmann Cc: Daniel Gutson, Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ;-) ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-24 9:44 ` Mika Westerberg @ 2020-08-24 16:00 ` Daniel Gutson 0 siblings, 0 replies; 25+ messages in thread From: Daniel Gutson @ 2020-08-24 16:00 UTC (permalink / raw) To: Mika Westerberg Cc: Arnd Bergmann, Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable 2020-08-19 8:38 ` Arnd Bergmann 2020-08-19 9:11 ` Mika Westerberg @ 2020-08-19 9:19 ` David Laight 1 sibling, 0 replies; 25+ messages in thread From: David Laight @ 2020-08-19 9:19 UTC (permalink / raw) To: 'Arnd Bergmann', Mika Westerberg Cc: Daniel Gutson, Tudor Ambarus, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel, Alex Bazhaniuk, Richard Hughes, Greg Kroah-Hartman 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) ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2020-08-24 16:01 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-04 13:58 [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable Daniel Gutson 2020-08-04 14:08 ` Mika Westerberg 2020-08-04 15:21 ` Arnd Bergmann [not found] ` <CAFmMkTHEm8k+5GZkVJbDZMEhMwpsqVKRb-hGskSpBstdLRuFyA@mail.gmail.com> 2020-08-04 19:06 ` Arnd Bergmann 2020-08-04 19:57 ` Daniel Gutson 2020-08-04 20:46 ` Arnd Bergmann 2020-08-04 21:26 ` Daniel Gutson 2020-08-12 15:41 ` Daniel Gutson 2020-08-13 9:01 ` Greg Kroah-Hartman [not found] ` <CAFmMkTFgjW+9gNfx=2SU7B0foww=SLiiyVi+P-hZpEFDbMTf2Q@mail.gmail.com> 2020-08-13 11:47 ` Greg Kroah-Hartman 2020-08-13 15:41 ` Arnd Bergmann 2020-08-13 21:40 ` Daniel Gutson 2020-08-16 8:41 ` Arnd Bergmann 2020-08-18 15:55 ` Daniel Gutson 2020-08-19 6:57 ` Mika Westerberg 2020-08-19 8:38 ` Arnd Bergmann 2020-08-19 9:11 ` Mika Westerberg 2020-08-22 16:06 ` Arnd Bergmann 2020-08-24 8:22 ` Mika Westerberg 2020-08-24 9:08 ` Arnd Bergmann 2020-08-24 9:15 ` Mika Westerberg 2020-08-24 9:31 ` Arnd Bergmann 2020-08-24 9:44 ` Mika Westerberg 2020-08-24 16:00 ` Daniel Gutson 2020-08-19 9:19 ` David Laight
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).