Re: overflow on proc_nr_files

[Kees Cook]

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?

-Kees

-- 
Kees Cook
Pixel Security