linux-kernel - Re: [PATCH v3 1/5] sysctl: fix lax sysctl_check

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] [day] [month] [year] [list]

Date:   Mon, 22 May 2017 15:40:55 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc:     viro@...iv.linux.org.uk, ebiederm@...ssion.com,
        keescook@...omium.org, acme@...hat.com, mingo@...nel.org,
        mgorman@...e.de, subashab@...eaurora.org, jeyu@...hat.com,
        rusty@...tcorp.com.au, swhiteho@...hat.com, deepa.kernel@...il.com,
        matt@...eblueprint.co.uk, adobriyan@...il.com, bp@...e.de,
        zlpnobody@...il.com, dmitry.torokhov@...il.com, shuah@...nel.org,
        torvalds@...ux-foundation.org, linux@...ck-us.net,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/5] sysctl: fix lax sysctl_check_table() sanity
 check

On Thu, 18 May 2017 20:35:50 -0700 "Luis R. Rodriguez" <mcgrof@...nel.org> wrote:

> Commit 7c60c48f58a7 ("sysctl: Improve the sysctl sanity checks")
> improved sanity checks considerbly, however the enhancements on
> sysctl_check_table() meant adding a functional change so that
> only the last table entry's sanity error is propagated. It also
> changed the way errors were propagated so that each new check
> reset the err value, this means only last sanity check computed
> is used for an error. This has been in the kernel since v3.4 days.
> 
> Fix this by carrying on errors from previous checks and iterations
> as we traverse the table and ensuring we keep any error from previous
> checks. We keep iterating on the table even if an error is found so
> we can complain for all errors found in one shot. This works as
> -EINVAL is always returned on error anyway, and the check for error
> is any non-zero value.
> 
> ...
>
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -1066,7 +1066,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
>  	int err = 0;
>  	for (; table->procname; table++) {
>  		if (table->child)
> -			err = sysctl_err(path, table, "Not a file");
> +			err |= sysctl_err(path, table, "Not a file");
>  
>  		if ((table->proc_handler == proc_dostring) ||
>  		    (table->proc_handler == proc_dointvec) ||
> @@ -1078,15 +1078,15 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
>  		    (table->proc_handler == proc_doulongvec_minmax) ||
>  		    (table->proc_handler == proc_doulongvec_ms_jiffies_minmax)) {
>  			if (!table->data)
> -				err = sysctl_err(path, table, "No data");
> +				err |= sysctl_err(path, table, "No data");
>  			if (!table->maxlen)
> -				err = sysctl_err(path, table, "No maxlen");
> +				err |= sysctl_err(path, table, "No maxlen");
>  		}
>  		if (!table->proc_handler)
> -			err = sysctl_err(path, table, "No proc_handler");
> +			err |= sysctl_err(path, table, "No proc_handler");
>  
>  		if ((table->mode & (S_IRUGO|S_IWUGO)) != table->mode)
> -			err = sysctl_err(path, table, "bogus .mode 0%o",
> +			err |= sysctl_err(path, table, "bogus .mode 0%o",
>  				table->mode);
>  	}
>  	return err;

glumpf.  I'm not a fan of this err|=foo() trick - if foo() returns
different errnos we can a mangled result.  This patch assumes that
syscal_err() will only ever return a single errno.

I guess we're safe enough in this case but still... ugh.