Re: [LinuxBIOS] [PATCH] LAR checksums

[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);