[LinuxBIOS] [PATCH] LAR checksums

[Alex Beregszaszi]

Hi,

checksum-fix.diff fixes a checksum calculation bug in the lar utility,
where it would checksum less bytes then desired.

lar-check-sum.diff implements checksum checking in the runtime lar code.

I implemented the check inside the filename check, thus it wont be
executed on unneeded entries. As my assumption was that I cant modify
the memory, the code is skipping bytes between 20-24 of the header
(checksum field) to generate a correct sum.

This is bad, as the numbers shall be updated at every lar structure
change. This is good, as we are not writing to memory.

[Alex Beregszaszi]

Hi,

On Thu, 2007-09-06 at 03:23 +0200, Alex Beregszaszi wrote:
> Hi,
>
> checksum-fix.diff fixes a checksum calculation bug in the lar utility,
> where it would checksum less bytes then desired.
>
> lar-check-sum.diff implements checksum checking in the runtime lar code.
>
> I implemented the check inside the filename check, thus it wont be
> executed on unneeded entries. As my assumption was that I cant modify
> the memory, the code is skipping bytes between 20-24 of the header
> (checksum field) to generate a correct sum.
>
> This is bad, as the numbers shall be updated at every lar structure
> change. This is good, as we are not writing to memory.

Any comments to this?

[Carl-Daniel Hailfinger]

On 06.09.2007 03:23, Alex Beregszaszi wrote:
> Hi,
>
> checksum-fix.diff fixes a checksum calculation bug in the lar utility,
> where it would checksum less bytes then desired.
>
> lar-check-sum.diff implements checksum checking in the runtime lar code.
>
> I implemented the check inside the filename check, thus it wont be
> executed on unneeded entries. As my assumption was that I cant modify
> the memory, the code is skipping bytes between 20-24 of the header
> (checksum field) to generate a correct sum.

Byte 20-24? Our checksum does not have 5 bytes. It seems both your
comment and the implementation suffer from a off-by-one error.
> This is bad, as the numbers shall be updated at every lar structure

Try offsetof() and sizeof() to fix that.
> change. This is good, as we are not writing to memory.

Carl-Daniel
> Signed-off-by: Alex Beregszaszi
> Index: util/lar/stream.c
>
> --- util/lar/stream.c	(revision 494)
> +++ util/lar/stream.c	(working copy)
> @@ -815,7 +825,7 @@
>
> csum = 0;
>    for (walk = (u32 *) (lar->map + offset);
>     walk < (u32 *) (lar->map + complen + hlen);
>     walk < (u32 *) (lar->map + offset + complen + hlen);
>   walk++) {
> csum += ntohl(*walk);
>  }
>
> Signed-off-by: Alex Beregszaszi
> Index: lib/lar.c
>
> --- lib/lar.c	(revision 494)
> +++ lib/lar.c	(working copy)
> @@ -99,9 +99,21 @@
>    	fullname = walk + sizeof(struct lar_header);
> printk(BIOS_SPEW, "LAR: search for %s\n", fullname);
> // FIXME: check checksum
>
> if (strcmp(fullname, filename) == 0) {
> 	u32 csum = 0, *p = (u32 *)walk;
> 	/* validate checksum */
> 	for (; p < (u32 *)(walk+ntohl(header->len)+ntohl(header->offset)); p++)
> 	    /* skip the checksum field itself */
> 	    if (((char*)p-walk) < 20 || ((char*)p - walk) > 24)

see above.
> 		csum += ntohl(*p);
> 	if (csum != ntohl(header->checksum)) {
> 	    printk(BIOS_SPEW, "LAR: checksum failed on %s, skipping (%x != %x)\n",
> 		fullname, csum, ntohl(header->checksum));
> 	    return 1;
> 	}
> printk(BIOS_SPEW, "LAR: CHECK %s @ %p\n", fullname, header);
> result->start = walk + ntohl(header->offset);
> result->len = ntohl(header->len);

[Stefan Reinauer]

* Alex Beregszaszi  [070906 03:23]:
> Hi,
>
> checksum-fix.diff fixes a checksum calculation bug in the lar utility,
> where it would checksum less bytes then desired.
>
> lar-check-sum.diff implements checksum checking in the runtime lar code.
>
> I implemented the check inside the filename check, thus it wont be
> executed on unneeded entries. As my assumption was that I cant modify
> the memory, the code is skipping bytes between 20-24 of the header
> (checksum field) to generate a correct sum.

What about writing 0-(summed u32s) instead of (summed u32s) as
checksum.. this way we could just sum up and should get a result of 0,
when adding the checksum.
The good thing would be, we don't have to care where the checksum sits,
as long as its at a 32bit aligned offset. (Which should be rather simple
to enforce)

Also, this should maybe not be done every time we look at a lar entry.
Otherwise we have to calculate checksums for all files when accessing
the last file in the archive. This can have a noticable impact.

Maybe we should use 2 checksums, one for the header and one for the
rest. When walking the archive, only check the header sums. When loading
a lar entry or XIPing it, check the data/code itself.

What do you think?

[ron minnich]

On 10/17/07, Stefan Reinauer  wrote:
> Maybe we should use 2 checksums, one for the header and one for the
> rest. When walking the archive, only check the header sums. When loading
> a lar entry or XIPing it, check the data/code itself.
>
> What do you think?

that makes a lot of sense, IP made the same decision, interestingly.

ron

[Carl-Daniel Hailfinger]

On 17.10.2007 18:14, ron minnich wrote:
> On 10/17/07, Stefan Reinauer  wrote:
> > Maybe we should use 2 checksums, one for the header and one for the
> > rest. When walking the archive, only check the header sums. When loading
> > a lar entry or XIPing it, check the data/code itself.
> >
> > What do you think?
>
> that makes a lot of sense, IP made the same decision, interestingly.

Alex? Do you want to modify your patch in that way?
Do you have any outstanding patches?

Carl-Daniel

[Alex Beregszaszi]

Hi,

On Wed, 2007-11-14 at 01:21 +0100, Carl-Daniel Hailfinger wrote:
> On 17.10.2007 18:14, ron minnich wrote:
> > On 10/17/07, Stefan Reinauer  wrote:
> > > Maybe we should use 2 checksums, one for the header and one for the
> > > rest. When walking the archive, only check the header sums. When loading
> > > a lar entry or XIPing it, check the data/code itself.
> > >
> > > What do you think?
> >
> > that makes a lot of sense, IP made the same decision, interestingly.
>
> Alex? Do you want to modify your patch in that way?
> Do you have any outstanding patches?

I have been way to busy in the last months, thus my silence.

I think I can prepare a patch this week.