* [PATCH 1/2] env: let setenv() take printf arguments @ 2022-06-17 8:05 Sascha Hauer 2022-06-17 8:05 ` [PATCH 2/2] treewide: Simplify setenv() calls Sascha Hauer 0 siblings, 1 reply; 7+ messages in thread From: Sascha Hauer @ 2022-06-17 8:05 UTC (permalink / raw) To: Barebox List 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] treewide: Simplify setenv() calls 2022-06-17 8:05 [PATCH 1/2] env: let setenv() take printf arguments Sascha Hauer @ 2022-06-17 8:05 ` Sascha Hauer 2022-06-17 21:53 ` Daniel Brát 0 siblings, 1 reply; 7+ messages in thread From: Sascha Hauer @ 2022-06-17 8:05 UTC (permalink / raw) To: Barebox List 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] treewide: Simplify setenv() calls 2022-06-17 8:05 ` [PATCH 2/2] treewide: Simplify setenv() calls Sascha Hauer @ 2022-06-17 21:53 ` Daniel Brát 2022-06-20 7:21 ` [PATCH] env: let setenv() take printf arguments Ahmad Fatoum 0 siblings, 1 reply; 7+ messages in thread From: Daniel Brát @ 2022-06-17 21:53 UTC (permalink / raw) To: s.hauer; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] env: let setenv() take printf arguments 2022-06-17 21:53 ` Daniel Brát @ 2022-06-20 7:21 ` Ahmad Fatoum 2022-06-20 7:47 ` Sascha Hauer 0 siblings, 1 reply; 7+ messages in thread From: Ahmad Fatoum @ 2022-06-20 7:21 UTC (permalink / raw) To: barebox; +Cc: 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] env: let setenv() take printf arguments 2022-06-20 7:21 ` [PATCH] env: let setenv() take printf arguments Ahmad Fatoum @ 2022-06-20 7:47 ` Sascha Hauer 2022-06-20 7:59 ` Ahmad Fatoum 0 siblings, 1 reply; 7+ messages in thread From: Sascha Hauer @ 2022-06-20 7:47 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox 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 | ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] env: let setenv() take printf arguments 2022-06-20 7:47 ` Sascha Hauer @ 2022-06-20 7:59 ` Ahmad Fatoum 2022-06-20 8:16 ` Sascha Hauer 0 siblings, 1 reply; 7+ messages in thread From: Ahmad Fatoum @ 2022-06-20 7:59 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox 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 | ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] env: let setenv() take printf arguments 2022-06-20 7:59 ` Ahmad Fatoum @ 2022-06-20 8:16 ` Sascha Hauer 0 siblings, 0 replies; 7+ messages in thread From: Sascha Hauer @ 2022-06-20 8:16 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox 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 | ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-06-20 8:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-17 8:05 [PATCH 1/2] env: let setenv() take printf arguments Sascha Hauer 2022-06-17 8:05 ` [PATCH 2/2] treewide: Simplify setenv() calls Sascha Hauer 2022-06-17 21:53 ` Daniel Brát 2022-06-20 7:21 ` [PATCH] env: let setenv() take printf arguments Ahmad Fatoum 2022-06-20 7:47 ` Sascha Hauer 2022-06-20 7:59 ` Ahmad Fatoum 2022-06-20 8:16 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox