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

[Måns Rullgård]

Siarhei Siamashka <siarhei.siamashka at gmail.com> writes:

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

Do U-Boot users really need that level of hand-holding?

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

See above.  Best case, it just wastes space in the created file.  If the
configuration is anything other than U-Boot immediately followed by env
on the same device, it _will_ break things horribly.  A few examples:

1.  U-Boot and env are on different devices, e.g. U-Boot on mmc and env
    in SPI NOR flash.  In this case, padding the file to the env offset
    makes no sense.  Writing the image will corrupt anything stored
    after U-Boot at a smaller (but still safe) offset.

2.  U-Boot at a non-zero offset, followed by env, but _not_ on mmc.  If
    CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, probably at its default
    value, is smaller than the offset of U-Boot in its actual device,
    the padding will be too large.  Writing the image file will then
    corrupt a stored env.

3.  U-Boot at start of device, env at end as indicated by a negative
    CONFIG_ENV_OFFSET.  With this configuration, binman tries to pad the
    image to (nearly) 2^64 bytes and promptly fills up your storage
    device.

I keep running into variants of these, and I'm weary of it.

-- 
Måns Rullgård