[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <B6E895DC-7425-43F4-BB4B-1F8499CCD952@brauner.io>
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.
Powered by blists - more mailing lists