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

[Måns Rullgård]

Maxime Ripard <maxime.ripard at bootlin.com> writes:

> 1;5201;0c
> On Wed, May 02, 2018 at 10:34:49AM +0100, Måns Rullgård wrote:
>> 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?
>
> Yes, and that's never a good argument to make, because...
>
>> > 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.
>
> .. I could make pretty much the same one for all your cases, which
> would be completely useless, and wouldn't fix anything.

Huh?  I'm saying we shouldn't "helpfully" do things that actually break
perfectly valid setups.  U-Boot users are expected to know what they are
doing, and shouldn't need that kind of help.  In my opinion.

> I guess this one could be solved using an ifdef guard with
> ENV_IS_IN_MMC.

Not if env is on a different mmc device.

>> 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.
>
> This one would be covered too.
>
>> 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'm not too sure about how to fix this one though. Any suggestion?

I just don't see the point in trying to pin down the very specific case
of U-Boot and env being on the same device with only a (small) amount of
padding between them.  There are a million other ways for users to screw
up, so why should we be making a half-hearted effort to prevent this
one?

-- 
Måns Rullgård