linux-kernel - Re: [PATCH 1/2] teach argv_split() to handle the mutable strings

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]

Message-Id: <20130318145333.abecd78f3dde5e1307a7e493@linux-foundation.org>
Date:	Mon, 18 Mar 2013 14:53:33 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andi Kleen <andi@...stfloor.org>,
	Lucas De Marchi <lucas.de.marchi@...il.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Paul Mackerras <paulus@...ba.org>, david@...son.dropbear.id.au,
	Kees Cook <keescook@...omium.org>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Feng Hong <hongfeng@...vell.com>,
	Lucas De Marchi <lucas.demarchi@...fusion.mobi>
Subject: Re: [PATCH 1/2] teach argv_split() to handle the mutable strings

On Sat, 16 Mar 2013 21:23:53 +0100 Oleg Nesterov <oleg@...hat.com> wrote:

> argv_split() allocates argv[count_argc(str)] array and assumes that
> it will find the same number of arguments later. This is obviously
> wrong if this string can be changed, say, by sysctl.
> 
> With this patch argv_split() kstrndup's the whole string and does
> not split it, we simply replace the spaces with zeroes and keep the
> allocated memory in argv[-1] for argv_free(arg).
> 
> We do not use argv[0] because:
> 
> 	- str can be all-spaces or empty. In fact this case is fine,
> 	  we could kfree() it before return, but:
> 
> 	- str can have a space at the start, and we can not rely on
> 	  kstrndup(skip_arg(str)) because it can equally race if this
> 	  string is mutable.
> 
> Also, simplify count_argc() and kill the no longer used skip_arg().
> 
> ...
>
>  char **argv_split(gfp_t gfp, const char *str, int *argcp)
>  {
> -	int argc = count_argc(str);
> -	char **argv = kzalloc(sizeof(*argv) * (argc+1), gfp);
> -	char **argvp;
> -
> -	if (argv == NULL)
> -		goto out;
> -
> -	if (argcp)
> -		*argcp = argc;
> -
> -	argvp = argv;
> -
> -	while (*str) {
> -		str = skip_spaces(str);
> -
> -		if (*str) {
> -			const char *p = str;
> -			char *t;
> -
> -			str = skip_arg(str);
> +	char *argv_str;
> +	bool was_space;
> +	char **argv, **argv_ret;
> +	int argc;
> +
> +	argv_str = kstrndup(str, KMALLOC_MAX_SIZE, gfp);

hm, I think that works.

kstrndup() does kmalloc_track_caller(len+1, gfp) so your
KMALLOC_MAX_SIZE is off-by-one?


>From reading the code it is rather unobvious why things were
implemented in this fashion.  People may come along in five years and
"clean it up".  Hence we should explain, no?

--- a/lib/argv_split.c~argv_split-teach-it-to-handle-mutable-strings-fix
+++ a/lib/argv_split.c
@@ -51,6 +51,10 @@ EXPORT_SYMBOL(argv_free);
  * considered to be a single argument separator.  The returned array
  * is always NULL-terminated.  Returns NULL on memory allocation
  * failure.
+ *
+ * The source string at `str' may be undergoing concurrent alteration via
+ * userspace sysctl activity (at least).  The argv_split() implementation
+ * attempts to handle this gracefully by taking a local copy to work on.
  */
 char **argv_split(gfp_t gfp, const char *str, int *argcp)
 {
_

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/