[U-Boot] [RFC PATCH] fastboot: Implement fetching uboot env variables via fastboot getvar

[U-Boot] [RFC PATCH] fastboot: Implement fetching uboot env variables via fastboot getvar

[Priit Laes]

From: Priit Laes <priit.laes@paf.com>

Add u-boot specific getvar "extension" to fetch u-boot environment
variables via `fastboot getvar uboot:var`. This makes it possible
to gather certain data (like mac addresses) in an automated way
during initial fastboot flashing for inventory purposes:

$ fastboot getvar uboot:ethaddr
uboot:ethaddr: 12:23:45:56:78:90

Output is currently truncated at 64 bytes, but this is good enough
for my own requirements.

Signed-off-by: Priit Laes <priit.laes@paf.com>
---
 drivers/fastboot/fb_getvar.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
index 4d264c985d..37bb8c7220 100644
--- a/drivers/fastboot/fb_getvar.c
+++ b/drivers/fastboot/fb_getvar.c
@@ -20,6 +20,7 @@ static void getvar_product(char *var_parameter, char *response);
 static void getvar_current_slot(char *var_parameter, char *response);
 static void getvar_slot_suffixes(char *var_parameter, char *response);
 static void getvar_has_slot(char *var_parameter, char *response);
+static void getvar_ubootenv(char *var_parameter, char *response);
 #if CONFIG_IS_ENABLED(FASTBOOT_FLASH_MMC)
 static void getvar_partition_type(char *part_name, char *response);
 #endif
@@ -64,6 +65,9 @@ static const struct {
 	}, {
 		.variable = "has_slot",
 		.dispatch = getvar_has_slot
+	}, {
+		.variable = "uboot",
+		.dispatch = getvar_ubootenv
 #if CONFIG_IS_ENABLED(FASTBOOT_FLASH_MMC)
 	}, {
 		.variable = "partition-type",
@@ -82,6 +86,16 @@ static void getvar_version(char *var_parameter, char *response)
 	fastboot_okay(FASTBOOT_VERSION, response);
 }
 
+static void getvar_ubootenv(char *var_parameter, char *response)
+{
+	const char *value = env_get(var_parameter);
+
+	if (value)
+		fastboot_okay(value, response);
+	else
+		fastboot_fail("Variable is not set", response);
+}
+
 static void getvar_bootloader_version(char *var_parameter, char *response)
 {
 	fastboot_okay(U_BOOT_VERSION, response);
-- 
2.11.0

[U-Boot] [RFC PATCH] fastboot: Implement fetching uboot env variables via fastboot getvar

[Sam Protsenko]

 Hi Priit,

On Wed, Jul 10, 2019 at 11:42 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Priit,
>
> On Tue, Jul 09, 2019 at 02:52:56PM +0300, Priit Laes wrote:
> > From: Priit Laes <priit.laes@paf.com>
> >
> > Add u-boot specific getvar "extension" to fetch u-boot environment
> > variables via `fastboot getvar uboot:var`. This makes it possible
> > to gather certain data (like mac addresses) in an automated way
> > during initial fastboot flashing for inventory purposes:
> >
> > $ fastboot getvar uboot:ethaddr
> > uboot:ethaddr: 12:23:45:56:78:90
> >
> > Output is currently truncated at 64 bytes, but this is good enough
> > for my own requirements.
> >
> > Signed-off-by: Priit Laes <priit.laes@paf.com>
> > ---
> >  drivers/fastboot/fb_getvar.c | 14 ++++++++++++++
>
> [..]
>
> > +static void getvar_ubootenv(char *var_parameter, char *response)
> > +{
> > +     const char *value = env_get(var_parameter);
>
> This would bring a lot of flexibility to the users. My only concern is
> that it exposes to the outside world an internal U-Boot API (env_get)
> which might have weaknesses (or might acquire them in time). I am not
> sure how env_get() behaves in below corner cases:
>  - NULL pointer
>  - empty string
>  - hugely long string
>
> IMHO the internal APIs are usually not designed to sustain high level of
> stress/fuzziness in their input parameters. Once made available to the
> users of fastboot tool, this will open room for more experiments to the
> CVE seekers.
>
> Another observation is that this patch will contribute with a deviation
> from the vanilla fastboot protocol (which might be fine).
>
> Since there are already multiple getvar functions which fetch the
> information from the U-Boot environment, I wonder if all of these could
> be centralized in a table like below:
>
>  {
>         /* fastboot    U-Boot */
>         { "serialno", "serial#" },
>         { "product",  "board" },
>         { "platform", "platform" },
>         { "ethaddr",  "ethaddr" },
>         { "anything", "anything" },
>  }
>
> The upsides of this approach:
>  - Unification, readability, decreased code size
> The downsides:
>  - Inflexible for getting arbitrary environment variables
>

I agree with Eugeniu on his points about security and deviation from
fastboot spec (can be found in AOSP). Instead of exposing the whole
U-Boot environment, I can suggest you to expose only actually needed
variables for your platform. In fact, we already have a mechanism
exactly for that, called "variable overrides". It's documented in [1].
Basically you just need to add "fastboot.xxx" variables to your
environment (can be don e.g. in your board_late_init() function), and
you'll be able to obtain those via "fastboot getvar xxx". You can see
an example in patches [2,3].

[1] https://gitlab.denx.de/u-boot/u-boot/blob/v2019.07/doc/README.android-fastboot#L90
[2] https://gitlab.denx.de/u-boot/u-boot/commit/fa24eca1f20a037d2dcbd1eae7ac8b2ecb1b0423
[3] https://gitlab.denx.de/u-boot/u-boot/commit/8bd29623b5223e880e7be475243a2bdb987aba38

> --
> Best Regards,
> Eugeniu.

[U-Boot] [RFC PATCH] fastboot: Implement fetching uboot env variables via fastboot getvar

[Eugeniu Rosca]

Hi Priit,

On Tue, Jul 09, 2019 at 02:52:56PM +0300, Priit Laes wrote:
> From: Priit Laes <priit.laes@paf.com>
> 
> Add u-boot specific getvar "extension" to fetch u-boot environment
> variables via `fastboot getvar uboot:var`. This makes it possible
> to gather certain data (like mac addresses) in an automated way
> during initial fastboot flashing for inventory purposes:
> 
> $ fastboot getvar uboot:ethaddr
> uboot:ethaddr: 12:23:45:56:78:90
> 
> Output is currently truncated at 64 bytes, but this is good enough
> for my own requirements.
> 
> Signed-off-by: Priit Laes <priit.laes@paf.com>
> ---
>  drivers/fastboot/fb_getvar.c | 14 ++++++++++++++

[..]

> +static void getvar_ubootenv(char *var_parameter, char *response)
> +{
> +	const char *value = env_get(var_parameter);

This would bring a lot of flexibility to the users. My only concern is
that it exposes to the outside world an internal U-Boot API (env_get)
which might have weaknesses (or might acquire them in time). I am not
sure how env_get() behaves in below corner cases:
 - NULL pointer
 - empty string
 - hugely long string

IMHO the internal APIs are usually not designed to sustain high level of
stress/fuzziness in their input parameters. Once made available to the
users of fastboot tool, this will open room for more experiments to the
CVE seekers.

Another observation is that this patch will contribute with a deviation
from the vanilla fastboot protocol (which might be fine).

Since there are already multiple getvar functions which fetch the
information from the U-Boot environment, I wonder if all of these could
be centralized in a table like below:

 {
 	/* fastboot    U-Boot */
 	{ "serialno", "serial#" },
 	{ "product",  "board" },
 	{ "platform", "platform" },
 	{ "ethaddr",  "ethaddr" },
 	{ "anything", "anything" },
 }

The upsides of this approach:
 - Unification, readability, decreased code size
The downsides:
 - Inflexible for getting arbitrary environment variables

-- 
Best Regards,
Eugeniu.

[U-Boot] [RFC PATCH] fastboot: Implement fetching uboot env variables via fastboot getvar

[Priit Laes]

From: Priit Laes <priit.laes@paf.com>

Add u-boot specific getvar "extension" to fetch u-boot environment
variables via `fastboot getvar uboot:var`. This makes it possible
to gather certain data (like mac addresses) in an automated way
during initial fastboot flashing for inventory purposes:

$ fastboot getvar uboot:ethaddr
uboot:ethaddr: 12:23:45:56:78:90

Output is currently truncated at 64 bytes, but this is good enough
for my own requirements.

Signed-off-by: Priit Laes <priit.laes@paf.com>
---
 drivers/fastboot/fb_getvar.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
index bf957e8326..d78ca0af42 100644
--- a/drivers/fastboot/fb_getvar.c
+++ b/drivers/fastboot/fb_getvar.c
@@ -20,6 +20,7 @@ static void getvar_product(char *var_parameter, char *response);
 static void getvar_platform(char *var_parameter, char *response);
 static void getvar_current_slot(char *var_parameter, char *response);
 static void getvar_slot_suffixes(char *var_parameter, char *response);
+static void getvar_ubootenv(char *var_parameter, char *response);
 #if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
 static void getvar_has_slot(char *var_parameter, char *response);
 #endif
@@ -67,6 +68,9 @@ static const struct {
 	}, {
 		.variable = "slot-suffixes",
 		.dispatch = getvar_slot_suffixes
+	}, {
+		.variable = "uboot",
+		.dispatch = getvar_ubootenv
 #if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
 	}, {
 		.variable = "has-slot",
@@ -131,6 +135,16 @@ static void getvar_version(char *var_parameter, char *response)
 	fastboot_okay(FASTBOOT_VERSION, response);
 }
 
+static void getvar_ubootenv(char *var_parameter, char *response)
+{
+	const char *value = env_get(var_parameter);
+
+	if (value)
+		fastboot_okay(value, response);
+	else
+		fastboot_fail("Variable is not set", response);
+}
+
 static void getvar_bootloader_version(char *var_parameter, char *response)
 {
 	fastboot_okay(U_BOOT_VERSION, response);
-- 
2.11.0