Re: [PATCH v2 0/9] sysctl: add and fix proper unsigned int support

[Kees Cook]

On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:
> On this v2 I've taken Alexey's recommendation and looked at array users
> of the proc sysctl interface which complicate the interfece to see if
> we can instead just simplify the unsigned int implementation. I could
> not find any clear candidate. As such I've just ripped out array
> support.
>
> Since some future unsigned int proc sysctl users might think there is
> array support I've taken measures to do sanity checks on initialization
> and warn the kernel if such users creep up. To validate this I ended up
> just writing a simple test driver, and extending our tests. In doing this
> I also found a really old issue with sysctl_check_table(), and yet another
> issue with the first incarnation of proc_douintvec().
>
> I hammered on proc_douintvec() as much as I could, and extended tests for
> this to ensure we don't regress should some int users convert over.
>
> I noticed one more issue but I did not fix as I figured it was worth
> discussing: proc_doi*_minmax() handlers have historically allowed users
> to register even if their own data does not match the expressed min/max
> values. When this happens the value is exposed on /proc/sys but reading
> or writing does not work against it. I'm of the opinion that
> sysctl_check_table() should just validate this and bail preventing such
> entries from ever creeping up. The only reason I didn't do this is this
> *could* mean some tables don't get registered in some cases -- I haven't
> done the vetting. If we're fine with this I can add it later.
>
> Luis R. Rodriguez (9):
>   sysctl: fix lax sysctl_check_table() sanity check
>   sysctl: add proper unsigned int support
>   sysctl: add unsigned int range support
>   test_sysctl: add dedicated proc sysctl test driver
>   test_sysctl: add generic script to expand on tests
>   test_sysctl: test against PAGE_SIZE for int
>   test_sysctl: add simple proc_dointvec() case
>   test_sysctl: add simple proc_douintvec() case
>   test_sysctl: test against int proc_dointvec() array support

Please go ahead and add a MAINTAINERS file entry for the two of us
(and Eric if he wants) for sysctl. We poke at it enough that really we
should declare it maintained (as you suggested privately). For now we
should likely still land it all through akpm, though.

-Kees

-- 
Kees Cook
Pixel Security