[1/2] sunxi: binman: Add U-Boot binary size check

Message ID 20171019140419.30996-2-maxime.ripard@free-electrons.com
State New
Headers show
Series
  • sunxi: Fix boot of Cubietruk and al.
Related show

Commit Message

Maxime Ripard Oct. 19, 2017, 2:04 p.m.
The U-boot binary may trip over its actual allocated size in the storage.
In such a case, the environment will not be readable anymore (because
corrupted when the new image was flashed), and any attempt at using saveenv
to reconstruct the environment will result in a corrupted U-boot binary.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/dts/sunxi-u-boot.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Bin Meng Oct. 19, 2017, 2:36 p.m. | #1
On Thu, Oct 19, 2017 at 10:04 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> The U-boot binary may trip over its actual allocated size in the storage.
> In such a case, the environment will not be readable anymore (because
> corrupted when the new image was flashed), and any attempt at using saveenv
> to reconstruct the environment will result in a corrupted U-boot binary.
>

nits: U-boot -> U-Boot

Please fix this globally in this commit.

> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  arch/arm/dts/sunxi-u-boot.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi
> index 5adfd9bca2ec..b44660e8d37e 100644
> --- a/arch/arm/dts/sunxi-u-boot.dtsi
> +++ b/arch/arm/dts/sunxi-u-boot.dtsi
> @@ -1,5 +1,13 @@
>  #include <config.h>
>
> +/*
> + * This is the maximum size the U-boot binary can be, which is
> + * basically the start of the environment, minus the (padded) size of
> + * the SPL), minus the offset at which the generated file is supposed
> + * to be flashed in the MMC.
> + */
> +#define UBOOT_MAX_SIZE (CONFIG_ENV_OFFSET - CONFIG_SPL_PAD_TO - (8 << 10))
> +
>  / {
>         binman {
>                 filename = "u-boot-sunxi-with-spl.bin";
> @@ -8,6 +16,9 @@
>                         filename = "spl/sunxi-spl.bin";
>                 };
>                 u-boot-img {
> +#ifdef CONFIG_MMC
> +                       size = <UBOOT_MAX_SIZE>;
> +#endif
>                         pos = <CONFIG_SPL_PAD_TO>;
>                 };
>         };
> --


Regards,
Bin
Andre Przywara Oct. 19, 2017, 3:07 p.m. | #2
Hi,

On 19/10/17 15:04, Maxime Ripard wrote:
> The U-boot binary may trip over its actual allocated size in the storage.
> In such a case, the environment will not be readable anymore (because
> corrupted when the new image was flashed), and any attempt at using saveenv
> to reconstruct the environment will result in a corrupted U-boot binary.

Merci beaucoup for that v2 series!

> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  arch/arm/dts/sunxi-u-boot.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi
> index 5adfd9bca2ec..b44660e8d37e 100644
> --- a/arch/arm/dts/sunxi-u-boot.dtsi
> +++ b/arch/arm/dts/sunxi-u-boot.dtsi
> @@ -1,5 +1,13 @@
>  #include <config.h>
>  
> +/*
> + * This is the maximum size the U-boot binary can be, which is
> + * basically the start of the environment, minus the (padded) size of
> + * the SPL), minus the offset at which the generated file is supposed
> + * to be flashed in the MMC.
> + */
> +#define UBOOT_MAX_SIZE	(CONFIG_ENV_OFFSET - CONFIG_SPL_PAD_TO - (8 << 10))

Just bikeshedding: Can't we use:
	(CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR << 9)
for the last two expressions?

But that's just a nit, so:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> +
>  / {
>  	binman {
>  		filename = "u-boot-sunxi-with-spl.bin";
> @@ -8,6 +16,9 @@
>  			filename = "spl/sunxi-spl.bin";
>  		};
>  		u-boot-img {
> +#ifdef CONFIG_MMC
> +			size = <UBOOT_MAX_SIZE>;
> +#endif
>  			pos = <CONFIG_SPL_PAD_TO>;
>  		};
>  	};
>
Frank Kunz Nov. 2, 2017, 3:53 p.m. | #3
Am Donnerstag, 19. Oktober 2017, 16:04:18 CET schrieb Maxime Ripard:
> The U-boot binary may trip over its actual allocated size in the storage.
> In such a case, the environment will not be readable anymore (because
> corrupted when the new image was flashed), and any attempt at using saveenv
> to reconstruct the environment will result in a corrupted U-boot binary.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  arch/arm/dts/sunxi-u-boot.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Hello,

I found that this patch causes the sunxi-fel tool to fail with:

sunxi-fel uboot u-boot-sunxi-with-spl.bin
U-Boot image data size mismatch: expected 516032, got 389212

Is there a patch for the sunxi tools missing?

Br,
Frank
Maxime Ripard Nov. 3, 2017, 8:42 a.m. | #4
Hi Frank,

On Thu, Nov 02, 2017 at 04:53:54PM +0100, Frank Kunz wrote:
> Am Donnerstag, 19. Oktober 2017, 16:04:18 CET schrieb Maxime Ripard:

> > The U-boot binary may trip over its actual allocated size in the storage.

> > In such a case, the environment will not be readable anymore (because

> > corrupted when the new image was flashed), and any attempt at using saveenv

> > to reconstruct the environment will result in a corrupted U-boot binary.

> > 

> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

> > ---

> >  arch/arm/dts/sunxi-u-boot.dtsi | 11 +++++++++++

> >  1 file changed, 11 insertions(+)

>

> I found that this patch causes the sunxi-fel tool to fail with:

> 

> sunxi-fel uboot u-boot-sunxi-with-spl.bin

> U-Boot image data size mismatch: expected 516032, got 389212

> 

> Is there a patch for the sunxi tools missing?


No, because no one reported it so far.

Can you test with:
http://code.bulix.org/mb0ic7-221765?raw

You'll need the zlib development files installed.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Frank Kunz Nov. 3, 2017, 5:01 p.m. | #5
Am Freitag, 3. November 2017, 09:42:44 CET schrieb Maxime Ripard:
> Hi Frank,
> 
> On Thu, Nov 02, 2017 at 04:53:54PM +0100, Frank Kunz wrote:
> > Am Donnerstag, 19. Oktober 2017, 16:04:18 CET schrieb Maxime Ripard:
> > > The U-boot binary may trip over its actual allocated size in the
> > > storage.
> > > In such a case, the environment will not be readable anymore (because
> > > corrupted when the new image was flashed), and any attempt at using
> > > saveenv
> > > to reconstruct the environment will result in a corrupted U-boot binary.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > ---
> > > 
> > >  arch/arm/dts/sunxi-u-boot.dtsi | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > 
> > I found that this patch causes the sunxi-fel tool to fail with:
> > 
> > sunxi-fel uboot u-boot-sunxi-with-spl.bin
> > U-Boot image data size mismatch: expected 516032, got 389212
> > 
> > Is there a patch for the sunxi tools missing?
> 
> No, because no one reported it so far.
> 
> Can you test with:
> http://code.bulix.org/mb0ic7-221765?raw
> 

That works.

Tested-by: Frank Kunz <mailinglists@kunz-im-inter.net>
Maxime Ripard Nov. 6, 2017, 2:35 p.m. | #6
On Fri, Nov 03, 2017 at 06:01:20PM +0100, Frank Kunz wrote:
> Am Freitag, 3. November 2017, 09:42:44 CET schrieb Maxime Ripard:

> > Hi Frank,

> > 

> > On Thu, Nov 02, 2017 at 04:53:54PM +0100, Frank Kunz wrote:

> > > Am Donnerstag, 19. Oktober 2017, 16:04:18 CET schrieb Maxime Ripard:

> > > > The U-boot binary may trip over its actual allocated size in the

> > > > storage.

> > > > In such a case, the environment will not be readable anymore (because

> > > > corrupted when the new image was flashed), and any attempt at using

> > > > saveenv

> > > > to reconstruct the environment will result in a corrupted U-boot binary.

> > > > 

> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

> > > > ---

> > > > 

> > > >  arch/arm/dts/sunxi-u-boot.dtsi | 11 +++++++++++

> > > >  1 file changed, 11 insertions(+)

> > > 

> > > I found that this patch causes the sunxi-fel tool to fail with:

> > > 

> > > sunxi-fel uboot u-boot-sunxi-with-spl.bin

> > > U-Boot image data size mismatch: expected 516032, got 389212

> > > 

> > > Is there a patch for the sunxi tools missing?

> > 

> > No, because no one reported it so far.

> > 

> > Can you test with:

> > http://code.bulix.org/mb0ic7-221765?raw

> > 

> 

> That works.

> 

> Tested-by: Frank Kunz <mailinglists@kunz-im-inter.net>


I created a pull request for sunxi-tools to fix the issue. Thanks for
the test, and sorry for the bug.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Patch

diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi
index 5adfd9bca2ec..b44660e8d37e 100644
--- a/arch/arm/dts/sunxi-u-boot.dtsi
+++ b/arch/arm/dts/sunxi-u-boot.dtsi
@@ -1,5 +1,13 @@ 
 #include <config.h>
 
+/*
+ * This is the maximum size the U-boot binary can be, which is
+ * basically the start of the environment, minus the (padded) size of
+ * the SPL), minus the offset at which the generated file is supposed
+ * to be flashed in the MMC.
+ */
+#define UBOOT_MAX_SIZE	(CONFIG_ENV_OFFSET - CONFIG_SPL_PAD_TO - (8 << 10))
+
 / {
 	binman {
 		filename = "u-boot-sunxi-with-spl.bin";
@@ -8,6 +16,9 @@ 
 			filename = "spl/sunxi-spl.bin";
 		};
 		u-boot-img {
+#ifdef CONFIG_MMC
+			size = <UBOOT_MAX_SIZE>;
+#endif
 			pos = <CONFIG_SPL_PAD_TO>;
 		};
 	};