linux-kernel - Re: [PATCH] sysctl: add proper unsigned int support

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: <20170201195646.GG24047@wotan.suse.de>
Date:   Wed, 1 Feb 2017 20:56:46 +0100
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     Alexey Dobriyan <adobriyan@...il.com>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Ingo Molnar <mingo@...nel.org>, Mel Gorman <mgorman@...e.de>,
        Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>,
        jeyu@...hat.com, Rusty Russell <rusty@...tcorp.com.au>,
        matt@...eblueprint.co.uk, bp@...e.de,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>, shuah@...nel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kselftest@...r.kernel.org,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        Heinrich Schuchardt <xypron.glpk@....de>,
        Kees Cook <keescook@...omium.org>,
        "David S. Miller" <davem@...emloft.net>,
        Ingo Molnar <mingo@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Julia Lawall <julia.lawall@...6.fr>
Subject: Re: [PATCH] sysctl: add proper unsigned int support

On Mon, Jan 30, 2017 at 03:56:20PM +0300, Alexey Dobriyan wrote:
> On Sun, Jan 29, 2017 at 10:29 PM, Luis R. Rodriguez <mcgrof@...nel.org> wrote:
> > Commit e7d316a02f6838 ("sysctl: handle error writing UINT_MAX to u32 fields")
> > added proc_douintvec() to start help adding support for unsigned int,
> > this however was only half the work needed, all these issues are present
> > with the current implementation:
> >
> >   o Printing the values shows a negative value, this happens
> >     since do_proc_dointvec() and this uses proc_put_long()
> >   o We can easily wrap around the int values: UINT_MAX is
> >     4294967295, if we echo in 4294967295 + 1 we end up with 0,
> >     using 4294967295 + 2 we end up with 1.
> >  o We echo negative values in and they are accepted
> >
> > Fix all these issues by adding our own do_proc_douintvec().
> >
> > Cc: Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
> > Cc: Heinrich Schuchardt <xypron.glpk@....de>
> > Cc: Kees Cook <keescook@...omium.org>
> > Cc: "David S. Miller" <davem@...emloft.net>
> > Cc: Ingo Molnar <mingo@...hat.com>
> > Cc: Andrew Morton <akpm@...ux-foundation.org>
> > Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Fixes: e7d316a02f68 ("sysctl: handle error writing UINT_MAX to u32 fields")
> > Signed-off-by: Luis R. Rodriguez <mcgrof@...nel.org>
> > ---
> >
> > I split this off as its own atomic fix from a larger RFC series [0].
> > I've only provided the fix here, and split off further functionality
> > into a separate patch for the future. Although this is a fix I don't think
> > its super critical, and specially due to its size do not think it can
> > be stable material.
> >
> > I do have proc_douintvec_minmax() but since we have no users for it
> > it can wait until I add something that makes use of it. If someone
> > needs it now though please let me know.
> >
> > Likewise adding proc_douintvec_minmax_sysadmin() is very trivial but I have no
> > immediate users for it so it can wait even longer.
> >
> > [0] https://lkml.kernel.org/r/20161208184801.1689-1-mcgrof@kernel.org
> >
> >  kernel/sysctl.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 115 insertions(+), 6 deletions(-)
> >
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index 8dbaec0e4f7f..118341d3a139 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -2125,12 +2125,12 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
> >         return 0;
> >  }
> >
> > -static int do_proc_douintvec_conv(bool *negp, unsigned long *lvalp,
> > -                                int *valp,
> > -                                int write, void *data)
> > +static int do_proc_douintvec_conv(unsigned long *lvalp,
> > +                                 unsigned int *valp,
> > +                                 int write, void *data)
> >  {
> >         if (write) {
> > -               if (*negp)
> > +               if (*lvalp > (unsigned long) UINT_MAX)
> 
> Cast is unnecessary here.

Fixed, thanks!

> > +static int __do_proc_douintvec(void *tbl_data, struct ctl_table *table,
> 
> > +       for (; left && vleft--; i++, first=false) {
> 
> I'd suggest to not implement "array of unsigned int" unless
> such sysctl already exists. Much of the complexity arises from this case.

Uh, yeah that is such crap.

As much as I'd like to I'm afraid the problem with this is there
may be array int setups which should / could be ported to unsigned
int. I tried to do a grammatical search with Coccinelle but ran into
issues, I'll follow up with Julia about that. A manual search however
reveals one so far:

ipv4_local_port_range(). There may be others. So, do we deal with the
old cases by leaving them as-is or provide a new separate interface?
I take it the array stuff is long tested, so don't the harm in providing
similar functionality for unsigned int. I'm perfectly happy in just
ripping out unsigned int array support though. Who makes this call ?

  Luis