[U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Alison Chaiken]

On Tue, Jun 27, 2017 at 12:05 AM, Lothar Waßmann <LW at karo-electronics.de> wrote:
>
> Hi,
>
> On Sun, 25 Jun 2017 14:54:56 -0700 Alison Chaiken wrote:
> > On Sun, Jun 18, 2017 at 4:03 AM, Wolfgang Denk <wd at denx.de> wrote:
> >
> > > Dear Alison,
> > >
> > > In message <CAOuSAjdHerD7iWSwv5HQmx07nALRHschnH5=XToNEZDqA9JsvQ at mail.
> > > gmail.com> you wrote:
> > > >
> > > > The idea behind the 'swap' mode is that a storage device can have two
> > > sets
> > > > of partitions, one set all named 'primary' and one set all named
> > > 'backup'.
> > > >   The software updater in userspace can then simply rename the partitions
> > > > with sgdisk in order to pick the new image.   The swap mode changes the
> > > > whole set of labels at once, so there's little chance of being
> > > interrupted.
> > >
> > > It's still a sequential, non-atomic operation, and "little chance"
> > > is exactly the places where Murphy likes to hit you.
> > >
> > > > One additional note: the last version I posted worked fine for the
> > > sandbox,
> > > > but wouldn't link for an ARM target with the Linaro toolchain, as the
> > > > linker couldn't find atoi().   I guess the libc for the x86 compiler
> > > > includes it.   To test on ARM, I copied in simple_atoi() from
> > > > lib/vsprintf.c, but assuredly that is an ugly solution.    Does anyone
> > > have
> > > > a better idea to solve this problem?
> > >
> > > U-Boot should always be self-contained and not link regular library
> > > code from the tool chain.
> > >
> > > Best regards,
> > >
> > > Wolfgang Denk
> > >
> >
> > I'm about to submit a new version of the patches that adopts Wolfgang's and
> > Tom's suggestions about replacing atoi().
> >
> > Regarding the atomicity of 'gpt swap, the point is that 'gpt swap' first
> > modifies the names in an in-memory
> > data structure, and then uses the existing 'gpt write' functionality to
> > change the actual partition table stored on the device.  Thus,
> > interruption of the new command is low-risk, as interruption of the
> > modification of the new data structure has no persistent effect, and
> > the risk associated with 'gpt write' is the same as always.
> >
> > By the way, in the course of testing an earlier version of this patch
> > series, I noticed that 'gpt write' and 'gpt verify' segv if presented with
> > a non-null-terminated partitions string.  It's the strlen function in lib
> > that actually generates an error. I haven't yet quite figured out what the
> > best solution to the problem is: should strlen() itself be modified, or is
> > it enough to test in gpt.c?   The right solution is not to present the
> > commands with poorly formed strings, but it's easy to do so.
> >
> You can use strnlen() if you know the maximum allowed length of the
> string.

Thanks for that tip: I didn't know that strnlen() existed.   That
indeed seems like a good protection against at least the problems
identified here.

>
> NB: A quick glance at set_gpt_info() revealed this potential crash
> cause:
> |       str = strdup(str_part);
> |
> |       /* extract disk guid */
> |       s = str;
> |       val = extract_val(str, "uuid_disk");
> strdup() may fail (especially if the input string is not zero
> terminated) and return a NULL pointer which then will happily be used
> by extract_val().
>
>
>
> Lothar Waßmann

The underlying cause of the problem is that u-boot's implementations
of strlen() and the CLI handle strings differently.   The former
terminates strings only on NULLs, while the latter terminates strings
*either* on NULLs or on semicolons.    Reading the partition table
back from a device's GPT results in a string with a NULL only at the
very end, but with one semicolon per partition.   Running 'gpt verify
mmc 0 $partitions" or 'gpt write mmc 0 $partitions' therefore causes a
crash if the user has previously typed 'setenv partitions 1;2;3;4;5;'
rather than 'setenv partitions "1;2;3;4;5;"'.   In the former case,
the partitions string ends up being '1' without NULL-termination,
resulting in the crash.

U-boot environment parameters often have lots of semi-colon-separated
lists.   I'm not sure what other code paths lead to strlen() or
strdup(), as Lothar alludes to above, but it seems likely to me that
there are other ways to reproduce this bug.   Looking at lib/string.c,
there are other functions like strcpy() that also check "!= '\0'".

--- Alison