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

[Christian Brauner]

On Tue, Oct 16, 2018 at 11:53:53AM -0400, Waiman Long wrote:
> On 10/16/2018 11:47 AM, Christian Brauner wrote:
> > On Tue, Oct 16, 2018 at 11:44:40AM -0400, Waiman Long wrote:
> >> On 10/16/2018 11:40 AM, Christian Brauner wrote:
> >>> On Tue, Oct 16, 2018 at 11:34:07AM -0400, Waiman Long wrote:
> >>>> On 10/16/2018 11:29 AM, Christian Brauner wrote:
> >>>>> On Tue, Oct 16, 2018 at 11:25:42AM -0400, Waiman Long wrote:
> >>>>>> On 10/16/2018 11:21 AM, Christian Brauner wrote:
> >>>>>>> On Tue, Oct 16, 2018 at 11:13:28AM -0400, Waiman Long wrote:
> >>>>>>>> On 10/15/2018 06:55 AM, Christian Brauner 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@xxxxxxxxxxxx>
> >>>>>>>>> Signed-off-by: Christian Brauner <christian@xxxxxxxxxx>
> >>>>>>>>> ---
> >>>>>>>>> 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,
> >>>>>>>> What is the point of having a maximum value of ULONG_MAX anyway? No
> >>>>>>>> value you can put into a ulong type can be bigger than that.
> >>>>>>> This is changed in the new code to LONG_MAX. See the full thread for
> >>>>>>> context. There's also an additional explantion in the commit message.
> >>>>>>>
> >>>>>>>>>  	},
> >>>>>>>>>  	{
> >>>>>>>>>  		.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;
> >>>>>>>> This does introduce a change in behavior. Previously the out-of-bound
> >>>>>>>> value is ignored, now it is capped at its maximum. This is a
> >>>>>>>> user-visible change.
> >>>>>>> Not completely true though. Try
> >>>>>>>
> >>>>>>> echo 18446744073709551616 > /proc/sys/fs/file-max
> >>>>>>>
> >>>>>>> on a system you find acceptable loosing.
> >>>>>>> So this is an acceptable user-visible change I'd say. But I'm open to
> >>>>>>> other suggestions.
> >>>>>> I am not saying this is unacceptable. I just say this is a user-visible
> >>>>>> change and so should be documented somehow. BTW, you cap the max value,
> >>>>> Sure, I'll update linux manpages and I can CC stable on the next round.
> >>>>>
> >>>>>> but not the min value. So there is inconsistency. I would say you either
> >>>>>> do both, or none of them.
> >>>>> The min value is 0. I don't think it needs to be set explicitly. I just
> >>>>> kept the max value because it is != ULONG_MAX but LONG_MAX for file-max.
> >>>> I think you are making the change with just one use case in mind. This
> >>>> is a generic function that can be used by many different callers. So any
> >>>> change you make has to be applicable to all use cases. You just can't
> >>>> assume min is always 0 in all the other use cases.
> >>> So, any caller that calls {do_}proc_doulongvec_minmax() must want an
> >>> unsigned long lest they are calling the wrong function.
> >>> The smallest value for an unsigned long is 0. So if any caller wants to
> >>> get into a situation where the caller needs to be capped they need to be
> >>> able to set the value to lower than 0 which they can't since they are
> >>> requesting an unsigned. So I'm not sure it makes sense.
> >>>
> >>> Christian
> >> There may be use cases where the developer may want a min value that is
> >> bigger than 0. As I said, you can't just make an assumption here.
> > Sorry, I'm not following your argument anymore. Why exactly is this
> > function not already handling this case and what exactly does break with
> > my changs?  And what exactly do we gain from setting file-max to extra1
> > = 0?
> 
> OK, I am not talking about the file-max case. Your patch caps the
> maximum, but not the minimum. This is inconsistent as users will see
> different behavior depending on whether the input value is bigger than
> the maximum or smaller than the minimum.

So the difference for me is that without the max cap we run into
overflow issues because you can in fact overflow since some callers are
long int but doulongvec is unsigned long but we can't really run into
underflow issues since it is an unsigned long and proc_get_long() is
handling the case where there's a leading '-' correctly.
So there's no need to cap and change behavior there was well.
But I'll send out another version and we'll see what other people think
of this. We can always make this change in the next version.

> 
> Cheers,
> Longman