Re: [coreboot] [PATCH] more intelligent cbfs walker

[Myles Watson]

On Sat, Apr 25, 2009 at 6:42 AM, Patrick Georgi  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

+	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