From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= Date: Tue, 08 May 2018 02:08:10 +0100 Subject: [U-Boot] [PATCH v3 2/2] sunxi: binman: Add U-Boot binary size check In-Reply-To: <20180507204658.vc6gi6vxjjl5ft6t@flea> (Maxime Ripard's message of "Mon, 7 May 2018 22:46:58 +0200") References: <20171020121614.9863-1-maxime.ripard@free-electrons.com> <20171020121614.9863-3-maxime.ripard@free-electrons.com> <20180501234804.715cac38@i7> <20180502134155.44qzb6uebpbaubkv@flea> <20180507204658.vc6gi6vxjjl5ft6t@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: > On Wed, May 02, 2018 at 03:24:50PM +0100, Måns Rullgård wrote: >> 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. > > 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