Re: [PATCH v2] tools: ensure zeroed padding in external FIT images
On Fri, Aug 25, 2023 at 10:10:14AM +0200, Roman Azarenko wrote:
> Padding the header of an external FIT image is achieved by truncating
> the existing temporary FIT file to match the required alignment before
> appending image data. Reusing an existing file this way means that the
> padding will likely contain a portion of the original data not
> overwritten by the new header.
>
> Zero out any data past the end of the new header, and stop at either
> the end of the desired padding, or the end of the old FIT file,
> whichever comes first.
>
> Fixes: 7946a814a319 ("Revert "mkimage: fit: Do not tail-pad fitImage with
> external data"")
> Signed-off-by: Roman Azarenko
> Reviewed-by: Simon Glass
Applied to u-boot/master, thanks!
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v2] tools: ensure zeroed padding in external FIT images
On Thu, Aug 31, 2023 at 09:39:52AM +, Roman Azarenko wrote:
> > On Mon, 28 Aug 2023 at 02:00, Roman Azarenko
> wrote:
> > >
> > > On Fri, 2023-08-25 at 12:06 -0600, Simon Glass wrote:
> > > > > @@ -564,9 +564,13 @@ static int fit_extract_data(struct
> > > > > image_tool_params *params, const char *fname)
> > > > > /* Pack the FDT and place the data after it */
> > > > > fdt_pack(fdt);
> > > > >
> > > > > - new_size = fdt_totalsize(fdt);
> > > > > - new_size = ALIGN(new_size, align_size);
> > > > > + unpadded_size = fdt_totalsize(fdt);
> > > > > + new_size = ALIGN(unpadded_size, align_size);
> > > > > fdt_set_totalsize(fdt, new_size);
> > > >
> > > > I didn't know that was allowed...I thought it needed fdt_open_into() ?
> > >
> > > The introduction of fdt_set_totalsize() comes from commit ebfe611be91e
> > > ("mkimage: fit_image: Add option to make fit header align"). The commit
> > > message doesn't describe the choice of this function vs fdt_open_into().
> > >
> > > Personally I'm unable to definitively comment on it. I can only blindly
> > > guess, that because we're only changing the total length of the fdt
> > > struct, and keeping all other fields the same, we don't need to allocate
> > > a new fdt struct with a different size.
> >
> > I am not sure if it would cause problems. I do understand that you
> > didn't write the code. You could copy the people who did (and those
> > that reviewed it) for their input.
> >
>
> @Kever, @Punit, @Tom, could you please comment on this remark by Simon?
> Thank you!
I assume that we're given an already well-ordered device tree at this
point and so that's why we don't use fdt_open_into to move things and
just handle it ourselves at that point in the tool.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v2] tools: ensure zeroed padding in external FIT images
> On Mon, 28 Aug 2023 at 02:00, Roman Azarenko
wrote:
> >
> > On Fri, 2023-08-25 at 12:06 -0600, Simon Glass wrote:
> > > > @@ -564,9 +564,13 @@ static int fit_extract_data(struct
> > > > image_tool_params *params, const char *fname)
> > > > /* Pack the FDT and place the data after it */
> > > > fdt_pack(fdt);
> > > >
> > > > - new_size = fdt_totalsize(fdt);
> > > > - new_size = ALIGN(new_size, align_size);
> > > > + unpadded_size = fdt_totalsize(fdt);
> > > > + new_size = ALIGN(unpadded_size, align_size);
> > > > fdt_set_totalsize(fdt, new_size);
> > >
> > > I didn't know that was allowed...I thought it needed fdt_open_into() ?
> >
> > The introduction of fdt_set_totalsize() comes from commit ebfe611be91e
> > ("mkimage: fit_image: Add option to make fit header align"). The commit
> > message doesn't describe the choice of this function vs fdt_open_into().
> >
> > Personally I'm unable to definitively comment on it. I can only blindly
> > guess, that because we're only changing the total length of the fdt
> > struct, and keeping all other fields the same, we don't need to allocate
> > a new fdt struct with a different size.
>
> I am not sure if it would cause problems. I do understand that you
> didn't write the code. You could copy the people who did (and those
> that reviewed it) for their input.
>
@Kever, @Punit, @Tom, could you please comment on this remark by Simon?
Thank you!
> Could you add another patch before or after this one?
I'll look into this as well, and see if I can figure it out and then
post v3 of the patchset. Thanks!
Re: [PATCH v2] tools: ensure zeroed padding in external FIT images
Hi Roman,
On Mon, 28 Aug 2023 at 02:00, Roman Azarenko wrote:
>
> On Fri, 2023-08-25 at 12:06 -0600, Simon Glass wrote:
> > > @@ -564,9 +564,13 @@ static int fit_extract_data(struct
> > > image_tool_params *params, const char *fname)
> > > /* Pack the FDT and place the data after it */
> > > fdt_pack(fdt);
> > >
> > > - new_size = fdt_totalsize(fdt);
> > > - new_size = ALIGN(new_size, align_size);
> > > + unpadded_size = fdt_totalsize(fdt);
> > > + new_size = ALIGN(unpadded_size, align_size);
> > > fdt_set_totalsize(fdt, new_size);
> >
> > I didn't know that was allowed...I thought it needed fdt_open_into() ?
>
> The introduction of fdt_set_totalsize() comes from commit ebfe611be91e
> ("mkimage: fit_image: Add option to make fit header align"). The commit
> message doesn't describe the choice of this function vs fdt_open_into().
>
> Personally I'm unable to definitively comment on it. I can only blindly
> guess, that because we're only changing the total length of the fdt
> struct, and keeping all other fields the same, we don't need to allocate
> a new fdt struct with a different size.
I am not sure if it would cause problems. I do understand that you
didn't write the code. You could copy the people who did (and those
that reviewed it) for their input.
But I think it should change to call fdt_open_into()...if you look at
that function it does extra things. Unfortunately, the function has no
comment in libfdt.h.
Could you add another patch before or after this one?
Regards,
Simon
Re: [PATCH v2] tools: ensure zeroed padding in external FIT images
On Fri, 2023-08-25 at 12:06 -0600, Simon Glass wrote:
> > @@ -564,9 +564,13 @@ static int fit_extract_data(struct
> > image_tool_params *params, const char *fname)
> > /* Pack the FDT and place the data after it */
> > fdt_pack(fdt);
> >
> > - new_size = fdt_totalsize(fdt);
> > - new_size = ALIGN(new_size, align_size);
> > + unpadded_size = fdt_totalsize(fdt);
> > + new_size = ALIGN(unpadded_size, align_size);
> > fdt_set_totalsize(fdt, new_size);
>
> I didn't know that was allowed...I thought it needed fdt_open_into() ?
The introduction of fdt_set_totalsize() comes from commit ebfe611be91e
("mkimage: fit_image: Add option to make fit header align"). The commit
message doesn't describe the choice of this function vs fdt_open_into().
Personally I'm unable to definitively comment on it. I can only blindly
guess, that because we're only changing the total length of the fdt
struct, and keeping all other fields the same, we don't need to allocate
a new fdt struct with a different size.
Re: [PATCH v2] tools: ensure zeroed padding in external FIT images
On Fri, 25 Aug 2023 at 02:10, Roman Azarenko wrote:
>
> Padding the header of an external FIT image is achieved by truncating
> the existing temporary FIT file to match the required alignment before
> appending image data. Reusing an existing file this way means that the
> padding will likely contain a portion of the original data not
> overwritten by the new header.
>
> Zero out any data past the end of the new header, and stop at either
> the end of the desired padding, or the end of the old FIT file,
> whichever comes first.
>
> Fixes: 7946a814a319 ("Revert "mkimage: fit: Do not tail-pad fitImage with
> external data"")
> Signed-off-by: Roman Azarenko
> ---
> v2:
> - Use memset() on `fdt` instead of ftruncate() on `fd`, zero only
> the part that will become padding in the new FIT file.
> v1: https://lists.denx.de/pipermail/u-boot/2023-August/528213.html
>
> I'm trying to be a bit smart here and only zero out the amount of
> padding that we actually need, but no more than the total length of the
> old FIT file (not to cross the boundary of the mmaped region).
>
> See v1 for info on how to reproduce the original issue.
> ---
> tools/fit_image.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/tools/fit_image.c b/tools/fit_image.c
> index 9fe69ea0d9f8..d9da0ce8388f 100644
> --- a/tools/fit_image.c
> +++ b/tools/fit_image.c
> @@ -497,7 +497,7 @@ static int fit_extract_data(struct image_tool_params
> *params, const char *fname)
> {
> void *buf = NULL;
> int buf_ptr;
> - int fit_size, new_size;
> + int fit_size, unpadded_size, new_size, pad_boundary;
> int fd;
> struct stat sbuf;
> void *fdt;
> @@ -564,9 +564,13 @@ static int fit_extract_data(struct image_tool_params
> *params, const char *fname)
> /* Pack the FDT and place the data after it */
> fdt_pack(fdt);
>
> - new_size = fdt_totalsize(fdt);
> - new_size = ALIGN(new_size, align_size);
> + unpadded_size = fdt_totalsize(fdt);
> + new_size = ALIGN(unpadded_size, align_size);
> fdt_set_totalsize(fdt, new_size);
I didn't know that was allowed...I thought it needed fdt_open_into() ?
> + if (unpadded_size < fit_size) {
> + pad_boundary = new_size < fit_size ? new_size : fit_size;
> + memset(fdt + unpadded_size, 0, pad_boundary - unpadded_size);
> + }
> debug("Size reduced from %x to %x\n", fit_size, fdt_totalsize(fdt));
> debug("External data size %x\n", buf_ptr);
> munmap(fdt, sbuf.st_size);
> --
> 2.42.0
>
Reviewed-by: Simon Glass
[PATCH v2] tools: ensure zeroed padding in external FIT images
Padding the header of an external FIT image is achieved by truncating
the existing temporary FIT file to match the required alignment before
appending image data. Reusing an existing file this way means that the
padding will likely contain a portion of the original data not
overwritten by the new header.
Zero out any data past the end of the new header, and stop at either
the end of the desired padding, or the end of the old FIT file,
whichever comes first.
Fixes: 7946a814a319 ("Revert "mkimage: fit: Do not tail-pad fitImage with
external data"")
Signed-off-by: Roman Azarenko
---
v2:
- Use memset() on `fdt` instead of ftruncate() on `fd`, zero only
the part that will become padding in the new FIT file.
v1: https://lists.denx.de/pipermail/u-boot/2023-August/528213.html
I'm trying to be a bit smart here and only zero out the amount of
padding that we actually need, but no more than the total length of the
old FIT file (not to cross the boundary of the mmaped region).
See v1 for info on how to reproduce the original issue.
---
tools/fit_image.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/fit_image.c b/tools/fit_image.c
index 9fe69ea0d9f8..d9da0ce8388f 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -497,7 +497,7 @@ static int fit_extract_data(struct image_tool_params
*params, const char *fname)
{
void *buf = NULL;
int buf_ptr;
- int fit_size, new_size;
+ int fit_size, unpadded_size, new_size, pad_boundary;
int fd;
struct stat sbuf;
void *fdt;
@@ -564,9 +564,13 @@ static int fit_extract_data(struct image_tool_params
*params, const char *fname)
/* Pack the FDT and place the data after it */
fdt_pack(fdt);
- new_size = fdt_totalsize(fdt);
- new_size = ALIGN(new_size, align_size);
+ unpadded_size = fdt_totalsize(fdt);
+ new_size = ALIGN(unpadded_size, align_size);
fdt_set_totalsize(fdt, new_size);
+ if (unpadded_size < fit_size) {
+ pad_boundary = new_size < fit_size ? new_size : fit_size;
+ memset(fdt + unpadded_size, 0, pad_boundary - unpadded_size);
+ }
debug("Size reduced from %x to %x\n", fit_size, fdt_totalsize(fdt));
debug("External data size %x\n", buf_ptr);
munmap(fdt, sbuf.st_size);
--
2.42.0
