Re: overflow on proc_nr_files

[Christian Brauner]

On October 11, 2018 7:18:15 PM GMT+02:00, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>On Thu, Oct 11, 2018 at 7:10 AM, Christian Brauner
><christian@xxxxxxxxxx> 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.