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

[Maxime Ripard <maxime.ripard@bootlin.com>]

1;5201;0c
On Wed, May 02, 2018 at 10:34:49AM +0100, M책ns Rullg책rd wrote:
> Siarhei Siamashka <siarhei.siamashka@gmail.com> writes:
> 
> > On Tue, 01 May 2018 18:25:06 +0100
> > M책ns Rullg책rd <mans@mansr.com> wrote:
> >
> >> Maxime Ripard <maxime.ripard@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@arm.com>
> >> > Signed-off-by: Maxime Ripard <maxime.ripard@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.

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

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

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180502/d376f331/attachment.sig>