Re: [PATCH 2/2] kernel: use sysctl shared variables for range check
From: Kees Cook
Date: Wed Apr 10 2019 - 17:51:10 EST
On Wed, Apr 10, 2019 at 12:24 PM Matteo Croce <mcroce@xxxxxxxxxx> wrote:
> On Wed, Apr 10, 2019 at 8:46 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> > On Mon, Apr 8, 2019 at 3:09 PM Matteo Croce <mcroce@xxxxxxxxxx> 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
> 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.*&(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.*&(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.