linux-kernel - Re: [PATCH v2] lib/cmdline: prevent unintented access to address

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 12 Aug 2020 20:31:41 -0700
From:   Randy Dunlap <rdunlap@...radead.org>
To:     Seungil Kang <sil.kang@...sung.com>,
        andriy.shevchenko@...ux.intel.com
Cc:     bhe@...hat.com, mingo@...nel.org, akpm@...ux-foundation.org,
        gregkh@...uxfoundation.org, herbert@...dor.apana.org.au,
        tglx@...utronix.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] lib/cmdline: prevent unintented access to address

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@...sung.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