* [PATCH] PCI: fix invalid ROM content detection in pci_get_rom_size() @ 2015-11-06 12:16 Vladis Dronov 2015-11-24 17:03 ` Bjorn Helgaas 0 siblings, 1 reply; 5+ messages in thread From: Vladis Dronov @ 2015-11-06 12:16 UTC (permalink / raw) To: linux-pci; +Cc: Vladis Dronov Make pci_get_rom_size() to check all bytes in the PCI ROM signature and issue a warning if the values are not following the standard. Signed-off-by: Vladis Dronov <vdronov@redhat.com> --- drivers/pci/rom.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c index eb0ad53..34f8b2c 100644 --- a/drivers/pci/rom.c +++ b/drivers/pci/rom.c @@ -77,22 +77,17 @@ size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size) do { void __iomem *pds; /* Standard PCI ROMs start out with these bytes 55 AA */ - if (readb(image) != 0x55) { - dev_err(&pdev->dev, "Invalid ROM contents\n"); + if ((readb(image) != 0x55) || (readb(image + 1) != 0xAA)) { + dev_err(&pdev->dev, "Invalid PCI ROM signature\n"); break; } - if (readb(image + 1) != 0xAA) - break; /* get the PCI data structure and check its signature */ pds = image + readw(image + 24); - if (readb(pds) != 'P') - break; - if (readb(pds + 1) != 'C') - break; - if (readb(pds + 2) != 'I') - break; - if (readb(pds + 3) != 'R') + if ((readb(pds) != 'P') || (readb(pds + 1) != 'C') || + (readb(pds + 2) != 'I') || (readb(pds + 3) != 'R')) { + dev_err(&pdev->dev, "Invalid PCI ROM data signature\n"); break; + } last_image = readb(pds + 21) & 0x80; length = readw(pds + 16); image += length * 512; -- 2.6.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI: fix invalid ROM content detection in pci_get_rom_size() 2015-11-06 12:16 [PATCH] PCI: fix invalid ROM content detection in pci_get_rom_size() Vladis Dronov @ 2015-11-24 17:03 ` Bjorn Helgaas 2015-11-27 17:05 ` Vladis Dronov 2015-11-27 17:20 ` [PATCH v2] PCI: fix missing ROM content warning " Vladis Dronov 0 siblings, 2 replies; 5+ messages in thread From: Bjorn Helgaas @ 2015-11-24 17:03 UTC (permalink / raw) To: Vladis Dronov; +Cc: linux-pci Hi Vladis, On Fri, Nov 06, 2015 at 01:16:36PM +0100, Vladis Dronov wrote: > Make pci_get_rom_size() to check all bytes in the PCI ROM signature > and issue a warning if the values are not following the standard. Can you include a reference to the spec, please? Does this fix a bug? If so, please include a reference to that as well. >From reading the patch, I don't think it would change the return value; it looks like the only change is that we'll emit a warning in some cases where we previously didn't. That does seem worthwhile, but it doesn't quite match your changelog, which implies that we'll check more bytes. While you're at it, maybe you could include the unexpected values in the error messages. Thanks, Bjorn > Signed-off-by: Vladis Dronov <vdronov@redhat.com> > --- > drivers/pci/rom.c | 17 ++++++----------- > 1 file changed, 6 insertions(+), 11 deletions(-) > > diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c > index eb0ad53..34f8b2c 100644 > --- a/drivers/pci/rom.c > +++ b/drivers/pci/rom.c > @@ -77,22 +77,17 @@ size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size) > do { > void __iomem *pds; > /* Standard PCI ROMs start out with these bytes 55 AA */ > - if (readb(image) != 0x55) { > - dev_err(&pdev->dev, "Invalid ROM contents\n"); > + if ((readb(image) != 0x55) || (readb(image + 1) != 0xAA)) { > + dev_err(&pdev->dev, "Invalid PCI ROM signature\n"); > break; > } > - if (readb(image + 1) != 0xAA) > - break; > /* get the PCI data structure and check its signature */ > pds = image + readw(image + 24); > - if (readb(pds) != 'P') > - break; > - if (readb(pds + 1) != 'C') > - break; > - if (readb(pds + 2) != 'I') > - break; > - if (readb(pds + 3) != 'R') > + if ((readb(pds) != 'P') || (readb(pds + 1) != 'C') || > + (readb(pds + 2) != 'I') || (readb(pds + 3) != 'R')) { > + dev_err(&pdev->dev, "Invalid PCI ROM data signature\n"); > break; > + } > last_image = readb(pds + 21) & 0x80; > length = readw(pds + 16); > image += length * 512; > -- > 2.6.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI: fix invalid ROM content detection in pci_get_rom_size() 2015-11-24 17:03 ` Bjorn Helgaas @ 2015-11-27 17:05 ` Vladis Dronov 2015-11-27 17:20 ` [PATCH v2] PCI: fix missing ROM content warning " Vladis Dronov 1 sibling, 0 replies; 5+ messages in thread From: Vladis Dronov @ 2015-11-27 17:05 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-pci Hello, Bjorn. > > Make pci_get_rom_size() to check all bytes in the PCI ROM signature > > and issue a warning if the values are not following the standard. > > Can you include a reference to the spec, please? I was not able to find publicly available "PCI Firmware Specification Revision 3.x" doc (only my company's internal one for 3.0), but the related part is matching one found in "PCI Local Bus Specification v2.2", at http://www.ics.uci.edu/~harris/ics216/pci/PCI_22.pdf ,page 207, "6.3.1.1. PCI Expansion ROM Header Format", "6.3.1.2. PCI Data Structure Format": Offset Length Value Description 0h 1 55h ROM Signature, byte 1 1h 1 AAh ROM Signature, byte 2 Offset Length Description 0 4 Signature, the string "PCIR" > Does this fix a bug? If so, please include a reference to that as well. > > From reading the patch, I don't think it would change the return value; it > looks like the only change is that we'll emit a warning in some cases where > we previously didn't. That does seem worthwhile, but it doesn't quite > match your changelog, which implies that we'll check more bytes. You're right, there is no bug, my wording is incorrect. The code indeed checks all signature bytes, it does not just print a warning in some cases. And my suggested code indeed does not change the return value, it only emits a warning in cases where it previously didn't. I believe, this change deserves a patch, and I will alter a changelog wording. > While you're at it, maybe you could include the unexpected values in the > error messages. Yes, I will reply with a patch-v2-which-prints-the-values to this message shortly. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer ----- Original Message ----- From: "Bjorn Helgaas" <helgaas@kernel.org> To: "Vladis Dronov" <vdronov@redhat.com> Cc: linux-pci@vger.kernel.org Sent: Tuesday, November 24, 2015 6:03:31 PM Subject: Re: [PATCH] PCI: fix invalid ROM content detection in pci_get_rom_size() Hi Vladis, On Fri, Nov 06, 2015 at 01:16:36PM +0100, Vladis Dronov wrote: > Make pci_get_rom_size() to check all bytes in the PCI ROM signature > and issue a warning if the values are not following the standard. Can you include a reference to the spec, please? Does this fix a bug? If so, please include a reference to that as well. >From reading the patch, I don't think it would change the return value; it looks like the only change is that we'll emit a warning in some cases where we previously didn't. That does seem worthwhile, but it doesn't quite match your changelog, which implies that we'll check more bytes. While you're at it, maybe you could include the unexpected values in the error messages. Thanks, Bjorn > Signed-off-by: Vladis Dronov <vdronov@redhat.com> > --- > drivers/pci/rom.c | 17 ++++++----------- > 1 file changed, 6 insertions(+), 11 deletions(-) > > diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c > index eb0ad53..34f8b2c 100644 > --- a/drivers/pci/rom.c > +++ b/drivers/pci/rom.c > @@ -77,22 +77,17 @@ size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size) > do { > void __iomem *pds; > /* Standard PCI ROMs start out with these bytes 55 AA */ > - if (readb(image) != 0x55) { > - dev_err(&pdev->dev, "Invalid ROM contents\n"); > + if ((readb(image) != 0x55) || (readb(image + 1) != 0xAA)) { > + dev_err(&pdev->dev, "Invalid PCI ROM signature\n"); > break; > } > - if (readb(image + 1) != 0xAA) > - break; > /* get the PCI data structure and check its signature */ > pds = image + readw(image + 24); > - if (readb(pds) != 'P') > - break; > - if (readb(pds + 1) != 'C') > - break; > - if (readb(pds + 2) != 'I') > - break; > - if (readb(pds + 3) != 'R') > + if ((readb(pds) != 'P') || (readb(pds + 1) != 'C') || > + (readb(pds + 2) != 'I') || (readb(pds + 3) != 'R')) { > + dev_err(&pdev->dev, "Invalid PCI ROM data signature\n"); > break; > + } > last_image = readb(pds + 21) & 0x80; > length = readw(pds + 16); > image += length * 512; > -- > 2.6.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] PCI: fix missing ROM content warning in pci_get_rom_size() 2015-11-24 17:03 ` Bjorn Helgaas 2015-11-27 17:05 ` Vladis Dronov @ 2015-11-27 17:20 ` Vladis Dronov 2015-12-04 23:44 ` Bjorn Helgaas 1 sibling, 1 reply; 5+ messages in thread From: Vladis Dronov @ 2015-11-27 17:20 UTC (permalink / raw) To: Bjorn Helgaas, linux-pci; +Cc: Vladis Dronov Make pci_get_rom_size() to emit a warning if any byte in a PCI ROM header or data signature is not following the standard ("PCI Local Bus Specification" or "PCI Firmware Specification Revision 3.x"), not only the first one. Signed-off-by: Vladis Dronov <vdronov@redhat.com> --- drivers/pci/rom.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c index eb0ad53..48404db 100644 --- a/drivers/pci/rom.c +++ b/drivers/pci/rom.c @@ -77,22 +77,18 @@ size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size) do { void __iomem *pds; /* Standard PCI ROMs start out with these bytes 55 AA */ - if (readb(image) != 0x55) { - dev_err(&pdev->dev, "Invalid ROM contents\n"); + if (readw(image) != 0xAA55) { + dev_err(&pdev->dev, "Invalid PCI ROM header signature:" + " 0x%4.4x\n", readw(image)); break; } - if (readb(image + 1) != 0xAA) - break; - /* get the PCI data structure and check its signature */ + /* get the PCI data structure and check its "PCIR" signature */ pds = image + readw(image + 24); - if (readb(pds) != 'P') - break; - if (readb(pds + 1) != 'C') - break; - if (readb(pds + 2) != 'I') - break; - if (readb(pds + 3) != 'R') + if (readl(pds) != 0x52494350) { + dev_err(&pdev->dev, "Invalid PCI ROM data signature:" + " 0x%8.8x\n", readl(pds)); break; + } last_image = readb(pds + 21) & 0x80; length = readw(pds + 16); image += length * 512; -- 2.4.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] PCI: fix missing ROM content warning in pci_get_rom_size() 2015-11-27 17:20 ` [PATCH v2] PCI: fix missing ROM content warning " Vladis Dronov @ 2015-12-04 23:44 ` Bjorn Helgaas 0 siblings, 0 replies; 5+ messages in thread From: Bjorn Helgaas @ 2015-12-04 23:44 UTC (permalink / raw) To: Vladis Dronov; +Cc: linux-pci On Fri, Nov 27, 2015 at 06:20:06PM +0100, Vladis Dronov wrote: > Make pci_get_rom_size() to emit a warning if any byte in a PCI ROM > header or data signature is not following the standard ("PCI Local > Bus Specification" or "PCI Firmware Specification Revision 3.x"), > not only the first one. > > Signed-off-by: Vladis Dronov <vdronov@redhat.com> Applied as follows to pci/misc for v4.5, thanks! commit f3744cad60c3ecae125c4be566867517d4bac848 Author: Vladis Dronov <vdronov@redhat.com> Date: Fri Nov 27 18:20:06 2015 +0100 PCI: Print warnings for all invalid expansion ROM headers We've always validated that both bytes of the Expansion ROM signature and all four bytes of the PCI Data Structure signature (see PCI Firmware spec r3.0, sec 5.1.1), but we only printed a warning if the first byte of the ROM signature was invalid. Print warnings if *any* of those bytes are invalid. Note that we only look at these headers if we map or read the ROM. [bhelgaas: changelog, tweak printk format] Signed-off-by: Vladis Dronov <vdronov@redhat.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c index eb0ad53..5a1a39d 100644 --- a/drivers/pci/rom.c +++ b/drivers/pci/rom.c @@ -77,22 +77,18 @@ size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size) do { void __iomem *pds; /* Standard PCI ROMs start out with these bytes 55 AA */ - if (readb(image) != 0x55) { - dev_err(&pdev->dev, "Invalid ROM contents\n"); + if (readw(image) != 0xAA55) { + dev_err(&pdev->dev, "Invalid PCI ROM header signature: expecting 0xaa55, got %#06x\n", + readw(image)); break; } - if (readb(image + 1) != 0xAA) - break; - /* get the PCI data structure and check its signature */ + /* get the PCI data structure and check its "PCIR" signature */ pds = image + readw(image + 24); - if (readb(pds) != 'P') - break; - if (readb(pds + 1) != 'C') - break; - if (readb(pds + 2) != 'I') - break; - if (readb(pds + 3) != 'R') + if (readl(pds) != 0x52494350) { + dev_err(&pdev->dev, "Invalid PCI ROM data signature: expecting 0x52494350, got %#010x\n", + readl(pds)); break; + } last_image = readb(pds + 21) & 0x80; length = readw(pds + 16); image += length * 512; ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-12-04 23:44 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-11-06 12:16 [PATCH] PCI: fix invalid ROM content detection in pci_get_rom_size() Vladis Dronov 2015-11-24 17:03 ` Bjorn Helgaas 2015-11-27 17:05 ` Vladis Dronov 2015-11-27 17:20 ` [PATCH v2] PCI: fix missing ROM content warning " Vladis Dronov 2015-12-04 23:44 ` Bjorn Helgaas
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).