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

[Siarhei Siamashka]

On Tue, 01 May 2018 18:25:06 +0100
Måns Rullgård <mans at mansr.com> wrote:

> Maxime Ripard <maxime.ripard at free-electrons.com> writes:
> 
> > 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.
> >
> > Reviewed-by: Andre Przywara <andre.przywara at arm.com>
> > Signed-off-by: Maxime Ripard <maxime.ripard at free-electrons.com>
> > ---
> >  arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi
> > index 5adfd9bca2ec..72e95afd780e 100644
> > --- a/arch/arm/dts/sunxi-u-boot.dtsi
> > +++ b/arch/arm/dts/sunxi-u-boot.dtsi
> > @@ -1,5 +1,14 @@
> >  #include <config.h>
> >
> > +/*
> > + * This is the maximum size the U-Boot binary can be, which is basically
> > + * the start of the environment, minus the start of the U-Boot binary in
> > + * the MMC. This makes the assumption that the MMC is using 512-bytes
> > + * blocks, but devices using something other than that remains to be
> > + * seen.
> > + */
> > +#define UBOOT_MMC_MAX_SIZE	(CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512))
> > +
> >  / {
> >  	binman {
> >  		filename = "u-boot-sunxi-with-spl.bin";
> > @@ -8,6 +17,9 @@
> >  			filename = "spl/sunxi-spl.bin";
> >  		};
> >  		u-boot-img {
> > +#ifdef CONFIG_MMC
> > +			size = <UBOOT_MMC_MAX_SIZE>;
> > +#endif
> >  			pos = <CONFIG_SPL_PAD_TO>;
> >  		};
> >  	};
> > --   
> 
> This is broken in (at least) two ways:
> 
> 1. It is simply nonsensical if u-boot and env are on different devices
>    or not on mmc even if mmc support is enabled.
> 
> 2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset.
>    At best, this is useless.  If the env doesn't immediately follow
>    u-boot, it really breaks things.
> 
> Please fix or revert, I don't really care which.

The padding is not useless. It reserves space for future size expansions
and makes it harder for the users to hurt themselves.

For example, if the padded U-Boot size is 1024K while the actual size
is only ~800K, then adventurous users might be tempted to fit some of
their data into this gap. Yay, ~200K of storage space for free! Except
that the next U-Boot release may grow up to 900K without any warning
and if the users are not careful enough, then their data would be
corrupted during upgrade.

Could you please tell us what is your problem and why reverting this
patch would fix it?

-- 
Best regards,
Siarhei Siamashka