Messages in this thread Patch in this message | | | From | Seungil Kang <> | Subject | [PATCH v2] lib/cmdline: prevent unintented access to address | Date | Thu, 13 Aug 2020 12:07:41 +0900 |
| |
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
| |