Re: [PATCH next] sysctl: add proc_dointvec_jiffies_minmax to limit the min/max write value

[Kees Cook <keescook@chromium.org>]

On Wed, May 15, 2019 at 10:53:55PM +0800, Zhiqiang Liu wrote:
> Friendly ping...
> 
> 在 2019/4/24 12:04, Zhiqiang Liu 写道:
> > 
> > Friendly ping...

Hi!

(Please include akpm on CC for next versions of this, as he's likely
the person to take this patch.)

> > 
> >> From: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> >>
> >> In proc_dointvec_jiffies func, the write value is only checked
> >> whether it is larger than INT_MAX. If the write value is less
> >> than zero, it can also be successfully writen in the data.

This appears to be "be design", but I see many "unsigned int" users
that might be tricked into giant values... (for example, see
net/netfilter/nf_conntrack_standalone.c)

Should proc_dointvec_jiffies() just be fixed to disallow negative values
entirely? Looking at the implementation, it seems to be very intentional
about accepting negative values.

However, when I looked through a handful of proc_dointvec_jiffies()
users, it looks like they're all expecting a positive value. Many in the
networking subsystem are, in fact, writing to unsigned long variables,
as I mentioned.

Are there real-world cases of wanting to set a negative jiffie value
via proc_dointvec_jiffies()?

> >>
> >> However, in some scenarios, users would adopt the data to
> >> set timers or check whether time is expired. Generally, the data
> >> will be cast to an unsigned type variable, then the negative data
> >> becomes a very large unsigned value, which leads to long waits
> >> or other unpredictable problems.
> >>
> >> Here, we add a new func, proc_dointvec_jiffies_minmax, to limit the
> >> min/max write value, which is similar to the proc_dointvec_minmax func.
> >>
> >> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> >> Reported-by: Qiang Ning <ningqiang1@huawei.com>
> >> Reviewed-by: Jie Liu <liujie165@huawei.com>

If proc_dointvec_jiffies() can't just be fixed, where will the new
function get used? It seems all the "unsigned int" users could benefit.

-- 
Kees Cook