[LinuxBIOS] [PATCH] pci_rom.c checksum extension rom

[Alex Beregszaszi]

Hi,

the attached patch adds code to checksum the pci extension rom and stop
if the stored and calculated checksum differ.

[Stefan Reinauer]

* Alex Beregszaszi  [070904 13:17]:
> Hi,
>
> the attached patch adds code to checksum the pci extension rom and stop
> if the stored and calculated checksum differ.

Is this checksum reliably correct? I am hesitating to add new
restrictions that might break otherwise working cards.

[Alex Beregszaszi]

Hi,

On Wed, 2007-09-05 at 04:29 +0200, Stefan Reinauer wrote:
> Alex Beregszaszi  [070904 13:17]:
> > Hi,
> >
> > the attached patch adds code to checksum the pci extension rom and stop
> > if the stored and calculated checksum differ.
>
> Is this checksum reliably correct? I am hesitating to add new
> restrictions that might break otherwise working cards.

This algorithm is used in vgabios to set the checksum, and bochsbios to
check the option roms. If you think this makes too much restrictions,
you might make this only a warning and not a fail condition. Or defined
under a PARANOID_SECURITY option :)

[Alex Beregszaszi]

Hi,

On Wed, 2007-09-05 at 04:29 +0200, Stefan Reinauer wrote:
> Alex Beregszaszi  [070904 13:17]:
> > Hi,
> >
> > the attached patch adds code to checksum the pci extension rom and stop
> > if the stored and calculated checksum differ.
>
> Is this checksum reliably correct? I am hesitating to add new
> restrictions that might break otherwise working cards.

You are right, attached is a correct method. There is no fixed checksum
byte, instead the whole should sum to zero.

[Alex Beregszaszi]

Hi,

On Fri, 2007-09-07 at 12:54 +0200, Alex Beregszaszi wrote:
> Hi,
>
> On Wed, 2007-09-05 at 04:29 +0200, Stefan Reinauer wrote:
> > Alex Beregszaszi  [070904 13:17]:
> > > Hi,
> > >
> > > the attached patch adds code to checksum the pci extension rom and stop
> > > if the stored and calculated checksum differ.
> >
> > Is this checksum reliably correct? I am hesitating to add new
> > restrictions that might break otherwise working cards.
>
> You are right, attached is a correct method. There is no fixed checksum
> byte, instead the whole should sum to zero.

Any comments on this?

[Carl-Daniel Hailfinger]

On 12.09.2007 21:37, Alex Beregszaszi wrote:
> Hi,
>
> On Fri, 2007-09-07 at 12:54 +0200, Alex Beregszaszi wrote:
> > Hi,
> >
> > On Wed, 2007-09-05 at 04:29 +0200, Stefan Reinauer wrote:
> > > Alex Beregszaszi  [070904 13:17]:
> > > > Hi,
> > > >
> > > > the attached patch adds code to checksum the pci extension rom and stop
> > > > if the stored and calculated checksum differ.
> > >
> > > Is this checksum reliably correct? I am hesitating to add new
> > > restrictions that might break otherwise working cards.
> >
> > You are right, attached is a correct method. There is no fixed checksum
> > byte, instead the whole should sum to zero.
>
> Any comments on this?

I like it. I'd give you an Ack, but I have no hardware to test with.

Stefan? Ron?

Worst case would be that buggy extension ROMs break with a really loud
warning, so anybody with such ROMs will see it prominently in the logs.

Alex: Is there an easy way to check extension ROMs in the current
machine for correct signatures? Maybe standalone utility or such stuff.

Carl-Daniel

[Alex Beregszaszi]

On Sat, 2007-09-29 at 23:49 +0200, Carl-Daniel Hailfinger wrote:
> On 12.09.2007 21:37, Alex Beregszaszi wrote:
> > Hi,
> >
> > On Fri, 2007-09-07 at 12:54 +0200, Alex Beregszaszi wrote:
> > > Hi,
> > >
> > > On Wed, 2007-09-05 at 04:29 +0200, Stefan Reinauer wrote:
> > > > Alex Beregszaszi  [070904 13:17]:
> > > > > Hi,
> > > > >
> > > > > the attached patch adds code to checksum the pci extension rom and stop
> > > > > if the stored and calculated checksum differ.
> > > >
> > > > Is this checksum reliably correct? I am hesitating to add new
> > > > restrictions that might break otherwise working cards.
> > >
> > > You are right, attached is a correct method. There is no fixed checksum
> > > byte, instead the whole should sum to zero.
> >
> > Any comments on this?
>
> I like it. I'd give you an Ack, but I have no hardware to test with.
>
> Stefan? Ron?
>
> Worst case would be that buggy extension ROMs break with a really loud
> warning, so anybody with such ROMs will see it prominently in the logs.
>
> Alex: Is there an easy way to check extension ROMs in the current
> machine for correct signatures? Maybe standalone utility or such stuff.

There is no utility for that, but you could copy out the memory between
0xc0000 - 0xf00000 from /dev/mem and search for extension headers in it
(see the code in pci_rom.c).

[Carl-Daniel Hailfinger]

On 03.10.2007 01:48, Alex Beregszaszi wrote:
> On Sat, 2007-09-29 at 23:49 +0200, Carl-Daniel Hailfinger wrote:
> > On 12.09.2007 21:37, Alex Beregszaszi wrote:
> > > Hi,
> > >
> > > On Fri, 2007-09-07 at 12:54 +0200, Alex Beregszaszi wrote:
> > > > Hi,
> > > >
> > > > On Wed, 2007-09-05 at 04:29 +0200, Stefan Reinauer wrote:
> > > > > Alex Beregszaszi  [070904 13:17]:
> > > > > > Hi,
> > > > > >
> > > > > > the attached patch adds code to checksum the pci extension rom and stop
> > > > > > if the stored and calculated checksum differ.
> > > > >
> > > > > Is this checksum reliably correct? I am hesitating to add new
> > > > > restrictions that might break otherwise working cards.
> > > >
> > > > You are right, attached is a correct method. There is no fixed checksum
> > > > byte, instead the whole should sum to zero.
> > >
> > > Any comments on this?
> >
> > I like it. I'd give you an Ack, but I have no hardware to test with.
> >
> > Stefan? Ron?
> >
> > Worst case would be that buggy extension ROMs break with a really loud
> > warning, so anybody with such ROMs will see it prominently in the logs.
> >
> > Alex: Is there an easy way to check extension ROMs in the current
> > machine for correct signatures? Maybe standalone utility or such stuff.
>
> There is no utility for that, but you could copy out the memory between
> 0xc0000 - 0xf00000 from /dev/mem and search for extension headers in it
> (see the code in pci_rom.c).

Can you commit with the following changelog?

--------
The attached patch adds code to checksum the pci extension rom and abort
if the stored and calculated checksum differ.

Worst case would be that buggy extension ROMs break with a really loud
warning, so anybody with such ROMs will see it prominently in the logs.

There is no easy way to check extension ROMs in the current machine for
correct signatures, but you could copy out the memory between 0xc0000 -
0xf00000 from /dev/mem and search for extension headers in it (see the
code in pci_rom.c).
--------

Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[ron minnich]

On 10/5/07, Carl-Daniel Hailfinger  wrote:
> Worst case would be that buggy extension ROMs break with a really loud
> warning, so anybody with such ROMs will see it prominently in the logs.

what logs? What if it is the VGA BIOS and there is no serial console
(sun ultra40)
> There is no easy way to check extension ROMs in the current machine for
> correct signatures,

I am not sure I agree with this. There are ways to do this check.

ron

[Peter Stuge]

On Thu, Oct 18, 2007 at 12:27:27AM -0700, ron minnich wrote:
> what logs? What if [..] there is no serial console (sun ultra40)

Debug port?

//Peter

[Uwe Hermann]

On Thu, Oct 25, 2007 at 02:08:21AM +0200, Peter Stuge wrote:
> On Thu, Oct 18, 2007 at 12:27:27AM -0700, ron minnich wrote:
> > what logs? What if [..] there is no serial console (sun ultra40)
>
> Debug port?

Yes, but not everybody has that. In the worst case, even if you _do_
have the hardware, the USB controller might not support the Debug Port
functionality.

I agree with Ron that this should be a warning only (without aborting
upon incorrect checksum, just warning about it).

Uwe.

[Alex Beregszaszi]

On Fri, 2007-10-05 at 18:41 +0200, Carl-Daniel Hailfinger wrote:
> On 03.10.2007 01:48, Alex Beregszaszi wrote:
> > On Sat, 2007-09-29 at 23:49 +0200, Carl-Daniel Hailfinger wrote:
> > > On 12.09.2007 21:37, Alex Beregszaszi wrote:
> > > > Hi,
> > > >
> > > > On Fri, 2007-09-07 at 12:54 +0200, Alex Beregszaszi wrote:
> > > > > Hi,
> > > > >
> > > > > On Wed, 2007-09-05 at 04:29 +0200, Stefan Reinauer wrote:
> > > > > > Alex Beregszaszi  [070904 13:17]:
> > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > the attached patch adds code to checksum the pci extension rom and stop
> > > > > > > if the stored and calculated checksum differ.
> > > > > > Is this checksum reliably correct? I am hesitating to add new
> > > > > > restrictions that might break otherwise working cards.
> > > > >
> > > > > You are right, attached is a correct method. There is no fixed checksum
> > > > > byte, instead the whole should sum to zero.
> > > >
> > > > Any comments on this?
> > >
> > > I like it. I'd give you an Ack, but I have no hardware to test with.
> > >
> > > Stefan? Ron?
> > >
> > > Worst case would be that buggy extension ROMs break with a really loud
> > > warning, so anybody with such ROMs will see it prominently in the logs.
> > >
> > > Alex: Is there an easy way to check extension ROMs in the current
> > > machine for correct signatures? Maybe standalone utility or such stuff.
> >
> > There is no utility for that, but you could copy out the memory between
> > 0xc0000 - 0xf00000 from /dev/mem and search for extension headers in it
> > (see the code in pci_rom.c).
>
> Can you commit with the following changelog?
>
> The attached patch adds code to checksum the pci extension rom and abort
> if the stored and calculated checksum differ.
>
> Worst case would be that buggy extension ROMs break with a really loud
> warning, so anybody with such ROMs will see it prominently in the logs.
>
> There is no easy way to check extension ROMs in the current machine for
> correct signatures, but you could copy out the memory between 0xc0000 -
> 0xf00000 from /dev/mem and search for extension headers in it (see the
> code in pci_rom.c).
>
> Acked-by: Carl-Daniel Hailfinger

Finally done.

[ron minnich]

On 9/12/07, Alex Beregszaszi  wrote:
> Any comments on this?

It worries me, given the habit pc vendors have of shipping broken IRQ,
ACPI, and other tables, to make us dependent on them actually getting
the checksum correct. Unless and until we do some testing to verify
that the checksums are correct on a  lot of cards, I would rather make
this a warning rather than an error.

Look at it this way. What is a factory bios going to do with an
expansion card with a bad checksum? My bet is they run it.

ron

[Stefan Reinauer]

* ron minnich  [071018 09:25]:
> On 9/12/07, Alex Beregszaszi  wrote:
> It worries me, given the habit pc vendors have of shipping broken IRQ,
> ACPI, and other tables, to make us dependent on them actually getting
> the checksum correct. Unless and until we do some testing to verify
> that the checksums are correct on a  lot of cards, I would rather make
> this a warning rather than an error.

Yes, it needs to be a warning!

[Stefan Reinauer]

* Alex Beregszaszi  [070907 12:54]:
> Hi,
>
> On Wed, 2007-09-05 at 04:29 +0200, Stefan Reinauer wrote:
> > Alex Beregszaszi  [070904 13:17]:
> > > Hi,
> > >
> > > the attached patch adds code to checksum the pci extension rom and stop
> > > if the stored and calculated checksum differ.
> >
> > Is this checksum reliably correct? I am hesitating to add new
> > restrictions that might break otherwise working cards.
>
> You are right, attached is a correct method. There is no fixed checksum
> byte, instead the whole should sum to zero.
>
> Signed-off-by: Alex Beregszaszi

Acked-by: Stefan Reinauer
> Index: device/pci_rom.c
>
> --- device/pci_rom.c	(revision 494)
> +++ device/pci_rom.c	(working copy)
> @@ -33,6 +33,8 @@
>    unsigned long rom_address;
>    struct rom_header *rom_header;
>    struct pci_data *rom_data;
>
> unsigned int i;
>
> unsigned char sum = 0, *rom_bytes;
>
> if (dev->on_mainboard) {
> /* In case some device PCI_ROM_ADDRESS can not be set
>
> @@ -67,7 +69,17 @@
>    	       le32_to_cpu(rom_header->signature));
>    	return NULL;
>    }
>
> /* checksum */
>
> rom_bytes = (unsigned char *)rom_address;
>
> for (i = 0; i < rom_header->size * 512; i++)
>    sum += *(rom_bytes + i);
>
> if (sum != 0) {
> printk(BIOS_ERR, "Incorrent Expansion ROM checksum (%02x != 0)\n", sum);
> return NULL;
>
> }
>
> rom_data = (struct pci_data *)((unsigned char *)rom_header +
>   		       le32_to_cpu(rom_header->data));

[Uwe Hermann]

On Wed, Oct 17, 2007 at 11:30:30AM +0200, Stefan Reinauer wrote:
> > > Is this checksum reliably correct? I am hesitating to add new
> > > restrictions that might break otherwise working cards.
> >
> > You are right, attached is a correct method. There is no fixed checksum
> > byte, instead the whole should sum to zero.
> >
> > Signed-off-by: Alex Beregszaszi
>
> Acked-by: Stefan Reinauer
> > Index: device/pci_rom.c
> >
> > --- device/pci_rom.c	(revision 494)
> > +++ device/pci_rom.c	(working copy)
> > @@ -33,6 +33,8 @@
> >    unsigned long rom_address;
> >    struct rom_header *rom_header;
> >    struct pci_data *rom_data;
> >
> > unsigned int i;
> >
> > unsigned char sum = 0, *rom_bytes;
> >
> > if (dev->on_mainboard) {
> > /* In case some device PCI_ROM_ADDRESS can not be set
> >
> > @@ -67,7 +69,17 @@
> >    	       le32_to_cpu(rom_header->signature));
> >    	return NULL;
> >    }
> >
> > /* checksum */
> >
> > rom_bytes = (unsigned char *)rom_address;
> >
> > for (i = 0; i < rom_header->size * 512; i++)
> >    sum += *(rom_bytes + i);
> >
> > if (sum != 0) {
> > printk(BIOS_ERR, "Incorrent Expansion ROM checksum (%02x != 0)\n", sum);
> > return NULL;
> >
> > }

Doesn't this mean it'll abort upon incorrect checksum? If so, NACK.
This should be a warning only, and not abort execution.

If it's implemented as warning only (without aborting):
Acked-by: Uwe Hermann

Uwe.