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:   Thu, 13 Aug 2020 17:00:09 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Seungil Kang <sil.kang@...sung.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 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