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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <201905150945.C9D1F811F@keescook>
Date:   Wed, 15 May 2019 10:06:09 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Zhiqiang Liu <liuzhiqiang26@...wei.com>
Cc:     mcgrof@...nel.org, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, ebiederm@...ssion.com,
        pbonzini@...hat.com, viro@...iv.linux.org.uk, adobriyan@...il.com,
        mingfangsen@...wei.com, wangxiaogang3@...wei.com,
        "Zhoukang (A)" <zhoukang7@...wei.com>, netdev@...r.kernel.org,
        akpm@...ux-foundation.org
Subject: Re: [PATCH next] sysctl: add proc_dointvec_jiffies_minmax to limit
 the min/max write value

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@...wei.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@...wei.com>
> >> Reported-by: Qiang Ning <ningqiang1@...wei.com>
> >> Reviewed-by: Jie Liu <liujie165@...wei.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