linux-kernel - Re: [PATCH 2/2] kernel: use sysctl shared variables for range 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXu5j+0sAc2fHTuYwYTLoHPDbnfxYuAEJ+tRfgc2Z5DDqp9Jg@mail.gmail.com>
Date:   Wed, 10 Apr 2019 14:50:53 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Matteo Croce <mcroce@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Alexey Dobriyan <adobriyan@...il.com>
Subject: Re: [PATCH 2/2] kernel: use sysctl shared variables for range check

On Wed, Apr 10, 2019 at 12:24 PM Matteo Croce <mcroce@...hat.com> wrote:
>
> On Wed, Apr 10, 2019 at 8:46 PM Kees Cook <keescook@...omium.org> wrote:
> >
> > On Mon, Apr 8, 2019 at 3:09 PM Matteo Croce <mcroce@...hat.com> wrote:
> > >
> > > Use the shared variables for range check, instead of declaring a local one
> > > in every source file.
> >
> > I was expecting this to be a tree-wide change for all the cases found
> > by patch 1's "git grep".
> >
>
> Hi Kees,
>
> I have already the whole patch ready, but I was frightened by the
> output of get_maintainer.pl, so I decided to split the patch into
> small pieces and send the first one.

Heh, sounds fine. Normally the big tree-wide changes go via Linus just
before cutting rc1 (or rc2). This is "only" 31 source files, though,
so maybe akpm wants to take these instead? Andrew, how do you feel
about that?

> Patches for /proc/sys/net and drivers/ are pretty big, and can be
> merged after the 1/2 inclusion.
>
> > Slight change to the grep for higher accuracy:
> >
> > $ git grep -E '\.extra[12].*&(zero|one|int_max)\b' |wc -l
> > 245
> >
>
> Right, my regexp wrongly matches also one_hundred, one_jiffy, etc.
> Anywqay, I did the changes by hand, so apart the commit message, the
> content should be safe.
>
> > Only 31 sources:
> > $ git grep -E '\.extra[12].*&(zero|one|int_max)\b' | cut -d: -f1 |
> > sort -u > /tmp/list.txt
> > $ wc -l /tmp/list.txt
> > 31
> >
> > One thing I wonder about is if any of these cases depend on the extra
> > variable being non-const (many of these are just "static int").
> >
> > $ egrep -H '\b(zero|one|int_max)\b.*=' $(cat /tmp/list.txt) | grep -v static
> >
> > Looks like none, so it'd be safe. How about doing this tree-wide for
> > all 31 cases? (Coccinelle might be able to help.)
>
> It could be true for other sysctl values like
> xpc_disengage_max_timelimit or fscache_op_wq, but it's very unlikely
> that someone writes, for example, 5 into a variable named "zero". If
> it does, it most likely a bug, so const is our friend.

I completely agree. :) I just wanted to make sure and it turned out
the grep was very easy.

-- 
Kees Cook