[U-Boot] [PATCH v3 2/2] sunxi: binman: Add U-Boot binary size check
Måns Rullgård
mans at mansr.com
Tue May 8 01:08:10 UTC 2018
Maxime Ripard <maxime.ripard at bootlin.com> writes:
> On Wed, May 02, 2018 at 03:24:50PM +0100, Måns Rullgård wrote:
>> 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.
>
> My point is that this is a slippery slope. Anyway..
Yes, trying to be overly helpful is indeed a slippery slope.
>> > 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.
>
> Ah, right...
>
>> >> 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?
>
> Unfortunately, this isn't a very specific case this is the default we
> shipped for years.
Your default is still just one specific case of nearly infinite
possibilities.
> And we have people relying on it in the wild.
How can anyone *rely* on that padding being there?
> This is why we added that check in the first place, because the size
> of uboot increased so much that it was now overlapping with the
> environment.
I get that. Unfortunately, the check was flawed for all but your
default use case.
> I guess now that we transitioned with the release of 2018.05, we can
> remove that check entirely, since the default is the first partition,
> wherever that is.
I don't care how you justify it as long as you get rid of the broken code.
--
Måns Rullgård
More information about the U-Boot
mailing list