* [PATCH 0/3] init/main.c: minor cleanup/bugfix of envvar handling
@ 2019-11-23 21:08 Arvind Sankar
2019-11-23 21:08 ` [PATCH 1/3] init/main.c: log arguments and environment passed to init Arvind Sankar
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-11-23 21:08 UTC (permalink / raw)
To: linux-kernel
unknown_bootoption passes unrecognized command line arguments to init as
either environment variables or arguments. Some of the logic in the
function is broken for quoted command line arguments.
When an argument of the form param="value" is processed by parse_args
and passed to unknown_bootoption, the command line has
param\0"value\0
with val pointing to the beginning of value. The helper function
repair_env_string is then used to restore the '=' character that was
removed by parse_args, and strip the quotes off fully. This results in
param=value\0\0
and val ends up pointing to the 'a' instead of the 'v' in value. This
bug was introduced when repair_env_string was refactored into a separate
function, and the decrement of val in repair_env_string became dead code.
This causes two problems in unknown_bootoption in the two places where
the val pointer is used as a substitute for the length of param:
1. An argument of the form param=".value" is misinterpreted as a
potential module parameter, with the result that it will not be placed
in init's environment.
2. An argument of the form param="value" is checked to see if param is
an existing environment variable that should be overwritten, but the
comparison is off-by-one and compares 'param=v' instead of 'param='
against the existing environment. So passing, for example, TERM="vt100"
on the command line results in init being passed both TERM=linux and
TERM=vt100 in its environment.
Patch 1 adds logging for the arguments and environment passed to init
and is independent of the rest: it can be dropped if this is
unnecessarily verbose.
Patch 2 removes repair_env_string from initcall parameter parsing in
do_initcall_level, as that uses a separate copy of the command line now
and the repairing is no longer necessary.
Patch 3 fixes the bug in unknown_bootoption by recording the length of
param explicitly instead of implying it from val-param.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] init/main.c: log arguments and environment passed to init
2019-11-23 21:08 [PATCH 0/3] init/main.c: minor cleanup/bugfix of envvar handling Arvind Sankar
@ 2019-11-23 21:08 ` Arvind Sankar
2019-11-23 21:08 ` [PATCH 2/3] init/main.c: remove unnecessary repair_env_string in do_initcall_level Arvind Sankar
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-11-23 21:08 UTC (permalink / raw)
To: linux-kernel, nivedita
Extend logging in `run_init_process` to also show the arguments and
environment that we are passing to init.
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
init/main.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/init/main.c b/init/main.c
index 91f6ebb30ef0..c92f0376b1bc 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1042,8 +1042,16 @@ static void __init do_pre_smp_initcalls(void)
static int run_init_process(const char *init_filename)
{
+ const char *const *p;
+
argv_init[0] = init_filename;
pr_info("Run %s as init process\n", init_filename);
+ pr_info(" with arguments:\n");
+ for (p = argv_init; *p; p++)
+ pr_info(" %s\n", *p);
+ pr_info(" with environment:\n");
+ for (p = envp_init; *p; p++)
+ pr_info(" %s\n", *p);
return do_execve(getname_kernel(init_filename),
(const char __user *const __user *)argv_init,
(const char __user *const __user *)envp_init);
--
2.23.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/3] init/main.c: remove unnecessary repair_env_string in do_initcall_level
2019-11-23 21:08 [PATCH 0/3] init/main.c: minor cleanup/bugfix of envvar handling Arvind Sankar
2019-11-23 21:08 ` [PATCH 1/3] init/main.c: log arguments and environment passed to init Arvind Sankar
@ 2019-11-23 21:08 ` Arvind Sankar
2019-11-23 21:08 ` [PATCH 3/3] init/main.c: fix quoted value handling in unknown_bootoption Arvind Sankar
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-11-23 21:08 UTC (permalink / raw)
To: linux-kernel, nivedita
Since commit 08746a65c296 ("init: fix in-place parameter modification
regression"), parse_args in do_initcall_level is called on a copy of
saved_command_line. It is unnecessary to call repair_env_string during
this parsing, as this copy is not used for anything later.
Remove the now unnecessary arguments from repair_env_string as well.
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
init/main.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/init/main.c b/init/main.c
index c92f0376b1bc..a2008e7a797f 100644
--- a/init/main.c
+++ b/init/main.c
@@ -246,8 +246,7 @@ static int __init loglevel(char *str)
early_param("loglevel", loglevel);
/* Change NUL term back to "=", to make "param" the whole string. */
-static int __init repair_env_string(char *param, char *val,
- const char *unused, void *arg)
+static void __init repair_env_string(char *param, char *val)
{
if (val) {
/* param=val or param="val"? */
@@ -256,11 +255,9 @@ static int __init repair_env_string(char *param, char *val,
else if (val == param+strlen(param)+2) {
val[-2] = '=';
memmove(val-1, val, strlen(val)+1);
- val--;
} else
BUG();
}
- return 0;
}
/* Anything after -- gets handed straight to init. */
@@ -272,7 +269,7 @@ static int __init set_init_arg(char *param, char *val,
if (panic_later)
return 0;
- repair_env_string(param, val, unused, NULL);
+ repair_env_string(param, val);
for (i = 0; argv_init[i]; i++) {
if (i == MAX_INIT_ARGS) {
@@ -292,7 +289,7 @@ static int __init set_init_arg(char *param, char *val,
static int __init unknown_bootoption(char *param, char *val,
const char *unused, void *arg)
{
- repair_env_string(param, val, unused, NULL);
+ repair_env_string(param, val);
/* Handle obsolete-style parameters */
if (obsolete_checksetup(param))
@@ -990,6 +987,12 @@ static const char *initcall_level_names[] __initdata = {
"late",
};
+static int __init ignore_unknown_bootoption(char *param, char *val,
+ const char *unused, void *arg)
+{
+ return 0;
+}
+
static void __init do_initcall_level(int level)
{
initcall_entry_t *fn;
@@ -999,7 +1002,7 @@ static void __init do_initcall_level(int level)
initcall_command_line, __start___param,
__stop___param - __start___param,
level, level,
- NULL, &repair_env_string);
+ NULL, ignore_unknown_bootoption);
trace_initcall_level(initcall_level_names[level]);
for (fn = initcall_levels[level]; fn < initcall_levels[level+1]; fn++)
--
2.23.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/3] init/main.c: fix quoted value handling in unknown_bootoption
2019-11-23 21:08 [PATCH 0/3] init/main.c: minor cleanup/bugfix of envvar handling Arvind Sankar
2019-11-23 21:08 ` [PATCH 1/3] init/main.c: log arguments and environment passed to init Arvind Sankar
2019-11-23 21:08 ` [PATCH 2/3] init/main.c: remove unnecessary repair_env_string in do_initcall_level Arvind Sankar
@ 2019-11-23 21:08 ` Arvind Sankar
2019-11-23 21:20 ` [PATCH 0/3] init/main.c: minor cleanup/bugfix of envvar handling Randy Dunlap
2019-11-23 21:40 ` [PATCH RESEND " Arvind Sankar
4 siblings, 0 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-11-23 21:08 UTC (permalink / raw)
To: linux-kernel, nivedita
Commit a99cd1125189 ("init: fix bug where environment vars can't be
passed via boot args") introduced two minor bugs in unknown_bootoption
by factoring out the quoted value handling into a separate function.
When value is quoted, repair_env_string will move the value up 1 byte to
strip the quotes, so val in unknown_bootoption no longer points to the
actual location of the value.
The result is that an argument of the form param=".value" is mistakenly
treated as a potential module parameter and is not placed in init's
environment, and an argument of the form param="value" can result in a
duplicate environment variable: eg TERM="vt100" on the command line will
result in both TERM=linux and TERM=vt100 being placed into init's
environment.
Fix this by recording the length of the param before calling
repair_env_string instead of relying on val.
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
init/main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/init/main.c b/init/main.c
index a2008e7a797f..1ee92517c515 100644
--- a/init/main.c
+++ b/init/main.c
@@ -289,6 +289,8 @@ static int __init set_init_arg(char *param, char *val,
static int __init unknown_bootoption(char *param, char *val,
const char *unused, void *arg)
{
+ size_t len = strlen(param);
+
repair_env_string(param, val);
/* Handle obsolete-style parameters */
@@ -296,7 +298,7 @@ static int __init unknown_bootoption(char *param, char *val,
return 0;
/* Unused module parameter. */
- if (strchr(param, '.') && (!val || strchr(param, '.') < val))
+ if (strnchr(param, len, '.'))
return 0;
if (panic_later)
@@ -310,7 +312,7 @@ static int __init unknown_bootoption(char *param, char *val,
panic_later = "env";
panic_param = param;
}
- if (!strncmp(param, envp_init[i], val - param))
+ if (!strncmp(param, envp_init[i], len+1))
break;
}
envp_init[i] = param;
--
2.23.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] init/main.c: minor cleanup/bugfix of envvar handling
2019-11-23 21:08 [PATCH 0/3] init/main.c: minor cleanup/bugfix of envvar handling Arvind Sankar
` (2 preceding siblings ...)
2019-11-23 21:08 ` [PATCH 3/3] init/main.c: fix quoted value handling in unknown_bootoption Arvind Sankar
@ 2019-11-23 21:20 ` Randy Dunlap
2019-11-23 21:26 ` Arvind Sankar
2019-11-23 21:40 ` [PATCH RESEND " Arvind Sankar
4 siblings, 1 reply; 15+ messages in thread
From: Randy Dunlap @ 2019-11-23 21:20 UTC (permalink / raw)
To: Arvind Sankar, linux-kernel
On 11/23/19 1:08 PM, Arvind Sankar wrote:
> unknown_bootoption passes unrecognized command line arguments to init as
> either environment variables or arguments. Some of the logic in the
> function is broken for quoted command line arguments.
>
Hi,
You will need to send your patches to some maintainer who could merge them.
Nobody browses LKML to pick up patches (other than bots).
See Documentation/process/submitting-patches.rst: section 5:
5) Select the recipients for your patch
---------------------------------------
for more info.
--
~Randy
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] init/main.c: minor cleanup/bugfix of envvar handling
2019-11-23 21:20 ` [PATCH 0/3] init/main.c: minor cleanup/bugfix of envvar handling Randy Dunlap
@ 2019-11-23 21:26 ` Arvind Sankar
0 siblings, 0 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-11-23 21:26 UTC (permalink / raw)
To: Randy Dunlap; +Cc: Arvind Sankar, linux-kernel
On Sat, Nov 23, 2019 at 01:20:07PM -0800, Randy Dunlap wrote:
> On 11/23/19 1:08 PM, Arvind Sankar wrote:
> > unknown_bootoption passes unrecognized command line arguments to init as
> > either environment variables or arguments. Some of the logic in the
> > function is broken for quoted command line arguments.
> >
>
> Hi,
>
> You will need to send your patches to some maintainer who could merge them.
> Nobody browses LKML to pick up patches (other than bots).
>
> See Documentation/process/submitting-patches.rst: section 5:
> 5) Select the recipients for your patch
> ---------------------------------------
>
> for more info.
>
> --
> ~Randy
>
init/ didn't seem to have anyone in MAINTAINERS, I see that Andrew
Morton is referenced in that doc as maintainer of last resort, I'll send
them to him.
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH RESEND 0/3] init/main.c: minor cleanup/bugfix of envvar handling
2019-11-23 21:08 [PATCH 0/3] init/main.c: minor cleanup/bugfix of envvar handling Arvind Sankar
` (3 preceding siblings ...)
2019-11-23 21:20 ` [PATCH 0/3] init/main.c: minor cleanup/bugfix of envvar handling Randy Dunlap
@ 2019-11-23 21:40 ` Arvind Sankar
2019-11-23 21:40 ` [PATCH RESEND 1/3] init/main.c: log arguments and environment passed to init Arvind Sankar
` (4 more replies)
4 siblings, 5 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-11-23 21:40 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
unknown_bootoption passes unrecognized command line arguments to init as
either environment variables or arguments. Some of the logic in the
function is broken for quoted command line arguments.
When an argument of the form param="value" is processed by parse_args
and passed to unknown_bootoption, the command line has
param\0"value\0
with val pointing to the beginning of value. The helper function
repair_env_string is then used to restore the '=' character that was
removed by parse_args, and strip the quotes off fully. This results in
param=value\0\0
and val ends up pointing to the 'a' instead of the 'v' in value. This
bug was introduced when repair_env_string was refactored into a separate
function, and the decrement of val in repair_env_string became dead code.
This causes two problems in unknown_bootoption in the two places where
the val pointer is used as a substitute for the length of param:
1. An argument of the form param=".value" is misinterpreted as a
potential module parameter, with the result that it will not be placed
in init's environment.
2. An argument of the form param="value" is checked to see if param is
an existing environment variable that should be overwritten, but the
comparison is off-by-one and compares 'param=v' instead of 'param='
against the existing environment. So passing, for example, TERM="vt100"
on the command line results in init being passed both TERM=linux and
TERM=vt100 in its environment.
Patch 1 adds logging for the arguments and environment passed to init
and is independent of the rest: it can be dropped if this is
unnecessarily verbose.
Patch 2 removes repair_env_string from initcall parameter parsing in
do_initcall_level, as that uses a separate copy of the command line now
and the repairing is no longer necessary.
Patch 3 fixes the bug in unknown_bootoption by recording the length of
param explicitly instead of implying it from val-param.
Arvind Sankar (3):
init/main.c: log arguments and environment passed to init
init/main.c: remove unnecessary repair_env_string in do_initcall_level
init/main.c: fix quoted value handling in unknown_bootoption
init/main.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH RESEND 1/3] init/main.c: log arguments and environment passed to init
2019-11-23 21:40 ` [PATCH RESEND " Arvind Sankar
@ 2019-11-23 21:40 ` Arvind Sankar
2019-11-23 21:40 ` [PATCH RESEND 2/3] init/main.c: remove unnecessary repair_env_string in do_initcall_level Arvind Sankar
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-11-23 21:40 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
Extend logging in `run_init_process` to also show the arguments and
environment that we are passing to init.
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
init/main.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/init/main.c b/init/main.c
index 91f6ebb30ef0..c92f0376b1bc 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1042,8 +1042,16 @@ static void __init do_pre_smp_initcalls(void)
static int run_init_process(const char *init_filename)
{
+ const char *const *p;
+
argv_init[0] = init_filename;
pr_info("Run %s as init process\n", init_filename);
+ pr_info(" with arguments:\n");
+ for (p = argv_init; *p; p++)
+ pr_info(" %s\n", *p);
+ pr_info(" with environment:\n");
+ for (p = envp_init; *p; p++)
+ pr_info(" %s\n", *p);
return do_execve(getname_kernel(init_filename),
(const char __user *const __user *)argv_init,
(const char __user *const __user *)envp_init);
--
2.23.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH RESEND 2/3] init/main.c: remove unnecessary repair_env_string in do_initcall_level
2019-11-23 21:40 ` [PATCH RESEND " Arvind Sankar
2019-11-23 21:40 ` [PATCH RESEND 1/3] init/main.c: log arguments and environment passed to init Arvind Sankar
@ 2019-11-23 21:40 ` Arvind Sankar
2019-11-23 21:40 ` [PATCH RESEND 3/3] init/main.c: fix quoted value handling in unknown_bootoption Arvind Sankar
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-11-23 21:40 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
Since commit 08746a65c296 ("init: fix in-place parameter modification
regression"), parse_args in do_initcall_level is called on a copy of
saved_command_line. It is unnecessary to call repair_env_string during
this parsing, as this copy is not used for anything later.
Remove the now unnecessary arguments from repair_env_string as well.
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
init/main.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/init/main.c b/init/main.c
index c92f0376b1bc..a2008e7a797f 100644
--- a/init/main.c
+++ b/init/main.c
@@ -246,8 +246,7 @@ static int __init loglevel(char *str)
early_param("loglevel", loglevel);
/* Change NUL term back to "=", to make "param" the whole string. */
-static int __init repair_env_string(char *param, char *val,
- const char *unused, void *arg)
+static void __init repair_env_string(char *param, char *val)
{
if (val) {
/* param=val or param="val"? */
@@ -256,11 +255,9 @@ static int __init repair_env_string(char *param, char *val,
else if (val == param+strlen(param)+2) {
val[-2] = '=';
memmove(val-1, val, strlen(val)+1);
- val--;
} else
BUG();
}
- return 0;
}
/* Anything after -- gets handed straight to init. */
@@ -272,7 +269,7 @@ static int __init set_init_arg(char *param, char *val,
if (panic_later)
return 0;
- repair_env_string(param, val, unused, NULL);
+ repair_env_string(param, val);
for (i = 0; argv_init[i]; i++) {
if (i == MAX_INIT_ARGS) {
@@ -292,7 +289,7 @@ static int __init set_init_arg(char *param, char *val,
static int __init unknown_bootoption(char *param, char *val,
const char *unused, void *arg)
{
- repair_env_string(param, val, unused, NULL);
+ repair_env_string(param, val);
/* Handle obsolete-style parameters */
if (obsolete_checksetup(param))
@@ -990,6 +987,12 @@ static const char *initcall_level_names[] __initdata = {
"late",
};
+static int __init ignore_unknown_bootoption(char *param, char *val,
+ const char *unused, void *arg)
+{
+ return 0;
+}
+
static void __init do_initcall_level(int level)
{
initcall_entry_t *fn;
@@ -999,7 +1002,7 @@ static void __init do_initcall_level(int level)
initcall_command_line, __start___param,
__stop___param - __start___param,
level, level,
- NULL, &repair_env_string);
+ NULL, ignore_unknown_bootoption);
trace_initcall_level(initcall_level_names[level]);
for (fn = initcall_levels[level]; fn < initcall_levels[level+1]; fn++)
--
2.23.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH RESEND 3/3] init/main.c: fix quoted value handling in unknown_bootoption
2019-11-23 21:40 ` [PATCH RESEND " Arvind Sankar
2019-11-23 21:40 ` [PATCH RESEND 1/3] init/main.c: log arguments and environment passed to init Arvind Sankar
2019-11-23 21:40 ` [PATCH RESEND 2/3] init/main.c: remove unnecessary repair_env_string in do_initcall_level Arvind Sankar
@ 2019-11-23 21:40 ` Arvind Sankar
2019-11-27 19:25 ` [PATCH RESEND 0/3] init/main.c: minor cleanup/bugfix of envvar handling Arvind Sankar
2019-12-12 18:00 ` [PATCH v2 " Arvind Sankar
4 siblings, 0 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-11-23 21:40 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
Commit a99cd1125189 ("init: fix bug where environment vars can't be
passed via boot args") introduced two minor bugs in unknown_bootoption
by factoring out the quoted value handling into a separate function.
When value is quoted, repair_env_string will move the value up 1 byte to
strip the quotes, so val in unknown_bootoption no longer points to the
actual location of the value.
The result is that an argument of the form param=".value" is mistakenly
treated as a potential module parameter and is not placed in init's
environment, and an argument of the form param="value" can result in a
duplicate environment variable: eg TERM="vt100" on the command line will
result in both TERM=linux and TERM=vt100 being placed into init's
environment.
Fix this by recording the length of the param before calling
repair_env_string instead of relying on val.
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
init/main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/init/main.c b/init/main.c
index a2008e7a797f..1ee92517c515 100644
--- a/init/main.c
+++ b/init/main.c
@@ -289,6 +289,8 @@ static int __init set_init_arg(char *param, char *val,
static int __init unknown_bootoption(char *param, char *val,
const char *unused, void *arg)
{
+ size_t len = strlen(param);
+
repair_env_string(param, val);
/* Handle obsolete-style parameters */
@@ -296,7 +298,7 @@ static int __init unknown_bootoption(char *param, char *val,
return 0;
/* Unused module parameter. */
- if (strchr(param, '.') && (!val || strchr(param, '.') < val))
+ if (strnchr(param, len, '.'))
return 0;
if (panic_later)
@@ -310,7 +312,7 @@ static int __init unknown_bootoption(char *param, char *val,
panic_later = "env";
panic_param = param;
}
- if (!strncmp(param, envp_init[i], val - param))
+ if (!strncmp(param, envp_init[i], len+1))
break;
}
envp_init[i] = param;
--
2.23.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH RESEND 0/3] init/main.c: minor cleanup/bugfix of envvar handling
2019-11-23 21:40 ` [PATCH RESEND " Arvind Sankar
` (2 preceding siblings ...)
2019-11-23 21:40 ` [PATCH RESEND 3/3] init/main.c: fix quoted value handling in unknown_bootoption Arvind Sankar
@ 2019-11-27 19:25 ` Arvind Sankar
2019-12-12 18:00 ` [PATCH v2 " Arvind Sankar
4 siblings, 0 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-11-27 19:25 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
On Sat, Nov 23, 2019 at 04:40:36PM -0500, Arvind Sankar wrote:
> unknown_bootoption passes unrecognized command line arguments to init as
> either environment variables or arguments. Some of the logic in the
> function is broken for quoted command line arguments.
>
Hi Andrew, will you have a chance to look at this patch? If you are not
the correct maintainer, would you be able to point me in the right
direction?
Thanks
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 0/3] init/main.c: minor cleanup/bugfix of envvar handling
2019-11-23 21:40 ` [PATCH RESEND " Arvind Sankar
` (3 preceding siblings ...)
2019-11-27 19:25 ` [PATCH RESEND 0/3] init/main.c: minor cleanup/bugfix of envvar handling Arvind Sankar
@ 2019-12-12 18:00 ` Arvind Sankar
2019-12-12 18:00 ` [PATCH v2 1/3] init/main.c: log arguments and environment passed to init Arvind Sankar
` (2 more replies)
4 siblings, 3 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-12-12 18:00 UTC (permalink / raw)
To: Andrew Morton, Linus Torvalds; +Cc: linux-kernel
unknown_bootoption passes unrecognized command line arguments to init as
either environment variables or arguments. Some of the logic in the
function is broken for quoted command line arguments.
When an argument of the form param="value" is processed by parse_args
and passed to unknown_bootoption, the command line has
param\0"value\0
with val pointing to the beginning of value. The helper function
repair_env_string is then used to restore the '=' character that was
removed by parse_args, and strip the quotes off fully. This results in
param=value\0\0
and val ends up pointing to the 'a' instead of the 'v' in value. This
bug was introduced when repair_env_string was refactored into a separate
function, and the decrement of val in repair_env_string became dead code.
This causes two problems in unknown_bootoption in the two places where
the val pointer is used as a substitute for the length of param:
1. An argument of the form param=".value" is misinterpreted as a
potential module parameter, with the result that it will not be placed
in init's environment.
2. An argument of the form param="value" is checked to see if param is
an existing environment variable that should be overwritten, but the
comparison is off-by-one and compares 'param=v' instead of 'param='
against the existing environment. So passing, for example, TERM="vt100"
on the command line results in init being passed both TERM=linux and
TERM=vt100 in its environment.
Patch 1 adds logging for the arguments and environment passed to init
and is independent of the rest: it can be dropped if this is
unnecessarily verbose.
Patch 2 removes repair_env_string from initcall parameter parsing in
do_initcall_level, as that uses a separate copy of the command line now
and the repairing is no longer necessary.
Patch 3 fixes the bug in unknown_bootoption by recording the length of
param explicitly instead of implying it from val-param.
Changes from v1:
- use pr_debug for additional logging in patch 1
- move removal of dead val--; line from patch 2 to patch 3
Arvind Sankar (3):
init/main.c: log arguments and environment passed to init
init/main.c: remove unnecessary repair_env_string in do_initcall_level
init/main.c: fix quoted value handling in unknown_bootoption
init/main.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/3] init/main.c: log arguments and environment passed to init
2019-12-12 18:00 ` [PATCH v2 " Arvind Sankar
@ 2019-12-12 18:00 ` Arvind Sankar
2019-12-12 18:00 ` [PATCH v2 2/3] init/main.c: remove unnecessary repair_env_string in do_initcall_level Arvind Sankar
2019-12-12 18:00 ` [PATCH v2 3/3] init/main.c: fix quoted value handling in unknown_bootoption Arvind Sankar
2 siblings, 0 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-12-12 18:00 UTC (permalink / raw)
To: Andrew Morton, Linus Torvalds; +Cc: linux-kernel
Extend logging in `run_init_process` to also show the arguments and
environment that we are passing to init.
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
init/main.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/init/main.c b/init/main.c
index 91f6ebb30ef0..4ebf4346a44c 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1042,8 +1042,16 @@ static void __init do_pre_smp_initcalls(void)
static int run_init_process(const char *init_filename)
{
+ const char *const *p;
+
argv_init[0] = init_filename;
pr_info("Run %s as init process\n", init_filename);
+ pr_debug(" with arguments:\n");
+ for (p = argv_init; *p; p++)
+ pr_debug(" %s\n", *p);
+ pr_debug(" with environment:\n");
+ for (p = envp_init; *p; p++)
+ pr_debug(" %s\n", *p);
return do_execve(getname_kernel(init_filename),
(const char __user *const __user *)argv_init,
(const char __user *const __user *)envp_init);
--
2.23.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/3] init/main.c: remove unnecessary repair_env_string in do_initcall_level
2019-12-12 18:00 ` [PATCH v2 " Arvind Sankar
2019-12-12 18:00 ` [PATCH v2 1/3] init/main.c: log arguments and environment passed to init Arvind Sankar
@ 2019-12-12 18:00 ` Arvind Sankar
2019-12-12 18:00 ` [PATCH v2 3/3] init/main.c: fix quoted value handling in unknown_bootoption Arvind Sankar
2 siblings, 0 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-12-12 18:00 UTC (permalink / raw)
To: Andrew Morton, Linus Torvalds; +Cc: linux-kernel
Since commit 08746a65c296 ("init: fix in-place parameter modification
regression"), parse_args in do_initcall_level is called on a copy of
saved_command_line. It is unnecessary to call repair_env_string during
this parsing, as this copy is not used for anything later.
Remove the now unnecessary arguments from repair_env_string as well.
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
init/main.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/init/main.c b/init/main.c
index 4ebf4346a44c..a1febc3de582 100644
--- a/init/main.c
+++ b/init/main.c
@@ -246,8 +246,7 @@ static int __init loglevel(char *str)
early_param("loglevel", loglevel);
/* Change NUL term back to "=", to make "param" the whole string. */
-static int __init repair_env_string(char *param, char *val,
- const char *unused, void *arg)
+static void __init repair_env_string(char *param, char *val)
{
if (val) {
/* param=val or param="val"? */
@@ -260,7 +259,6 @@ static int __init repair_env_string(char *param, char *val,
} else
BUG();
}
- return 0;
}
/* Anything after -- gets handed straight to init. */
@@ -272,7 +270,7 @@ static int __init set_init_arg(char *param, char *val,
if (panic_later)
return 0;
- repair_env_string(param, val, unused, NULL);
+ repair_env_string(param, val);
for (i = 0; argv_init[i]; i++) {
if (i == MAX_INIT_ARGS) {
@@ -292,7 +290,7 @@ static int __init set_init_arg(char *param, char *val,
static int __init unknown_bootoption(char *param, char *val,
const char *unused, void *arg)
{
- repair_env_string(param, val, unused, NULL);
+ repair_env_string(param, val);
/* Handle obsolete-style parameters */
if (obsolete_checksetup(param))
@@ -990,6 +988,12 @@ static const char *initcall_level_names[] __initdata = {
"late",
};
+static int __init ignore_unknown_bootoption(char *param, char *val,
+ const char *unused, void *arg)
+{
+ return 0;
+}
+
static void __init do_initcall_level(int level)
{
initcall_entry_t *fn;
@@ -999,7 +1003,7 @@ static void __init do_initcall_level(int level)
initcall_command_line, __start___param,
__stop___param - __start___param,
level, level,
- NULL, &repair_env_string);
+ NULL, ignore_unknown_bootoption);
trace_initcall_level(initcall_level_names[level]);
for (fn = initcall_levels[level]; fn < initcall_levels[level+1]; fn++)
--
2.23.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 3/3] init/main.c: fix quoted value handling in unknown_bootoption
2019-12-12 18:00 ` [PATCH v2 " Arvind Sankar
2019-12-12 18:00 ` [PATCH v2 1/3] init/main.c: log arguments and environment passed to init Arvind Sankar
2019-12-12 18:00 ` [PATCH v2 2/3] init/main.c: remove unnecessary repair_env_string in do_initcall_level Arvind Sankar
@ 2019-12-12 18:00 ` Arvind Sankar
2 siblings, 0 replies; 15+ messages in thread
From: Arvind Sankar @ 2019-12-12 18:00 UTC (permalink / raw)
To: Andrew Morton, Linus Torvalds; +Cc: linux-kernel
Commit a99cd1125189 ("init: fix bug where environment vars can't be
passed via boot args") introduced two minor bugs in unknown_bootoption
by factoring out the quoted value handling into a separate function.
When value is quoted, repair_env_string will move the value up 1 byte to
strip the quotes, so val in unknown_bootoption no longer points to the
actual location of the value.
The result is that an argument of the form param=".value" is mistakenly
treated as a potential module parameter and is not placed in init's
environment, and an argument of the form param="value" can result in a
duplicate environment variable: eg TERM="vt100" on the command line will
result in both TERM=linux and TERM=vt100 being placed into init's
environment.
Fix this by recording the length of the param before calling
repair_env_string instead of relying on val.
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
init/main.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/init/main.c b/init/main.c
index a1febc3de582..efece9fe988e 100644
--- a/init/main.c
+++ b/init/main.c
@@ -255,7 +255,6 @@ static void __init repair_env_string(char *param, char *val)
else if (val == param+strlen(param)+2) {
val[-2] = '=';
memmove(val-1, val, strlen(val)+1);
- val--;
} else
BUG();
}
@@ -290,6 +289,8 @@ static int __init set_init_arg(char *param, char *val,
static int __init unknown_bootoption(char *param, char *val,
const char *unused, void *arg)
{
+ size_t len = strlen(param);
+
repair_env_string(param, val);
/* Handle obsolete-style parameters */
@@ -297,7 +298,7 @@ static int __init unknown_bootoption(char *param, char *val,
return 0;
/* Unused module parameter. */
- if (strchr(param, '.') && (!val || strchr(param, '.') < val))
+ if (strnchr(param, len, '.'))
return 0;
if (panic_later)
@@ -311,7 +312,7 @@ static int __init unknown_bootoption(char *param, char *val,
panic_later = "env";
panic_param = param;
}
- if (!strncmp(param, envp_init[i], val - param))
+ if (!strncmp(param, envp_init[i], len+1))
break;
}
envp_init[i] = param;
--
2.23.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-12-12 18:00 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-23 21:08 [PATCH 0/3] init/main.c: minor cleanup/bugfix of envvar handling Arvind Sankar
2019-11-23 21:08 ` [PATCH 1/3] init/main.c: log arguments and environment passed to init Arvind Sankar
2019-11-23 21:08 ` [PATCH 2/3] init/main.c: remove unnecessary repair_env_string in do_initcall_level Arvind Sankar
2019-11-23 21:08 ` [PATCH 3/3] init/main.c: fix quoted value handling in unknown_bootoption Arvind Sankar
2019-11-23 21:20 ` [PATCH 0/3] init/main.c: minor cleanup/bugfix of envvar handling Randy Dunlap
2019-11-23 21:26 ` Arvind Sankar
2019-11-23 21:40 ` [PATCH RESEND " Arvind Sankar
2019-11-23 21:40 ` [PATCH RESEND 1/3] init/main.c: log arguments and environment passed to init Arvind Sankar
2019-11-23 21:40 ` [PATCH RESEND 2/3] init/main.c: remove unnecessary repair_env_string in do_initcall_level Arvind Sankar
2019-11-23 21:40 ` [PATCH RESEND 3/3] init/main.c: fix quoted value handling in unknown_bootoption Arvind Sankar
2019-11-27 19:25 ` [PATCH RESEND 0/3] init/main.c: minor cleanup/bugfix of envvar handling Arvind Sankar
2019-12-12 18:00 ` [PATCH v2 " Arvind Sankar
2019-12-12 18:00 ` [PATCH v2 1/3] init/main.c: log arguments and environment passed to init Arvind Sankar
2019-12-12 18:00 ` [PATCH v2 2/3] init/main.c: remove unnecessary repair_env_string in do_initcall_level Arvind Sankar
2019-12-12 18:00 ` [PATCH v2 3/3] init/main.c: fix quoted value handling in unknown_bootoption Arvind Sankar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).