Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Christian Brauner <christian@brauner.io>]

On Mon, Oct 15, 2018 at 02:20:15PM -0700, Kees Cook wrote:
> On Mon, Oct 15, 2018 at 9:28 AM, Christian Brauner <christian@brauner.io> wrote:
> > On Mon, Oct 15, 2018 at 09:11:51AM -0700, Kees Cook wrote:
> >> On Mon, Oct 15, 2018 at 3:55 AM, Christian Brauner <christian@brauner.io> wrote:
> >> > Currently, when writing
> >> >
> >> > echo 18446744073709551616 > /proc/sys/fs/file-max
> >> >
> >> > /proc/sys/fs/file-max will overflow and be set to 0. That quickly
> >> > crashes the system.
> >> > This commit explicitly caps the value for file-max to ULONG_MAX.
> >> >
> >> > Note, this isn't technically necessary since proc_get_long() will already
> >> > return ULONG_MAX. However, two reason why we still should do this:
> >> > 1. it makes it explicit what the upper bound of file-max is instead of
> >> >    making readers of the code infer it from proc_get_long() themselves
> >> > 2. other tunebles than file-max may want to set a lower max value than
> >> >    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
> >> >    such cases too
> >> >
> >> > Cc: Kees Cook <keescook@chromium.org>
> >> > Signed-off-by: Christian Brauner <christian@brauner.io>
> >> > ---
> >> > v0->v1:
> >> > - if max value is < than ULONG_MAX use max as upper bound
> >> > - (Dominik) remove double "the" from commit message
> >> > ---
> >> >  kernel/sysctl.c | 4 ++++
> >> >  1 file changed, 4 insertions(+)
> >> >
> >> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> >> > index 97551eb42946..226d4eaf4b0e 100644
> >> > --- a/kernel/sysctl.c
> >> > +++ b/kernel/sysctl.c
> >> > @@ -127,6 +127,7 @@ static int __maybe_unused one = 1;
> >> >  static int __maybe_unused two = 2;
> >> >  static int __maybe_unused four = 4;
> >> >  static unsigned long one_ul = 1;
> >> > +static unsigned long ulong_max = ULONG_MAX;
> >> >  static int one_hundred = 100;
> >> >  static int one_thousand = 1000;
> >> >  #ifdef CONFIG_PRINTK
> >> > @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
> >> >                 .maxlen         = sizeof(files_stat.max_files),
> >> >                 .mode           = 0644,
> >> >                 .proc_handler   = proc_doulongvec_minmax,
> >> > +               .extra2         = &ulong_max,
> >>
> >> Don't we want this capped lower? The percpu comparisons, for example,
> >> are all signed long. And there is at least this test, which could
> >> overflow:
> >>
> >>         if (atomic_long_read(&unix_nr_socks) > 2 * get_max_files())
> >>                 goto out;
> >
> > Does that check even make sense?
> > Commit 518de9b39e854542de59bfb8b9f61c8f7ecf808b made get_max_files()
> > return a long to bump the number of allowed files to more than 2^31.
> >
> > But assuming a platform where an unsigned long is 64bit which is what
> > get_max_files() returns and atomic_long_read() is 64bit too this is
> > guaranteed to overflow, no?  So I'm not clear what this is trying to do.
> > Seems this should simply be:
> >
> > if (atomic_long_read(&unix_nr_socks) > get_max_files())
> >         goto out;
> >
> > or am I missing a crucial point?
> >
> >>
> >> Seems like max-files should be  SLONG_MAX / 2 or something instead?
> >
> > Hm. Isn't that a bit low? Iiuc, this would mean cutting the maximum
> > number of open files in half? If at all shouldn't it be LONG_MAX?
> 
> LONG_MAX would align us with the values in the percpu stuff. I'm
> really not sure what's happening in the sock check, but it's prone to
> an unsigned multiplication overflow, if I'm reading it right. Probably
> should just be a separate bug fix:
> 
> -         if (atomic_long_read(&unix_nr_socks) > 2 * get_max_files())
> +         if (atomic_long_read(&unix_nr_socks) / 2 > get_max_files())

Yeah, as I said before this is just waiting for an overflow and it seems
this bug has existed since the switch to git. The intention apparently
was indeed to allow the number of sockets to be double the open file
limit. However, as it is right now this just overflows. I'll send a
separate patch for this but I'd sugest to do something like:


diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index d1edfa3cad61..7e461f4f0466 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -754,9 +754,12 @@ static struct sock *unix_create1(struct net *net, struct socket *sock, int kern)
 {
 	struct sock *sk = NULL;
 	struct unix_sock *u;
+	static atomic_long_t tmp;
 
 	atomic_long_inc(&unix_nr_socks);
-	if (atomic_long_read(&unix_nr_socks) > 2 * get_max_files())
+	tmp = atomic_long_read(&unix_nr_socks);
+	do_div(tmp, 2);
+	if (tmp > get_max_files())
 		goto out;
 
 	sk = sk_alloc(net, PF_UNIX, GFP_KERNEL, &unix_proto, kern);

> 
> -Kees
> 
> >
> >>
> >> >         },
> >> >         {
> >> >                 .procname       = "nr_open",
> >> > @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> >> >                                 break;
> >> >                         if (neg)
> >> >                                 continue;
> >> > +                       if (max && val > *max)
> >> > +                               val = *max;
> >> >                         val = convmul * val / convdiv;
> >> >                         if ((min && val < *min) || (max && val > *max))
> >> >                                 continue;
> >> > --
> >> > 2.17.1
> >> >
> >>
> >> -Kees
> >>
> >> --
> >> Kees Cook
> >> Pixel Security
> 
> 
> 
> -- 
> Kees Cook
> Pixel Security