Re: [PATCH v3 1/5] sysctl: fix lax sysctl_check_table() sanity check

[Andrew Morton]

On Thu, 18 May 2017 20:35:50 -0700 "Luis R. Rodriguez" <mcgrof@xxxxxxxxxx> 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.