[PATCH 1/2] env: let setenv() take printf arguments

[PATCH 1/2] env: let setenv() take printf arguments

[Sascha Hauer]

It's a common pattern to (ba)sprintf to a string and then call setenv()
with this string. Let setenv() take printf arguments to make that
easier.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/env.c          | 10 +++++++++-
 include/environment.h |  5 +++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/common/env.c b/common/env.c
index 05add63f62..d69c86feab 100644
--- a/common/env.c
+++ b/common/env.c
@@ -251,11 +251,18 @@ static int dev_setenv(const char *name, const char *val)
  * Use unsetenv() to unset.
  */
 
-int setenv(const char *_name, const char *value)
+int setenv(const char *_name, const char *fmt, ...)
 {
+	va_list ap;
 	char *name = strdup(_name);
 	int ret = 0;
 	struct list_head *list;
+	char *value;
+	int len;
+
+	va_start(ap, fmt);
+	len = vasprintf(&value, fmt, ap);
+	va_end(ap);
 
 	if (strchr(name, '.')) {
 		ret = dev_setenv(name, value);
@@ -271,6 +278,7 @@ int setenv(const char *_name, const char *value)
 
 	ret = setenv_raw(list, name, value);
 out:
+	free(value);
 	free(name);
 
 	return ret;
diff --git a/include/environment.h b/include/environment.h
index 19e522cfb6..9e1cb5a929 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -31,7 +31,7 @@ char *var_name(struct variable_d *);
 
 #ifdef CONFIG_ENVIRONMENT_VARIABLES
 const char *getenv(const char *);
-int setenv(const char *, const char *);
+int setenv(const char *, const char *fmt, ...)  __attribute__ ((format(__printf__, 2, 3)));
 void export_env_ull(const char *name, unsigned long long val);
 int getenv_ull(const char *name, unsigned long long *val);
 int getenv_ul(const char *name, unsigned long *val);
@@ -44,7 +44,8 @@ static inline char *getenv(const char *var)
 	return NULL;
 }
 
-static inline int setenv(const char *var, const char *val)
+static inline __attribute__ ((format(__printf__, 2, 3))) int setenv(
+	const char *var, const char *fmt, ...)
 {
 	return 0;
 }
-- 
2.30.2

[PATCH 2/2] treewide: Simplify setenv() calls

[Sascha Hauer]

setenv() now takes printf arguments, use this where possible to simplify
the code a bit.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 commands/clk.c      | 10 +++-------
 commands/crc.c      | 14 ++++----------
 commands/hwclock.c  |  4 +---
 commands/loadb.c    |  4 +---
 commands/loads.c    |  4 +---
 common/bootsource.c |  8 ++------
 common/menutree.c   |  9 +--------
 7 files changed, 13 insertions(+), 40 deletions(-)

diff --git a/commands/clk.c b/commands/clk.c
index dfbc7c988f..7ff6679dad 100644
--- a/commands/clk.c
+++ b/commands/clk.c
@@ -139,13 +139,9 @@ static int do_clk_get_rate(int argc, char *argv[])
 
 	rate = clk_get_rate(clk);
 
-	if (variable_name) {
-		char *t;
-
-		t = basprintf("%lu", rate);
-		setenv(variable_name, t);
-		free(t);
-	} else
+	if (variable_name)
+		setenv(variable_name, "%lu", rate);
+	else
 		printf("%lu\n", rate);
 
 	return COMMAND_SUCCESS;
diff --git a/commands/crc.c b/commands/crc.c
index 80ecf7fe29..3a9f6db741 100644
--- a/commands/crc.c
+++ b/commands/crc.c
@@ -83,17 +83,11 @@ static int do_crc(int argc, char *argv[])
 	printf("CRC32 for %s 0x%08lx ... 0x%08lx ==> 0x%08lx",
 			filename, (ulong)start, (ulong)start + total - 1, crc);
 
-	if (crcvarname) {
-		char *crcstr = basprintf("0x%lx", crc);
-		setenv(crcvarname, crcstr);
-		kfree(crcstr);
-	}
+	if (crcvarname)
+		setenv(crcvarname, "0x%lx", crc);
 
-	if (sizevarname) {
-		char *sizestr = basprintf("0x%lx", total);
-		setenv(sizevarname, sizestr);
-		kfree(sizestr);
-	}
+	if (sizevarname)
+		setenv(sizevarname, "0x%lx", total);
 
 #ifdef CONFIG_CMD_CRC_CMP
 	if (vfilename) {
diff --git a/commands/hwclock.c b/commands/hwclock.c
index abb0500e6a..b3cd7cb8ed 100644
--- a/commands/hwclock.c
+++ b/commands/hwclock.c
@@ -153,11 +153,9 @@ static int do_hwclock(int argc, char *argv[])
 
 	if (env_name) {
 		unsigned long time;
-		char t[12];
 
 		rtc_tm_to_time(&tm, &time);
-		snprintf(t, 12, "%lu", time);
-		setenv(env_name, t);
+		setenv(env_name, "%lu", time);
 	} else {
 		printf("%s\n", time_str(&tm));
 	}
diff --git a/commands/loadb.c b/commands/loadb.c
index 17d3af84b5..5c486d4d73 100644
--- a/commands/loadb.c
+++ b/commands/loadb.c
@@ -542,7 +542,6 @@ packet_error:
 static ulong load_serial_bin(void)
 {
 	int size, i;
-	char buf[32];
 
 	/* Try to allocate the buffer we shall write to files */
 	write_buffer = malloc(MAX_WRITE_BUFFER);
@@ -576,8 +575,7 @@ static ulong load_serial_bin(void)
 		write_idx = 0;
 	}
 	printf("## Total Size      = 0x%08x = %d Bytes\n", size, size);
-	sprintf(buf, "%X", size);
-	setenv("filesize", buf);
+	setenv("filesize", "%X", size);
 
 err_quit:
 	free(write_buffer);
diff --git a/commands/loads.c b/commands/loads.c
index 8260673c51..129bcaba25 100644
--- a/commands/loads.c
+++ b/commands/loads.c
@@ -65,7 +65,6 @@ static ulong load_serial(ulong offset)
 	int	type;				/* return code for record type	*/
 	ulong	addr;				/* load address from S-Record	*/
 	ulong	size;				/* number of bytes transferred	*/
-	char	buf[32];
 	ulong	store_addr;
 	ulong	start_addr = ~0;
 	ulong	end_addr   =  0;
@@ -100,8 +99,7 @@ static ulong load_serial(ulong offset)
 			    "## Total Size      = 0x%08lX = %ld Bytes\n",
 			    start_addr, end_addr, size, size
 			    );
-			sprintf(buf, "%lX", size);
-			setenv("filesize", buf);
+			setenv("filesize", "%lX", size);
 			return addr;
 		case SREC_START:
 			break;
diff --git a/common/bootsource.c b/common/bootsource.c
index 1f8d053a81..11e39db92a 100644
--- a/common/bootsource.c
+++ b/common/bootsource.c
@@ -113,16 +113,12 @@ void bootsource_set(enum bootsource src)
 
 void bootsource_set_instance(int instance)
 {
-	char buf[32];
-
 	bootsource_instance = instance;
 
 	if (instance < 0)
-		sprintf(buf, "unknown");
+		setenv("bootsource_instance","unknown");
 	else
-		snprintf(buf, sizeof(buf), "%d", instance);
-
-	setenv("bootsource_instance", buf);
+		setenv("bootsource_instance", "%d", instance);
 }
 
 enum bootsource bootsource_get(void)
diff --git a/common/menutree.c b/common/menutree.c
index 7fa835a7fe..44d6a7b72c 100644
--- a/common/menutree.c
+++ b/common/menutree.c
@@ -34,14 +34,7 @@ static void menutree_action(struct menu *m, struct menu_entry *me)
 
 static void setenv_bool(const char *var, bool val)
 {
-	const char *str;
-
-	if (val)
-		str = "1";
-	else
-		str = "0";
-
-	setenv(var, str);
+	setenv(var, "%d", val);
 }
 
 static void menutree_box(struct menu *m, struct menu_entry *me)
-- 
2.30.2

Re: [PATCH 2/2] treewide: Simplify setenv() calls

[Daniel Brát]

Since this patch, I am getting a bunch of
'warning: format not a string literal and no format arguments [-Wformat-security]'
warnings when compiling for aarch64 rpi. I am using 'aarch64-linux-gnu-gcc 7.5.0'
on Ubuntu 18.04. Full compmpile log: https://pastebin.com/iCsBJbXU

[PATCH] env: let setenv() take printf arguments

[Ahmad Fatoum]

From: Sascha Hauer <s.hauer@pengutronix.de>

It's a common pattern to (ba)sprintf to a string and then call setenv()
with this string. Let setenv() take printf arguments to make that
easier. To avoid the overhead that goes with changing other callers
to using setenv(var, "%s", val) to avoid security implications (and
GCC warnings), fallback to the non-formatted version when there are
only two arguments.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
[afa: fall back to non-formatted version on old two arg version]
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
Thoughts?
---
 common/env.c           | 37 +++++++++++++++++++++++++++++++++----
 include/environment.h  | 19 +++++++++++++++++--
 include/linux/kernel.h | 12 ++++++++++++
 3 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/common/env.c b/common/env.c
index 05add63f625c..c36f6846ee21 100644
--- a/common/env.c
+++ b/common/env.c
@@ -243,15 +243,15 @@ static int dev_setenv(const char *name, const char *val)
 }
 
 /**
- * setenv - set environment variables
+ * __setenv_str - set environment variables
  * @_name - Variable name
  * @value - the value to set, empty string not handled specially
  *
  * Returns 0 for success and a negative error code otherwise
- * Use unsetenv() to unset.
+ * Use unsetenv() to unset. Don't use directly, use setenv()
  */
 
-int setenv(const char *_name, const char *value)
+int __setenv_str(const char *_name, const char *value)
 {
 	char *name = strdup(_name);
 	int ret = 0;
@@ -275,7 +275,36 @@ out:
 
 	return ret;
 }
-EXPORT_SYMBOL(setenv);
+EXPORT_SYMBOL(__setenv_str);
+
+/**
+ * __setenv_fmt - set environment variables
+ * @name - Variable name
+ * @fmt - format string describing how to format arguments to come
+ *
+ * Returns 0 for success and a negative error code otherwise
+ * Use unsetenv() to unset. Don't use directly, use setenv()
+ */
+
+int __setenv_fmt(const char *name, const char *fmt, ...)
+{
+	va_list ap;
+	int ret;
+	char *value;
+
+	va_start(ap, fmt);
+	ret = vasprintf(&value, fmt, ap);
+	va_end(ap);
+
+	if (ret < 0)
+		return ret;
+
+	ret = __setenv_str(name, value);
+
+	free(value);
+	return ret;
+}
+EXPORT_SYMBOL(__setenv_fmt);
 
 int export(const char *varname)
 {
diff --git a/include/environment.h b/include/environment.h
index 19e522cfb6b4..e5b9a9da3167 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -7,6 +7,7 @@
 #ifndef _ENVIRONMENT_H_
 #define _ENVIRONMENT_H_
 
+#include <linux/kernel.h>
 #include <linux/list.h>
 #include <errno.h>
 
@@ -31,7 +32,8 @@ char *var_name(struct variable_d *);
 
 #ifdef CONFIG_ENVIRONMENT_VARIABLES
 const char *getenv(const char *);
-int setenv(const char *, const char *);
+int __setenv_str(const char *, const char *val);
+int __setenv_fmt(const char *, const char *fmt, ...) __printf(2, 3);
 void export_env_ull(const char *name, unsigned long long val);
 int getenv_ull(const char *name, unsigned long long *val);
 int getenv_ul(const char *name, unsigned long *val);
@@ -44,7 +46,13 @@ static inline char *getenv(const char *var)
 	return NULL;
 }
 
-static inline int setenv(const char *var, const char *val)
+static inline int __setenv_str(const char *var, const char *val)
+{
+	return 0;
+}
+
+static inline __printf(2, 3) int __setenv_fmt(
+	const char *var, const char *fmt, ...)
 {
 	return 0;
 }
@@ -82,6 +90,13 @@ static inline const char *getenv_nonempty(const char *var)
 }
 #endif
 
+/*
+ * avoid the varargs overhead when using a fixed string
+ */
+#undef setenv
+#define setenv(args...) \
+	__optionally_variadic2(__setenv_str, __setenv_fmt, args)
+
 int env_pop_context(void);
 int env_push_context(void);
 
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 4483d33e65bb..ebae8f666cf6 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -7,6 +7,7 @@
 #include <linux/barebox-wrapper.h>
 #include <linux/limits.h>
 #include <linux/math64.h>
+#include <linux/stringify.h>
 
 #define ALIGN(x, a)		__ALIGN_MASK(x, (typeof(x))(a) - 1)
 #define ALIGN_DOWN(x, a)	ALIGN((x) - ((a) - 1), (a))
@@ -17,6 +18,17 @@
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
 #define ARRAY_AND_SIZE(x)	(x), ARRAY_SIZE(x)
 
+/*
+ * Call func_variadic, when more than 2 arguments and func_fixed otherwise
+ */
+#define __optionally_variadic2(func_fixed, func_variadic, arg1, arg2, ...) ({ \
+		char _______STR[] = __stringify((__VA_ARGS__));  \
+		sizeof(_______STR) > 3 ?                         \
+			func_variadic(arg1, arg2, ##__VA_ARGS__) \
+		:                                                \
+			func_fixed(arg1, arg2);                  \
+	})
+
 /*
  * This looks more complex than it should be. But we need to
  * get the type for the ~ right in round_down (it needs to be
-- 
2.30.2

Re: [PATCH] env: let setenv() take printf arguments

[Sascha Hauer]

On Mon, Jun 20, 2022 at 09:21:39AM +0200, Ahmad Fatoum wrote:
> From: Sascha Hauer <s.hauer@pengutronix.de>
> 
> It's a common pattern to (ba)sprintf to a string and then call setenv()
> with this string. Let setenv() take printf arguments to make that
> easier. To avoid the overhead that goes with changing other callers
> to using setenv(var, "%s", val) to avoid security implications (and
> GCC warnings), fallback to the non-formatted version when there are
> only two arguments.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> [afa: fall back to non-formatted version on old two arg version]
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> Thoughts?

While I'm impressed by this macro I don't like this very much. My desire
was to simplify things, now with this patch I'm no longer sure I reached
that goal.

Alternatively we could

a) Drop the original patch
b) Replace the problematic places with setenv(foo, "%s", not_a_string_literal);
c) Pass -Wno-format-security, The Kernel does this for over a decade.

My vote is c)

Sascha

> ---
>  common/env.c           | 37 +++++++++++++++++++++++++++++++++----
>  include/environment.h  | 19 +++++++++++++++++--
>  include/linux/kernel.h | 12 ++++++++++++
>  3 files changed, 62 insertions(+), 6 deletions(-)
> 
> diff --git a/common/env.c b/common/env.c
> index 05add63f625c..c36f6846ee21 100644
> --- a/common/env.c
> +++ b/common/env.c
> @@ -243,15 +243,15 @@ static int dev_setenv(const char *name, const char *val)
>  }
>  
>  /**
> - * setenv - set environment variables
> + * __setenv_str - set environment variables
>   * @_name - Variable name
>   * @value - the value to set, empty string not handled specially
>   *
>   * Returns 0 for success and a negative error code otherwise
> - * Use unsetenv() to unset.
> + * Use unsetenv() to unset. Don't use directly, use setenv()
>   */
>  
> -int setenv(const char *_name, const char *value)
> +int __setenv_str(const char *_name, const char *value)
>  {
>  	char *name = strdup(_name);
>  	int ret = 0;
> @@ -275,7 +275,36 @@ out:
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL(setenv);
> +EXPORT_SYMBOL(__setenv_str);
> +
> +/**
> + * __setenv_fmt - set environment variables
> + * @name - Variable name
> + * @fmt - format string describing how to format arguments to come
> + *
> + * Returns 0 for success and a negative error code otherwise
> + * Use unsetenv() to unset. Don't use directly, use setenv()
> + */
> +
> +int __setenv_fmt(const char *name, const char *fmt, ...)
> +{
> +	va_list ap;
> +	int ret;
> +	char *value;
> +
> +	va_start(ap, fmt);
> +	ret = vasprintf(&value, fmt, ap);
> +	va_end(ap);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = __setenv_str(name, value);
> +
> +	free(value);
> +	return ret;
> +}
> +EXPORT_SYMBOL(__setenv_fmt);
>  
>  int export(const char *varname)
>  {
> diff --git a/include/environment.h b/include/environment.h
> index 19e522cfb6b4..e5b9a9da3167 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -7,6 +7,7 @@
>  #ifndef _ENVIRONMENT_H_
>  #define _ENVIRONMENT_H_
>  
> +#include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <errno.h>
>  
> @@ -31,7 +32,8 @@ char *var_name(struct variable_d *);
>  
>  #ifdef CONFIG_ENVIRONMENT_VARIABLES
>  const char *getenv(const char *);
> -int setenv(const char *, const char *);
> +int __setenv_str(const char *, const char *val);
> +int __setenv_fmt(const char *, const char *fmt, ...) __printf(2, 3);
>  void export_env_ull(const char *name, unsigned long long val);
>  int getenv_ull(const char *name, unsigned long long *val);
>  int getenv_ul(const char *name, unsigned long *val);
> @@ -44,7 +46,13 @@ static inline char *getenv(const char *var)
>  	return NULL;
>  }
>  
> -static inline int setenv(const char *var, const char *val)
> +static inline int __setenv_str(const char *var, const char *val)
> +{
> +	return 0;
> +}
> +
> +static inline __printf(2, 3) int __setenv_fmt(
> +	const char *var, const char *fmt, ...)
>  {
>  	return 0;
>  }
> @@ -82,6 +90,13 @@ static inline const char *getenv_nonempty(const char *var)
>  }
>  #endif
>  
> +/*
> + * avoid the varargs overhead when using a fixed string
> + */
> +#undef setenv
> +#define setenv(args...) \
> +	__optionally_variadic2(__setenv_str, __setenv_fmt, args)
> +
>  int env_pop_context(void);
>  int env_push_context(void);
>  
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 4483d33e65bb..ebae8f666cf6 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -7,6 +7,7 @@
>  #include <linux/barebox-wrapper.h>
>  #include <linux/limits.h>
>  #include <linux/math64.h>
> +#include <linux/stringify.h>
>  
>  #define ALIGN(x, a)		__ALIGN_MASK(x, (typeof(x))(a) - 1)
>  #define ALIGN_DOWN(x, a)	ALIGN((x) - ((a) - 1), (a))
> @@ -17,6 +18,17 @@
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>  #define ARRAY_AND_SIZE(x)	(x), ARRAY_SIZE(x)
>  
> +/*
> + * Call func_variadic, when more than 2 arguments and func_fixed otherwise
> + */
> +#define __optionally_variadic2(func_fixed, func_variadic, arg1, arg2, ...) ({ \
> +		char _______STR[] = __stringify((__VA_ARGS__));  \
> +		sizeof(_______STR) > 3 ?                         \
> +			func_variadic(arg1, arg2, ##__VA_ARGS__) \
> +		:                                                \
> +			func_fixed(arg1, arg2);                  \
> +	})
> +
>  /*
>   * This looks more complex than it should be. But we need to
>   * get the type for the ~ right in round_down (it needs to be
> -- 
> 2.30.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] env: let setenv() take printf arguments

[Ahmad Fatoum]

Hello Sascha,

On 20.06.22 09:47, Sascha Hauer wrote:
> On Mon, Jun 20, 2022 at 09:21:39AM +0200, Ahmad Fatoum wrote:
>> From: Sascha Hauer <s.hauer@pengutronix.de>
>>
>> It's a common pattern to (ba)sprintf to a string and then call setenv()
>> with this string. Let setenv() take printf arguments to make that
>> easier. To avoid the overhead that goes with changing other callers
>> to using setenv(var, "%s", val) to avoid security implications (and
>> GCC warnings), fallback to the non-formatted version when there are
>> only two arguments.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> [afa: fall back to non-formatted version on old two arg version]
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> Thoughts?
> 
> While I'm impressed by this macro I don't like this very much. My desire
> was to simplify things, now with this patch I'm no longer sure I reached
> that goal.

Usage _is_ simpler. Declaration indeed looks a bit odd, but ¯\_(ツ)_/¯

> 
> Alternatively we could
> 
> a) Drop the original patch
> b) Replace the problematic places with setenv(foo, "%s", not_a_string_literal);
> c) Pass -Wno-format-security, The Kernel does this for over a decade.

Then it probably needs to be revisited there then.

> My vote is c)

I am not fine with c). We don't sanitize for % in environment variable values
and ignoring the warning has very clear security implications.

Cheers,
Ahmad

> 
> Sascha
> 
>> ---
>>  common/env.c           | 37 +++++++++++++++++++++++++++++++++----
>>  include/environment.h  | 19 +++++++++++++++++--
>>  include/linux/kernel.h | 12 ++++++++++++
>>  3 files changed, 62 insertions(+), 6 deletions(-)
>>
>> diff --git a/common/env.c b/common/env.c
>> index 05add63f625c..c36f6846ee21 100644
>> --- a/common/env.c
>> +++ b/common/env.c
>> @@ -243,15 +243,15 @@ static int dev_setenv(const char *name, const char *val)
>>  }
>>  
>>  /**
>> - * setenv - set environment variables
>> + * __setenv_str - set environment variables
>>   * @_name - Variable name
>>   * @value - the value to set, empty string not handled specially
>>   *
>>   * Returns 0 for success and a negative error code otherwise
>> - * Use unsetenv() to unset.
>> + * Use unsetenv() to unset. Don't use directly, use setenv()
>>   */
>>  
>> -int setenv(const char *_name, const char *value)
>> +int __setenv_str(const char *_name, const char *value)
>>  {
>>  	char *name = strdup(_name);
>>  	int ret = 0;
>> @@ -275,7 +275,36 @@ out:
>>  
>>  	return ret;
>>  }
>> -EXPORT_SYMBOL(setenv);
>> +EXPORT_SYMBOL(__setenv_str);
>> +
>> +/**
>> + * __setenv_fmt - set environment variables
>> + * @name - Variable name
>> + * @fmt - format string describing how to format arguments to come
>> + *
>> + * Returns 0 for success and a negative error code otherwise
>> + * Use unsetenv() to unset. Don't use directly, use setenv()
>> + */
>> +
>> +int __setenv_fmt(const char *name, const char *fmt, ...)
>> +{
>> +	va_list ap;
>> +	int ret;
>> +	char *value;
>> +
>> +	va_start(ap, fmt);
>> +	ret = vasprintf(&value, fmt, ap);
>> +	va_end(ap);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = __setenv_str(name, value);
>> +
>> +	free(value);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(__setenv_fmt);
>>  
>>  int export(const char *varname)
>>  {
>> diff --git a/include/environment.h b/include/environment.h
>> index 19e522cfb6b4..e5b9a9da3167 100644
>> --- a/include/environment.h
>> +++ b/include/environment.h
>> @@ -7,6 +7,7 @@
>>  #ifndef _ENVIRONMENT_H_
>>  #define _ENVIRONMENT_H_
>>  
>> +#include <linux/kernel.h>
>>  #include <linux/list.h>
>>  #include <errno.h>
>>  
>> @@ -31,7 +32,8 @@ char *var_name(struct variable_d *);
>>  
>>  #ifdef CONFIG_ENVIRONMENT_VARIABLES
>>  const char *getenv(const char *);
>> -int setenv(const char *, const char *);
>> +int __setenv_str(const char *, const char *val);
>> +int __setenv_fmt(const char *, const char *fmt, ...) __printf(2, 3);
>>  void export_env_ull(const char *name, unsigned long long val);
>>  int getenv_ull(const char *name, unsigned long long *val);
>>  int getenv_ul(const char *name, unsigned long *val);
>> @@ -44,7 +46,13 @@ static inline char *getenv(const char *var)
>>  	return NULL;
>>  }
>>  
>> -static inline int setenv(const char *var, const char *val)
>> +static inline int __setenv_str(const char *var, const char *val)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline __printf(2, 3) int __setenv_fmt(
>> +	const char *var, const char *fmt, ...)
>>  {
>>  	return 0;
>>  }
>> @@ -82,6 +90,13 @@ static inline const char *getenv_nonempty(const char *var)
>>  }
>>  #endif
>>  
>> +/*
>> + * avoid the varargs overhead when using a fixed string
>> + */
>> +#undef setenv
>> +#define setenv(args...) \
>> +	__optionally_variadic2(__setenv_str, __setenv_fmt, args)
>> +
>>  int env_pop_context(void);
>>  int env_push_context(void);
>>  
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index 4483d33e65bb..ebae8f666cf6 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -7,6 +7,7 @@
>>  #include <linux/barebox-wrapper.h>
>>  #include <linux/limits.h>
>>  #include <linux/math64.h>
>> +#include <linux/stringify.h>
>>  
>>  #define ALIGN(x, a)		__ALIGN_MASK(x, (typeof(x))(a) - 1)
>>  #define ALIGN_DOWN(x, a)	ALIGN((x) - ((a) - 1), (a))
>> @@ -17,6 +18,17 @@
>>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>>  #define ARRAY_AND_SIZE(x)	(x), ARRAY_SIZE(x)
>>  
>> +/*
>> + * Call func_variadic, when more than 2 arguments and func_fixed otherwise
>> + */
>> +#define __optionally_variadic2(func_fixed, func_variadic, arg1, arg2, ...) ({ \
>> +		char _______STR[] = __stringify((__VA_ARGS__));  \
>> +		sizeof(_______STR) > 3 ?                         \
>> +			func_variadic(arg1, arg2, ##__VA_ARGS__) \
>> +		:                                                \
>> +			func_fixed(arg1, arg2);                  \
>> +	})
>> +
>>  /*
>>   * This looks more complex than it should be. But we need to
>>   * get the type for the ~ right in round_down (it needs to be
>> -- 
>> 2.30.2
>>
>>
>>
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] env: let setenv() take printf arguments

[Sascha Hauer]

On Mon, Jun 20, 2022 at 09:59:00AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 20.06.22 09:47, Sascha Hauer wrote:
> > On Mon, Jun 20, 2022 at 09:21:39AM +0200, Ahmad Fatoum wrote:
> >> From: Sascha Hauer <s.hauer@pengutronix.de>
> >>
> >> It's a common pattern to (ba)sprintf to a string and then call setenv()
> >> with this string. Let setenv() take printf arguments to make that
> >> easier. To avoid the overhead that goes with changing other callers
> >> to using setenv(var, "%s", val) to avoid security implications (and
> >> GCC warnings), fallback to the non-formatted version when there are
> >> only two arguments.
> >>
> >> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> >> [afa: fall back to non-formatted version on old two arg version]
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >> Thoughts?
> > 
> > While I'm impressed by this macro I don't like this very much. My desire
> > was to simplify things, now with this patch I'm no longer sure I reached
> > that goal.
> 
> Usage _is_ simpler. Declaration indeed looks a bit odd, but ¯\_(ツ)_/¯
> 
> > 
> > Alternatively we could
> > 
> > a) Drop the original patch
> > b) Replace the problematic places with setenv(foo, "%s", not_a_string_literal);
> > c) Pass -Wno-format-security, The Kernel does this for over a decade.
> 
> Then it probably needs to be revisited there then.
> 
> > My vote is c)
> 
> I am not fine with c). We don't sanitize for % in environment variable values
> and ignoring the warning has very clear security implications.

Ok, good point.

Then there's of course

d) keep setenv like it was before and introduce
   pr_setenv(const char *_name, const char *fmt, ...)

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |