[Skiboot] [PATCH] libflash/libffs: Zero checksum words

[Cyril Bur]

On Mon, 2017-06-26 at 18:51 +0930, Joel Stanley wrote:
> On Mon, Jun 26, 2017 at 11:18 AM, Cyril Bur <cyril.bur at au1.ibm.com> wrote:
> > On writing ffs entries to flash libffs doesn't zero checksum words
> > before calculating the checksum across the entire structure. This causes
> > an inaccurate calculation of the checksum as it may calculate a checksum
> > on non-zero checksum bytes.
> 
> It's kinda crazy that you pass a pointer of the thing you're
> checksumming and assign it to that pointer.
> 
> Instead of relying on the checksum algorithm to perform a noop on
> zero, why don't you just calculate the checksum over the data bytes of
> the structure (stopping before the checksum member)?
> 

Yes this is the other solution. The reason I went with this way is so I
can have one ffs_entry_checksum() and ffs_hdr_checksum(), because when
I read the structure off flash I can pass to it to ffs_*_checksum() -
in that case I do want to checksum including the checksum word.
Otherwise we'll need checksum functions which magically subtracts
sizeof(checksum) from the size of the structure and one that doesn't
when returning the checksum.

> > 
> > This patch solves this by zeroing the entire structure which is to be
> > written to the flash before calculating the checksum across the struct.
> > 
> > Fixes: 602dee45 libflash/libffs: Rework libffs
> > Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> > ---
> >  libflash/libffs.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/libflash/libffs.c b/libflash/libffs.c
> > index 763e061c..ab6f9328 100644
> > --- a/libflash/libffs.c
> > +++ b/libflash/libffs.c
> > @@ -144,6 +144,14 @@ static int ffs_entry_to_flash(struct ffs_hdr *hdr,
> >         if (!ent)
> >                 return FFS_ERR_PART_NOT_FOUND;
> > 
> > +       /*
> > +        * So that the checksum gets calculated correctly at least the
> > +        * dst->checksum must be zero before calling ffs_entry_checksum()
> > +        * memset()ting the entire struct to zero is probably wise as it
> > +        * appears the reserved fields are always zero.
> > +        */
> > +       memset(dst, 0, sizeof(*dst));
> 
> I'm struggling to follow your code, but it looks like the memory
> pointed to by dst comes from the allocation on line 622, which is
> already zeroed?
> 

You mean the malloc() that you suggest I turn into a calloc() below?
Perhaps but ffs_entry_to_flash() is also called with a stack allocated
dst in ffs_update_act_size() line 775, this is the problem. That's also
why the bug wasn't caught - a lot of the testing happens on x86 where
that stack allocation is always zeroed, not so on power.

Also I'm not sure it's wise to rely on the caller to zero.

> Double check, as it was hard to follow.
> 
> > +
> >         memcpy(dst->name, src->name, sizeof(dst->name));
> >         dst->name[FFS_PART_NAME_MAX] = '\0';
> >         dst->base = cpu_to_be32(src->base / hdr->block_size);
> > @@ -623,6 +631,14 @@ int ffs_hdr_finalise(struct blocklevel_device *bl, struct ffs_hdr *hdr)
> >         if (!real_hdr)
> >                 return FLASH_ERR_MALLOC_FAILED;
> > 
> > +       /*
> > +        * So that the checksum gets calculated correctly at least the
> > +        * real_hdr->checksum must be zero before calling ffs_hdr_checksum()
> > +        * memset()ting the entire struct to zero is probably wise as it
> > +        * appears the reserved fields are always zero.
> > +        */
> > +       memset(real_hdr, 0, sizeof(*real_hdr));
> 
> You allocate this memory about three lines above that comment. How
> about you allocate with calloc instead?
> 

So the malloc() actually allocates a lot more than sizeof(*read_hdr),
struct __ffs_hdr has a static array of entries after it, which are
going to get individually zeroed again because of what I mention above.


Cyril


> Cheers,
> 
> Joel
> 
> > +
> >         real_hdr->magic = cpu_to_be32(FFS_MAGIC);
> >         real_hdr->version = cpu_to_be32(hdr->version);
> >         real_hdr->size = cpu_to_be32(hdr->size / hdr->block_size);
> > --
> > 2.13.1
> > 
> > _______________________________________________
> > Skiboot mailing list
> > Skiboot at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/skiboot
> 
>