lkml.org 
[lkml]   [2020]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Subject[PATCH v2] lib/cmdline: prevent unintented access to address
Date
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
\
 
 \ /
  Last update: 2020-08-13 05:09    [W:0.050 / U:0.900 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site