On Sat, Apr 25, 2009 at 6:42 AM, Patrick Georgi <[email protected]> wrote:
> Am 25.04.2009 14:14, schrieb Myles Watson:
>>
>> I'd prefer that it used the ALIGN macro, but it's a pretty trivial
>> macro. It just makes it more clear.
>>
>
> Beware, this patch might create an infinite loop.
> Attached patch avoids the infinite loop, ends lookup on invalid magic (we
> can do this now), and uses ALIGN.
> Other than the original patch this one is only compile tested, so please
> test.
I won't be able to until Monday, but I have a couple of comments/questions.
>
> Signed-off-by: Patrick Georgi <[email protected]>
>
+ int align= ntohl(header->align);
+
while(1) {
struct cbfs_file *file = (struct cbfs_file *) offset;
- if (cbfs_check_magic(file)) printk_info("Check %s\n",
CBFS_NAME(file));
- if (cbfs_check_magic(file) &&
- !strcmp(CBFS_NAME(file), name))
+ if (!cbfs_check_magic(file)) return NULL;
+ printk_info("Check %s\n", CBFS_NAME(file));
+ if (!strcmp(CBFS_NAME(file), name))
return file;
- offset += ntohl(header->align);
+ int flen = ntohl(file->len);
+ int foffset = ntohl(file->offset);
+ printk_spew("CBFS: follow chain: %p + %x + %x + align -> ",
offset,
foffset, flen);
+ unsigned long oldoffset = offset;
+ offset = ALIGN(offset + foffset + flen, align);
+ printk_spew("%p\n", offset);
+ if (offset == oldoffset) return NULL;
Why do we have this check? Is there a time when offset ==
ALIGN(offset + foffset + flen, align)?
+
if (offset < 0xFFFFFFFF - ntohl(header->romsize))
return NULL;
I know this line isn't part of your change, but shouldn't we check
that we're within the file system, not just within the flash?
}
Thanks,
Myles
--
coreboot mailing list: [email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
