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

[Joel Stanley]

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)?

>
> 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?

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?

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