linux-kernel - Re: [PATCH v3] proc/sysctl: add shared variables for range check

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1109DAD2-E25B-47A3-8381-E02260FE51B9@redhat.com>
Date:   Fri, 19 Apr 2019 09:17:17 +0900
From:   Matteo Croce <mcroce@...hat.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
CC:     LKML <linux-kernel@...r.kernel.org>, linux-fsdevel@...r.kernel.org,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH v3] proc/sysctl: add shared variables for range check

On April 19, 2019 7:40:45 AM GMT+09:00, Andrew Morton <akpm@...ux-foundation.org> wrote:
> On Wed, 17 Apr 2019 15:15:31 +0200 Matteo Croce <mcroce@...hat.com>
> wrote:
> 
> > In the sysctl code the proc_dointvec_minmax() function is often used
> to
> > validate the user supplied value between an allowed range. This
> function
> > uses the extra1 and extra2 members from struct ctl_table as minimum
> and
> > maximum allowed value.
> > 
> > On sysctl handler declaration, in every source file there are some
> readonly
> > variables containing just an integer which address is assigned to
> the
> > extra1 and extra2 members, so the sysctl range is enforced.
> > 
> > The special values 0, 1 and INT_MAX are very often used as range
> boundary,
> > leading duplication of variables like zero=0, one=1, int_max=INT_MAX
> in
> > different source files:
> > 
> >     $ git grep -E '\.extra[12].*&(zero|one|int_max)\b' |wc -l
> >     245
> > 
> > This patch adds three const variables for the most commonly used
> values,
> > and use them instead of creating a local one for every object file.
> > 
> > ...
> >
> > --- a/arch/s390/appldata/appldata_base.c
> > +++ b/arch/s390/appldata/appldata_base.c
> > @@ -220,15 +220,13 @@ appldata_timer_handler(struct ctl_table *ctl,
> int write,
> >  			   void __user *buffer, size_t *lenp, loff_t *ppos)
> >  {
> >  	int timer_active = appldata_timer_active;
> > -	int zero = 0;
> > -	int one = 1;
> >  	int rc;
> >  	struct ctl_table ctl_entry = {
> >  		.procname	= ctl->procname,
> >  		.data		= &timer_active,
> >  		.maxlen		= sizeof(int),
> > -		.extra1		= &zero,
> > -		.extra2		= &one,
> > +		.extra1		= (void *)&sysctl_zero,
> > +		.extra2		= (void *)&sysctl_one,
> >  	};
> 
> Still not liking the casts :(
> 
> Did we decide whether making extra1&2 const void*'s was feasible?
> 
> I'm wondering if it would be better to do
> 
> extern const int sysctl_zero;
> /* comment goes here */
> #define SYSCTL_ZERO ((void *)&sysctl_zero)
> 
> and then use SYSCTL_ZERO everywhere.  That centralizes the ugliness
> and
> makes it easier to switch over if/when extra1&2 are constified.
> 
> But it's all a bit sad and lame :( 

No, we didn't decide yet. I need to check for all extra1,2 assignment. Not an impossible task, anyway.

I agree that the casts are ugly. Your suggested macro moves the ugliness in a single point, which is good. Or maybe we can do a single macro like:

#define SYSCTL_VAL(x) ((void *)&sysctl_##x)

to avoid defining one for every value. And when we decide that everything can be const, we just update the macro.

Regards,
-- 
Matteo Croce
per aspera ad upstream