From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= Date: Wed, 02 May 2018 15:24:50 +0100 Subject: [U-Boot] [PATCH v3 2/2] sunxi: binman: Add U-Boot binary size check In-Reply-To: <20180502134155.44qzb6uebpbaubkv@flea> (Maxime Ripard's message of "Wed, 2 May 2018 15:41:55 +0200") References: <20171020121614.9863-1-maxime.ripard@free-electrons.com> <20171020121614.9863-3-maxime.ripard@free-electrons.com> <20180501234804.715cac38@i7> <20180502134155.44qzb6uebpbaubkv@flea> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de Maxime Ripard writes: > 1;5201;0c > On Wed, May 02, 2018 at 10:34:49AM +0100, Måns Rullgård wrote: >> Siarhei Siamashka writes: >> >> > On Tue, 01 May 2018 18:25:06 +0100 >> > Måns Rullgård wrote: >> > >> >> Maxime Ripard 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 >> >> > Signed-off-by: Maxime Ripard >> >> > --- >> >> > 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 >> >> > >> >> > +/* >> >> > + * 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 = ; >> >> > +#endif >> >> > pos = ; >> >> > }; >> >> > }; >> >> > -- >> >> >> >> 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