Re: [PATCH v2 2/9] sysctl: add proper unsigned int support

["Luis R. Rodriguez" <mcgrof@kernel.org>]

On Mon, Feb 13, 2017 at 12:19:51PM -0800, Kees Cook wrote:
> On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > ---
> >  fs/proc/proc_sysctl.c |  15 +++++
> >  kernel/sysctl.c       | 161 ++++++++++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 170 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> > index d22ee738d2eb..73696a73a1ec 100644
> > --- a/fs/proc/proc_sysctl.c
> > +++ b/fs/proc/proc_sysctl.c
> > @@ -1031,6 +1031,18 @@ static int sysctl_err(const char *path, struct ctl_table *table, char *fmt, ...)
> >         return -EINVAL;
> >  }
> >
> > +static int sysctl_check_table_array(const char *path, struct ctl_table *table)
> > +{
> > +       int err = 0;
> > +
> > +       if (table->proc_handler == proc_douintvec) {
> 
> Should this be inverted? i.e. explicitly allow proc_handlers instead
> of only rejecting douintvec?

The goal is to start avoiding the use of the silly arrays, and we start drawing
the line with proc_douintvec. The expectation then is that this list will grow
so the check should easily allow for growth of more exclusions for now, once we
are done we would hopefully only end up with strings that use arrays. For now
then we are limited to a specific check against the size of the parameter that
the proc handler uses, so for instance:

> > +               if (table->maxlen != sizeof(unsigned int))
> > +                       err |= sysctl_err(path, table, "array now allowed");
> > +       }

This uses unsigned int, I expect the check to be different for proc_dointvec_jiffies
once we vet no array uses exist for it. The way I think its best to expand on this
list is to later add:

else if (table->proc_handler == proc_dointvec_jiffies) {
	...
}

If did the other way around and started this check with:

if (table->proc_handler != proc_douintvec)
	return 0;

We'd still have to add a specific check for each handler for the actual expected
size for one element. So I would prefer to keep it this way for now. Once we have
vetted arrays are only needed for strings (and hopefully if we can change a few
ints users, not sure if proc is "uapi"; think it is so we might be shit out of luck),
then we'd only have a short white-list. Even so, the size check on the others is
probably good to leave, as such a check does not exist so we would not have to
nuke those old checks.

> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index 1aea594a54db..493bc05e546a 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -2243,6 +2243,155 @@ static int do_proc_dointvec(struct ctl_table *table, int write,
> >                         buffer, lenp, ppos, conv, data);
> >  }
> >
> > +static int do_proc_douintvec_w(unsigned int *tbl_data,
> > +                              struct ctl_table *table,
> > +                              void __user *buffer,
> > +                              size_t *lenp, loff_t *ppos,
> > +                              int (*conv)(unsigned long *lvalp,
> > +                                          unsigned int *valp,
> > +                                          int write, void *data),
> > +                              void *data)
> > +{
> > +       unsigned long lval;
> > +       int err = 0;
> > +       size_t left;
> > +       bool neg;
> > +       char *kbuf = NULL, *p;
> > +
> > +       left = *lenp;
> > +
> > +       if (*ppos) {
> > +               switch (sysctl_writes_strict) {
> > +               case SYSCTL_WRITES_STRICT:
> > +                       goto bail_early;
> > +               case SYSCTL_WRITES_WARN:
> > +                       warn_sysctl_write(table);
> > +                       break;
> > +               default:
> > +                       break;
> > +               }
> > +       }
> 
> I wonder if this SYSCTL_WRITES_* test copy/pasting needs to be a function?

Good call. Folded in a new patch that does this. This stuff was a bit cryptic
so also folded in another patch which documented the strict stuff a bit in
kdoc form. Later with time we can move to new sphinx doc format and take
advantage of that.

<-- snip -->

> > +static int __do_proc_douintvec(void *tbl_data, struct ctl_table *table,
> > +                              int write, void __user *buffer,
> > +                              size_t *lenp, loff_t *ppos,
> > +                              int (*conv)(unsigned long *lvalp,
> > +                                          unsigned int *valp,
> > +                                          int write, void *data),
> > +                              void *data)
> > +{
> > +       unsigned int *i, vleft;
> > +
> > +       if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) {
> > +               *lenp = 0;
> > +               return 0;
> > +       }
> > +
> > +       i = (unsigned int *) tbl_data;
> > +       vleft = table->maxlen / sizeof(*i);
> > +
> > +       /*
> > +        * Arrays are not supported, keep this simple. *Do not* add
> > +        * support for them.
> > +        */
> > +       if (vleft != 1) {
> > +               *lenp = 0;
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (!conv)
> > +               conv = do_proc_douintvec_conv;
> > +
> > +       if (write)
> > +               return do_proc_douintvec_w(i, table, buffer, lenp, ppos,
> > +                                          conv, data);
> > +       return do_proc_douintvec_r(i, buffer, lenp, ppos, conv, data);
> > +}
> 
> I like the split of read/write. That makes things easier to review.

Great.

  Luis