[LinuxBIOS] [PATCH] v3: lar header walk off-by-one

[Carl-Daniel Hailfinger]

OK, this patch is not for the weak of hart because it touches some logic
we had lots of problems with in the past.
Basically, the lar archive header walk will not see a header at the end
of the archive if nothing is after that header. However, since a file
with zero size is a legal lar member, we really can't forbid to look at
a header terminating the archive.

Signed-off-by: Carl-Daniel Hailfinger

Index: LinuxBIOSv3/lib/lar.c
===================================================================
--- LinuxBIOSv3/lib/lar.c	(Revision 501)
+++ LinuxBIOSv3/lib/lar.c	(Arbeitskopie)
@@ -90,7 +90,7 @@
      * Unsigned pointer arithmetic that wraps to zero can be messy.
      */
    for (walk = archive->start;
-	     (walk < (char *)(archive->start + archive->len - sizeof(struct lar_header))) && 
+	     (walk <= (char *)(archive->start + archive->len - sizeof(struct lar_header))) && 
    		(walk >= (char *)archive->start); walk += 16) {
    	if (strncmp(walk, MAGIC, 8) != 0)
    		continue;

[ron minnich]

On 9/26/07, Carl-Daniel Hailfinger  wrote:
> OK, this patch is not for the weak of hart because it touches some logic
> we had lots of problems with in the past.
> Basically, the lar archive header walk will not see a header at the end
> of the archive if nothing is after that header. However, since a file
> with zero size is a legal lar member, we really can't forbid to look at
> a header terminating the archive.

I need to be convinced that there is any value if having a header for
a zero-length file at the very end of the archive. I think this is way
more capability than we need, and I am very paranoid about LAR, given
the subtle bugs that we have had. Otherwise, I would rather not commit
this.

thanks

ron

[Carl-Daniel Hailfinger]

On 26.09.2007 18:11, ron minnich wrote:
> On 9/26/07, Carl-Daniel Hailfinger  wrote:
> > OK, this patch is not for the weak of hart because it touches some logic
> > we had lots of problems with in the past.
> > Basically, the lar archive header walk will not see a header at the end
> > of the archive if nothing is after that header. However, since a file
> > with zero size is a legal lar member, we really can't forbid to look at
> > a header terminating the archive.
>
> I need to be convinced that there is any value if having a header for
> a zero-length file at the very end of the archive. I think this is way
> more capability than we need, and I am very paranoid about LAR, given
> the subtle bugs that we have had. Otherwise, I would rather not commit
> this.

I have a case where this is needed if we don't want to maintain two
different lar archive formats:
AFAIK not all architectures start executing code at the top of ROM, some
start at the bottom (low address). In that case, we can't start the
archive with a LAR header, but we can end it with one. We could redefine
offset as signed instead of unsigned and make the last lar header point
to the boot block at the beginning. That way, even bottom boot
architectures can use the current format unless the flash size is >2GB.

Current LAR archives look like this:

Hn=header n
-=padding
Dn=data n
H0D0D0D0H1D1----H2D2D2
where the last bytes of D2 are executed after poweron.

The patch (together with signed offset) would make archives like the
below one possible:
D0D0D0H1D1----H2D2D2H0 (header at end of archive points to boot block)
or this one:
D0D0D0H0H1D1----H2D2D2 (header after boot block points to boot block)

Both new archives allow placing a boot block at the beginning without
sacrificing the goal of one common archive format. However, there are
two potential problems:
- endless loops if we aren't careful when walking the LAR
- "nobody will need more than 640kB", in our case it would be 2GB

Regards,
Carl-Daniel

[ron minnich]

OK, I like signed offset, but let's let stefan weigh in. It's critical
that we get this right.

ron

[Uwe Hermann]

On Thu, Sep 27, 2007 at 10:19:57AM -0700, ron minnich wrote:
> OK, I like signed offset, but let's let stefan weigh in. It's critical
> that we get this right.

I don't have any particular opinion here, but I'll NACK the patch in
this form. If everybody else agrees with the patch _and_ there are at least
3-4 lines of code comments which explain all the issues involved here, the
drawbacks and advantages of this (and the alternative) solution(s) etc.,
I'll be happy to ACK :) As already stated, this is way too tricky and
non-obvious to go in without a good code comment.

Uwe.

[Carl-Daniel Hailfinger]

On 28.09.2007 15:33, Uwe Hermann wrote:
> On Thu, Sep 27, 2007 at 10:19:57AM -0700, ron minnich wrote:
> > OK, I like signed offset, but let's let stefan weigh in. It's critical
> > that we get this right.
>
> I don't have any particular opinion here, but I'll NACK the patch in
> this form. If everybody else agrees with the patch _and_ there are at least
> 3-4 lines of code comments which explain all the issues involved here, the
> drawbacks and advantages of this (and the alternative) solution(s) etc.,
> I'll be happy to ACK :) As already stated, this is way too tricky and
> non-obvious to go in without a good code comment.

I'll add a detailed writeup and remove incorrect comments so that the
comments match the code again.

Carl-Daniel