* [PATCH v2] lib/cmdline: prevent unintented access to address
[not found] <CGME20200813030810epcas1p39ad56c069ab4fa41312f91f994c17cac@epcas1p3.samsung.com>
@ 2020-08-13 3:07 ` Seungil Kang
2020-08-13 3:31 ` Randy Dunlap
2020-08-13 14:00 ` Andy Shevchenko
0 siblings, 2 replies; 4+ messages in thread
From: Seungil Kang @ 2020-08-13 3:07 UTC (permalink / raw)
To: andriy.shevchenko
Cc: bhe, mingo, akpm, gregkh, herbert, tglx, linux-kernel, Seungil Kang
When args = "\"\0", "i" will be 0 and args[i-1] is used. (*lib/cmdline.c +238)
Because of "i" is an unsigned int type, the function will access at args[0xFFFFFFFF].
It can make a crash.
Signed-off-by: Seungil Kang <sil.kang@samsung.com>
---
Thanks for your review, my comments below
> Can you be less ambiguous with the args value? (Perhaps provide a hexdump of it
for better understanding)
This kind of args as hexdump below can cause crash.
00000000: 736f 6d65 7468 696e 6731 3d73 6f6d 655f something1=some_
00000010: 7661 6c75 6573 2022 0000 0000 0000 0000 values "
The args end with "\"\0".
> Please, use proper punctuation, I'm lost where is the sentence and what are the
logical parts of them.
I'm sorry to confuse you. I fix the commit msg
> Can you point out to the code that calls this and leads to a crash?
*lib/cmdlinc + 201 ~, next_arg function with args = "\"\0"
char *next_arg(char *args, char **param, char **val) <-- args = "\"\0".
{
unsigned int i, equals = 0;
int in_quote = 0, quoted = 0;
char *next;
if (*args == '"') { <-- *args == '"' is a true condition,
args++; <-- args++, so *args = '\0'.
in_quote = 1;
quoted = 1; <-- quoted also set 1.
}
for (i = 0; args[i]; i++) { <-- when reached this point, i = 0, and arg[0] == '\0',
so for loop is skipped.
if (isspace(args[i]) && !in_quote)
break;
if (equals == 0) {
if (args[i] == '=')
equals = i;
}
if (args[i] == '"')
in_quote = !in_quote;
}
*param = args;
if (!equals)
*val = NULL;
else {
args[equals] = '\0';
*val = args + equals + 1;
/* Don't include quotes in value. */
if (**val == '"') {
(*val)++;
if (args[i-1] == '"')
args[i-1] = '\0';
}
}
if (quoted && args[i-1] == '"') <-- When reached this point, quoted is still set 1,
"i" is still 0, and "i" is unsigned int type,
so address will be {address of args} + 0xFFFFFFFF.
It can make a crash.
args[i-1] = '\0';
if (args[i]) {
args[i] = '\0';
next = args + i + 1;
} else
next = args + i;
/* Chew up trailing spaces. */
return skip_spaces(next);
}
> Can you provide a KUnit test module which can check the case?
If necessary, I will make it and share it.
lib/cmdline.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/cmdline.c b/lib/cmdline.c
index fbb9981a04a4..2fd29d7723b2 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -200,7 +200,7 @@ bool parse_option_str(const char *str, const char *option)
*/
char *next_arg(char *args, char **param, char **val)
{
- unsigned int i, equals = 0;
+ int i, equals = 0;
int in_quote = 0, quoted = 0;
char *next;
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] lib/cmdline: prevent unintented access to address
2020-08-13 3:07 ` [PATCH v2] lib/cmdline: prevent unintented access to address Seungil Kang
@ 2020-08-13 3:31 ` Randy Dunlap
2020-08-13 14:00 ` Andy Shevchenko
1 sibling, 0 replies; 4+ messages in thread
From: Randy Dunlap @ 2020-08-13 3:31 UTC (permalink / raw)
To: Seungil Kang, andriy.shevchenko
Cc: bhe, mingo, akpm, gregkh, herbert, tglx, linux-kernel
On 8/12/20 8:07 PM, Seungil Kang wrote:
> When args = "\"\0", "i" will be 0 and args[i-1] is used. (*lib/cmdline.c +238)
> Because of "i" is an unsigned int type, the function will access at args[0xFFFFFFFF].
> It can make a crash.
>
> Signed-off-by: Seungil Kang <sil.kang@samsung.com>
> ---
>
> Thanks for your review, my comments below
>
>> Can you be less ambiguous with the args value? (Perhaps provide a hexdump of it
> for better understanding)
>
> This kind of args as hexdump below can cause crash.
>
> 00000000: 736f 6d65 7468 696e 6731 3d73 6f6d 655f something1=some_
> 00000010: 7661 6c75 6573 2022 0000 0000 0000 0000 values "
>
> The args end with "\"\0".
>
>> Please, use proper punctuation, I'm lost where is the sentence and what are the
> logical parts of them.
>
> I'm sorry to confuse you. I fix the commit msg
>
>> Can you point out to the code that calls this and leads to a crash?
>
> *lib/cmdlinc + 201 ~, next_arg function with args = "\"\0"
>
> char *next_arg(char *args, char **param, char **val) <-- args = "\"\0".
> {
> unsigned int i, equals = 0;
> int in_quote = 0, quoted = 0;
> char *next;
>
> if (*args == '"') { <-- *args == '"' is a true condition,
> args++; <-- args++, so *args = '\0'.
> in_quote = 1;
> quoted = 1; <-- quoted also set 1.
> }
>
> for (i = 0; args[i]; i++) { <-- when reached this point, i = 0, and arg[0] == '\0',
> so for loop is skipped.
> if (isspace(args[i]) && !in_quote)
> break;
> if (equals == 0) {
> if (args[i] == '=')
> equals = i;
> }
> if (args[i] == '"')
> in_quote = !in_quote;
> }
>
> *param = args;
> if (!equals)
> *val = NULL;
> else {
> args[equals] = '\0';
> *val = args + equals + 1;
>
> /* Don't include quotes in value. */
> if (**val == '"') {
> (*val)++;
> if (args[i-1] == '"')
> args[i-1] = '\0';
> }
> }
> if (quoted && args[i-1] == '"') <-- When reached this point, quoted is still set 1,
> "i" is still 0, and "i" is unsigned int type,
> so address will be {address of args} + 0xFFFFFFFF.
> It can make a crash.
> args[i-1] = '\0';
>
> if (args[i]) {
> args[i] = '\0';
> next = args + i + 1;
> } else
> next = args + i;
>
> /* Chew up trailing spaces. */
> return skip_spaces(next);
> }
>
>
>> Can you provide a KUnit test module which can check the case?
>
> If necessary, I will make it and share it.
Hi,
Have you tested this patch?
If so, how?
>
> lib/cmdline.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/cmdline.c b/lib/cmdline.c
> index fbb9981a04a4..2fd29d7723b2 100644
> --- a/lib/cmdline.c
> +++ b/lib/cmdline.c
> @@ -200,7 +200,7 @@ bool parse_option_str(const char *str, const char *option)
> */
> char *next_arg(char *args, char **param, char **val)
> {
> - unsigned int i, equals = 0;
> + int i, equals = 0;
> int in_quote = 0, quoted = 0;
> char *next;
>
>
thanks.
--
~Randy
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] lib/cmdline: prevent unintented access to address
2020-08-13 3:07 ` [PATCH v2] lib/cmdline: prevent unintented access to address Seungil Kang
2020-08-13 3:31 ` Randy Dunlap
@ 2020-08-13 14:00 ` Andy Shevchenko
2020-12-09 12:23 ` Andy Shevchenko
1 sibling, 1 reply; 4+ messages in thread
From: Andy Shevchenko @ 2020-08-13 14:00 UTC (permalink / raw)
To: Seungil Kang; +Cc: bhe, mingo, akpm, gregkh, herbert, tglx, linux-kernel
On Thu, Aug 13, 2020 at 12:07:41PM +0900, Seungil Kang wrote:
> When args = "\"\0", "i" will be 0 and args[i-1] is used. (*lib/cmdline.c +238)
What I meant is to put hex dump of the args here in the parentheses, something like
"When args = "... \"\0" (... 0x22 0x00), 'i' will be..."
> Because of "i" is an unsigned int type, the function will access at args[0xFFFFFFFF].
> It can make a crash.
...
> > Can you point out to the code that calls this and leads to a crash?
>
> *lib/cmdlinc + 201 ~, next_arg function with args = "\"\0"
Not the next_arg() code :-) The code which calls here...
...
> > Can you provide a KUnit test module which can check the case?
>
> If necessary, I will make it and share it.
Please, do as a separate patch in the series.
...
> --- a/lib/cmdline.c
> +++ b/lib/cmdline.c
> @@ -200,7 +200,7 @@ bool parse_option_str(const char *str, const char *option)
> */
> char *next_arg(char *args, char **param, char **val)
> {
> - unsigned int i, equals = 0;
> + int i, equals = 0;
> int in_quote = 0, quoted = 0;
> char *next;
At the first glance this is not correct fix for it: 0 - 1 is always 'all 1:s'
independently on signedness, but I need to think about.
And your test case / module would help a lot, if present.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] lib/cmdline: prevent unintented access to address
2020-08-13 14:00 ` Andy Shevchenko
@ 2020-12-09 12:23 ` Andy Shevchenko
0 siblings, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2020-12-09 12:23 UTC (permalink / raw)
To: Seungil Kang; +Cc: bhe, mingo, akpm, gregkh, herbert, tglx, linux-kernel
On Thu, Aug 13, 2020 at 05:00:09PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 13, 2020 at 12:07:41PM +0900, Seungil Kang wrote:
> And your test case / module would help a lot, if present.
Just a heads up: I have created a cmdline_kunit.c to test functions
in the module (currently only get_option() test cases are there).
It's in Andrew's quilt, pending for v5.11-rc1.
Feel free to extend it along with amended fix (as per my comments).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-12-09 12:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20200813030810epcas1p39ad56c069ab4fa41312f91f994c17cac@epcas1p3.samsung.com>
2020-08-13 3:07 ` [PATCH v2] lib/cmdline: prevent unintented access to address Seungil Kang
2020-08-13 3:31 ` Randy Dunlap
2020-08-13 14:00 ` Andy Shevchenko
2020-12-09 12:23 ` Andy Shevchenko
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).