linux-kernel - Re: overflow on proc_nr

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] [day] [month] [year] [list]

Date:   Fri, 12 Oct 2018 00:29:35 +0200
From:   Christian Brauner <christian@...uner.io>
To:     Kees Cook <keescook@...omium.org>
CC:     LKML <linux-kernel@...r.kernel.org>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        Waiman Long <longman@...hat.com>
Subject: Re: overflow on proc_nr_files

On October 11, 2018 7:18:15 PM GMT+02:00, Kees Cook <keescook@...omium.org> wrote:
>On Thu, Oct 11, 2018 at 7:10 AM, Christian Brauner
><christian@...uner.io> wrote:
>> Hey,
>>
>> I've just got pinged by Lennart who discovered that you can get your
>> system into an unuseable state by writing something that exceeds a
>s64
>> into /proc/sys/fs/file-max. Say,
>>
>> echo 20000000000000000000 > /proc/sys/fs/file-max
>>
>> which will trigger an overflow and percpu_counter_read_positive()
>will
>> return 0 and cat /proc/sys/fs/file-max will return 0.
>>
>> That effectively means you write that number and it succeeds and all
>is
>> well and a few seconds/minutes later your system just dies or gets
>into
>> an unuseable state pretty quickly
>>
>> I wonder if we shouldn't accept overflows or - if we have no way in
>this
>> codepath to detect them - set it to some pre-defined hard-coded
>value.
>>
>> Or maybe this is even a known issue and by design but before I work
>on a
>> patch here I just wanted to check.
>
>There was work done recently to keep proc_dointvec_minmax from
>wrapping, but it seems that the problem here is that file-max uses
>proc_doulongvec_minmax, so it explicitly thinks it can be larger than
>s64. (And max_files itself is unsigned long...)
>
>It looks like the counter is expected to be a long, not unsigned:
>
>static long get_nr_files(void)
>{
>        return percpu_counter_read_positive(&nr_files);
>}
>
>And there are places where this goes weird:
>
>                if (percpu_counter_sum_positive(&nr_files) >=
>files_stat.max_files)
>
>etc.
>
>It seems like maybe the sysctl needs to be explicitly capped in
>kernel/sysctl.c to S64_MAX?

Yeah, either that or (I need to look at the
code again though if that's possible.) 
return EOVERFLOW but provide a way 
(macro) for userspace to know what the 
maximum value is that this can be set to.

I can send a patch and we can take it 
from there.