Maxime Ripard <maxime.rip...@bootlin.com> writes:
> 1;5201;0c
> On Wed, May 02, 2018 at 10:34:49AM +0100, Måns Rullgård wrote:
>> Siarhei Siamashka <siarhei.siamas...@gmail.com> writes:
>>
>> > On Tue, 01 May 2018 18:25:06 +0100
>> > Måns Rullgård <m...@mansr.com> wrote:
>> >
>> >> Maxime Ripard <maxime.rip...@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.przyw...@arm.com>
>> >> > Signed-off-by: Maxime Ripard <maxime.rip...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
