linux-kernel - Re: Re: Re: [PATCH] proc/sysctl: fix return error forproc_doulongvec

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>] [day] [month] [year] [list]
Message-ID: <20181205180818.GA28501@garbanzo.do-not-panic.com>
Date:   Wed, 5 Dec 2018 10:08:18 -0800
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     cheng.lin130@....com.cn
Cc:     keescook@...omium.org, akpm@...ux-foundation.org,
        ebiederm@...ssion.com, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, zhong.weidong@....com.cn,
        wang.yi59@....com.cn
Subject: Re: Re: Re: [PATCH] proc/sysctl: fix return error
 forproc_doulongvec_minmax

On Wed, Dec 05, 2018 at 03:10:07PM +0800, cheng.lin130@....com.cn wrote:
> > On Mon, Dec 03, 2018 at 01:12:39PM +0800, cheng.lin130@....com.cn wrote:
> > > >Cheng, thanks for the patch!
> > > >
> > > >On Fri, Nov 30, 2018 at 02:35:17PM +0800, Cheng Lin wrote:
> > > >> If the number of input parameters is less than the total
> > > >> parameters, an INVAL error will be returned.
> > > >
> > > >Do you mean EINVAL?
> > > >
> > > Yes, it's EINVAL.
> > 
> > Please adjust the commit log.
> > 
> > > >> This patch ensure no error returned in this condition, just
> > > >> like other interfaces do.
> > > >
> > > >Have an actual example to reproduce?
> > > >
> > > >Luis
> > > >
> > > We use proc_doulongvec_minmax to pass up to two parameters with kern_table.
> > > e.g.
> >         {
> > >                 .procname       = "monitor_signals",
> > >                 .data           = &monitor_sigs,
> > >                 .maxlen         = 2*sizeof(unsigned long),
> > >                 .mode           = 0644,
> > >                 .proc_handler   = proc_doulongvec_minmax,
> > >         },
> > >
> > > Reproduce:
> > > When passing two parameters, it's work normal. But passing only one parameter, an error "Invalid argument"(EINVAL) is returned.
> > > [root@...50 ~]# echo 1 2 > /proc/sys/kernel/monitor_signals
> > > [root@...50 ~]# cat /proc/sys/kernel/monitor_signals
> > > 1       2
> > > [root@...50 ~]# echo 3 > /proc/sys/kernel/monitor_signals
> > > -bash: echo: write error: Invalid argument
> > > [root@...50 ~]# echo $?
> > > 1
> > > [root@...50 ~]# cat /proc/sys/kernel/monitor_signals
> > > 3       2
> > > [root@...50 ~]#
> > >
> > > The following is the result after apply this patch. No error is returned when the number of input parameters is less than the total parameters.
> > > [root@...50 ~]# echo 1 2 > /proc/sys/kernel/monitor_signals
> > > [root@...50 ~]# cat /proc/sys/kernel/monitor_signals
> > > 1       2
> > > [root@...50 ~]# echo 3 > /proc/sys/kernel/monitor_signals
> > > [root@...50 ~]# echo $?
> > > 0
> > > [root@...50 ~]# cat /proc/sys/kernel/monitor_signals
> > > 3       2
> > > [root@...50 ~]#
> > 
> > This would be good to have in the commit log as well. But your patch
> > only addresses one of the proc users, there are a few other checks like
> > this that would also need to be expanded for this. So please expand
> > your patch to cover the other cases as well.
> 
> I have done the check for the interfaces exported in kernel/sysctl.c.
> EXPORT_SYMBOL(proc_dointvec);
> EXPORT_SYMBOL(proc_douintvec);
> EXPORT_SYMBOL(proc_dointvec_jiffies);
> EXPORT_SYMBOL(proc_dointvec_minmax);
> EXPORT_SYMBOL_GPL(proc_douintvec_minmax);
> EXPORT_SYMBOL(proc_dointvec_userhz_jiffies);
> EXPORT_SYMBOL(proc_dointvec_ms_jiffies);
> EXPORT_SYMBOL(proc_dostring);
> EXPORT_SYMBOL(proc_doulongvec_minmax);
> EXPORT_SYMBOL(proc_doulongvec_ms_jiffies_minmax);
> 
> The function call relationship is as follows. There are three processing functions dealing with digital parameters, __do_proc_dointvec/__do_proc_douintvec/__do_proc_doulongvec_minmax.
> 
> proc_dointvec------------------------------|
> proc_dointvec_jiffies----------------------|
> proc_dointvec_minmax------------------|
> proc_dointvec_userhz_jiffies------------|
> proc_dointvec_ms_jiffies-----------------|-> do_proc_dointvec----|-> __do_proc_dointvec
> 
> proc_douintvec-----------------------------|
> proc_douintvec_minmax-----------------|-> do_proc_douintvec---|->	__do_proc_douintvec
> 
> proc_doulongvec_minmax---------------|
> proc_doulongvec_ms_jiffies_minmax--|-> do_proc_doulongvec_minmax----|-> __do_proc_doulongvec_minmax

OK

> This patch deals with  __do_proc_doulongvec_minmax, just as
> __do_proc_dointvec does, adding a check for parameters 'left'. In
> __do_proc_douintvec, its code implementation explicitly does not
> support multiple inputs.  static int __do_proc_douintvec(...){
> 		...
>         /*
>          * Arrays are not supported, keep this simple. *Do not* add
>          * support for them.
>          */
>         if (vleft != 1) {
>                 *lenp = 0;
>                 return -EINVAL;
> 		...
> 		}
> 
> 		
> So, just __do_proc_doulongvec_minmax has the problem.  And most use of
> proc_doulongvec_minmax/proc_doulongvec_ms_jiffies_minmax just have one
> parameter.

The above text, up to my OK, is useful information for the commit log,
please add that.

> It's well hidden.

You mean that the issue is not widely spread? If so please add that
comment to the commit log, and resubmit a v2.

  Luis

> Typical multi-parameter applications for
> proc_dointvec, such as /proc/sys/kernel/printk.