[PATCH v2] lib/cmdline: prevent unintented access to address

* [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