diff mbox series

Revert "mkimage: fit: Do not tail-pad fitImage with external data"

Message ID 20200506150517.28103-1-trini@konsulko.com
State Accepted
Commit 7946a814a31989998120b4b4aa417222ba21b2fa
Headers show
Series Revert "mkimage: fit: Do not tail-pad fitImage with external data" | expand

Commit Message

Tom Rini May 6, 2020, 3:05 p.m. UTC
This has been reported to break booting of U-Boot from SPL on a number
of platforms due to a lack of alignment of the external data.  The
issues this commit is addressing will need to be resolved another way.

This reverts commit 20a154f95bfe0a3b5bfba90bea7f001c58217536.

Reported-by: Alex Kiernan <alex.kiernan at gmail.com>
Reported-by: Michael Walle <michael at walle.cc>
Signed-off-by: Tom Rini <trini at konsulko.com>
---
 tools/fit_image.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Marek Vasut May 6, 2020, 3:08 p.m. UTC | #1
On 5/6/20 5:05 PM, Tom Rini wrote:
> This has been reported to break booting of U-Boot from SPL on a number
> of platforms due to a lack of alignment of the external data.  The
> issues this commit is addressing will need to be resolved another way.
> 
> This reverts commit 20a154f95bfe0a3b5bfba90bea7f001c58217536.
> 
> Reported-by: Alex Kiernan <alex.kiernan at gmail.com>
> Reported-by: Michael Walle <michael at walle.cc>
> Signed-off-by: Tom Rini <trini at konsulko.com>

The commit message should also warn that this re-opens the data leak in
the padding.
Tom Rini May 6, 2020, 3:19 p.m. UTC | #2
On Wed, May 06, 2020 at 05:08:12PM +0200, Marek Vasut wrote:
> On 5/6/20 5:05 PM, Tom Rini wrote:
> > This has been reported to break booting of U-Boot from SPL on a number
> > of platforms due to a lack of alignment of the external data.  The
> > issues this commit is addressing will need to be resolved another way.
> > 
> > This reverts commit 20a154f95bfe0a3b5bfba90bea7f001c58217536.
> > 
> > Reported-by: Alex Kiernan <alex.kiernan at gmail.com>
> > Reported-by: Michael Walle <michael at walle.cc>
> > Signed-off-by: Tom Rini <trini at konsulko.com>
> 
> The commit message should also warn that this re-opens the data leak in
> the padding.

Sure.  Are you going to send a patch switching to calloc or should I?
Marek Vasut May 6, 2020, 3:23 p.m. UTC | #3
On 5/6/20 5:19 PM, Tom Rini wrote:
> On Wed, May 06, 2020 at 05:08:12PM +0200, Marek Vasut wrote:
>> On 5/6/20 5:05 PM, Tom Rini wrote:
>>> This has been reported to break booting of U-Boot from SPL on a number
>>> of platforms due to a lack of alignment of the external data.  The
>>> issues this commit is addressing will need to be resolved another way.
>>>
>>> This reverts commit 20a154f95bfe0a3b5bfba90bea7f001c58217536.
>>>
>>> Reported-by: Alex Kiernan <alex.kiernan at gmail.com>
>>> Reported-by: Michael Walle <michael at walle.cc>
>>> Signed-off-by: Tom Rini <trini at konsulko.com>
>>
>> The commit message should also warn that this re-opens the data leak in
>> the padding.
> 
> Sure.  Are you going to send a patch switching to calloc or should I?

I think the discussion about the padding requirement isn't finished yet?
See my remark about the load = <>; part.
Tom Rini May 6, 2020, 3:31 p.m. UTC | #4
On Wed, May 06, 2020 at 05:23:55PM +0200, Marek Vasut wrote:
> On 5/6/20 5:19 PM, Tom Rini wrote:
> > On Wed, May 06, 2020 at 05:08:12PM +0200, Marek Vasut wrote:
> >> On 5/6/20 5:05 PM, Tom Rini wrote:
> >>> This has been reported to break booting of U-Boot from SPL on a number
> >>> of platforms due to a lack of alignment of the external data.  The
> >>> issues this commit is addressing will need to be resolved another way.
> >>>
> >>> This reverts commit 20a154f95bfe0a3b5bfba90bea7f001c58217536.
> >>>
> >>> Reported-by: Alex Kiernan <alex.kiernan at gmail.com>
> >>> Reported-by: Michael Walle <michael at walle.cc>
> >>> Signed-off-by: Tom Rini <trini at konsulko.com>
> >>
> >> The commit message should also warn that this re-opens the data leak in
> >> the padding.
> > 
> > Sure.  Are you going to send a patch switching to calloc or should I?
> 
> I think the discussion about the padding requirement isn't finished yet?
> See my remark about the load = <>; part.

I'm unsure what more you need to provide a potential fix to the
platforms that have reported breakage.  I suspect you might even have a
platform handy that is broken, given the imx platform.
Marek Vasut May 6, 2020, 4:18 p.m. UTC | #5
On 5/6/20 5:31 PM, Tom Rini wrote:
> On Wed, May 06, 2020 at 05:23:55PM +0200, Marek Vasut wrote:
>> On 5/6/20 5:19 PM, Tom Rini wrote:
>>> On Wed, May 06, 2020 at 05:08:12PM +0200, Marek Vasut wrote:
>>>> On 5/6/20 5:05 PM, Tom Rini wrote:
>>>>> This has been reported to break booting of U-Boot from SPL on a number
>>>>> of platforms due to a lack of alignment of the external data.  The
>>>>> issues this commit is addressing will need to be resolved another way.
>>>>>
>>>>> This reverts commit 20a154f95bfe0a3b5bfba90bea7f001c58217536.
>>>>>
>>>>> Reported-by: Alex Kiernan <alex.kiernan at gmail.com>
>>>>> Reported-by: Michael Walle <michael at walle.cc>
>>>>> Signed-off-by: Tom Rini <trini at konsulko.com>
>>>>
>>>> The commit message should also warn that this re-opens the data leak in
>>>> the padding.
>>>
>>> Sure.  Are you going to send a patch switching to calloc or should I?
>>
>> I think the discussion about the padding requirement isn't finished yet?
>> See my remark about the load = <>; part.
> 
> I'm unsure what more you need to provide a potential fix to the
> platforms that have reported breakage.  I suspect you might even have a
> platform handy that is broken, given the imx platform.

Let's continue the discussion in one thread.

I think we should only ever enforce the padding if really required.
Apply this one and add the calloc fix, but we should fix the root cause,
not go back to papering over this problem.
Tom Rini May 7, 2020, 1:05 p.m. UTC | #6
On Wed, May 06, 2020 at 11:05:17AM -0400, Tom Rini wrote:

> This has been reported to break booting of U-Boot from SPL on a number
> of platforms due to a lack of alignment of the external data.  The
> issues this commit is addressing will need to be resolved another way.
> 
> This reverts commit 20a154f95bfe0a3b5bfba90bea7f001c58217536.
> 
> Reported-by: Alex Kiernan <alex.kiernan at gmail.com>
> Reported-by: Michael Walle <michael at walle.cc>
> Signed-off-by: Tom Rini <trini at konsulko.com>

With a note about the information leak re-added:

Applied to u-boot/master, thanks!
diff mbox series

Patch

diff --git a/tools/fit_image.c b/tools/fit_image.c
index 1e0f1e9fce8b..88ff093d05be 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -435,7 +435,7 @@  static int fit_extract_data(struct image_tool_params *params, const char *fname)
 	int image_number;
 	int align_size;
 
-	align_size = params->bl_len ? params->bl_len : 1;
+	align_size = params->bl_len ? params->bl_len : 4;
 	fd = mmap_fdt(params->cmdname, fname, 0, &fdt, &sbuf, false, false);
 	if (fd < 0)
 		return -EIO;
@@ -493,6 +493,7 @@  static int fit_extract_data(struct image_tool_params *params, const char *fname)
 	fdt_pack(fdt);
 
 	new_size = fdt_totalsize(fdt);
+	new_size = ALIGN(new_size, align_size);
 	fdt_set_totalsize(fdt, new_size);
 	debug("Size reduced from %x to %x\n", fit_size, fdt_totalsize(fdt));
 	debug("External data size %x\n", buf_ptr);