[coreboot] [PATCH] more intelligent cbfs walker

[Myles Watson]

> -----Original Message-----
> From: Patrick Georgi [mailto:patrick at georgi-clan.de]
> Sent: Saturday, April 25, 2009 6:57 AM
> To: Myles Watson
> Cc: coreboot at coreboot.org
> Subject: Re: [coreboot] [PATCH] more intelligent cbfs walker
> 
> Am 25.04.2009 14:53, schrieb Myles Watson:
> > +		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)?
> >
> offset == ALIGN(offset + 0 + 0, align) // offset is already aligned
>
> If, for some twisted reason, right after the last file, there's a CBFS
> file magic, and otherwise zeroes (and offset is already aligned, which
> it should be), you end up in an endless loop. This test ends it.

OK.  I think that would be more clear if we tested foffset.  How about 
if (foffset == 0) /* Invalid CBFS entry that would cause an infinite loop */
	return NULL;

Or we could just test foffset when we test the magic number.

> > +
> >   		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?
> >
> Good idea. Another issue, another patch.
Sure.  

Thanks,
Myles