linux-kernel - Re: usermode-helper code oddity query..

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] [day] [month] [year] [list]
Date:   Fri, 3 Mar 2023 15:07:01 -0800
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Eric Paris <eparis@...hat.com>,
        David Howells <dhowells@...hat.com>,
        Serge Hallyn <serge@...lyn.com>,
        "Andrew G. Morgan" <morgan@...nel.org>,
        James Morris <jmorris@...ei.org>
Cc:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Christoph Hellwig <hch@....de>,
        Kees Cook <keescook@...omium.org>,
        Iurii Zaikin <yzaikin@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: usermode-helper code oddity query..

On Thu, Mar 02, 2023 at 03:44:17PM -0800, Linus Torvalds wrote:
> So this is a bit out of the blue, but I cleaned up some really old
> legacy capability code in commit f122a08b197d ("capability: just use a
> 'u64' instead of a 'u32[2]' array") and in the process I became the
> last person to touch kernel/umh.c.
> 
> Tag, I'm clearly it. Not that I want to take that glory away from
> PeterZ, who was the previous last person to touch that code. In fact,
> I'm just cc'ing everybody who has been touching that file at all in
> the last years, and a few /proc sysctl maintainers too.
> 
> Anyway, I wanted to try to keep the capability code cleanups clear,
> and really only touched the data structure conversion, but I'm just
> left staring at that code and wondering why we have those odd CAP_BSET
> / CAP_PI dummy pointers. They've been there since the whole /proc
> interface was introduced, but they seem strangely pointless.

Actually that seems to have come from Eric Paris on v3.0 via commit 17f60a7da150f
("capabilites: allow the application of capability limits to usermode helpers")

mcgrof@...ton ~/linux (git::master)$ git describe --contains 17f60a7da150f
v3.0-rc1~309^2~1^2~12

> It would _seem_ like it would be a lot simpler and more
> straightforward to just put the actual pointer to the usermodehelper
> capability in there instead, and not have that odd fake pointer
> enumeration at all.

Agreed.

> IOW, I'm wondering what's wrong with a patch like the attached. I
> might be missing something.

Yes, the only thing I think think of is that at first it just seemed
like a good way to abstract access to usage of the same routine for
two separate variables. I can't really see *why* its done that way
though.

The only thing I can think of is perhaps it was a sort of defensive
mechanism back from the days we had tons of sysctls on kernel/sysctl.c
large kitchen sink to prevent someone from thinking they could use
proc_cap_handler() for other variables. That file used to be hell.

> I also would have like that array to be an array of "u32" rather than
> "unsigned long" (because that is, sadly, the interface we have, like
> it or not), but we don't seem to have a proc_dou32vec_minmax(). I
> guess "uint" is the same thing, but it's not pretty. Anyway, that's a
> separate and independent issue from this.
> 
> And no, none of this is important. Just random cleanup of code I
> happened to look at for other reasons.
> 
>            Linus

>  kernel/umh.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/umh.c b/kernel/umh.c
> index 2a4708277335..60aa9e764a38 100644
> --- a/kernel/umh.c
> +++ b/kernel/umh.c
> @@ -32,9 +32,6 @@
>  
>  #include <trace/events/module.h>
>  
> -#define CAP_BSET	(void *)1
> -#define CAP_PI		(void *)2
> -
>  static kernel_cap_t usermodehelper_bset = CAP_FULL_SET;
>  static kernel_cap_t usermodehelper_inheritable = CAP_FULL_SET;
>  static DEFINE_SPINLOCK(umh_sysctl_lock);
> @@ -512,16 +509,11 @@ static int proc_cap_handler(struct ctl_table *table, int write,
>  	/*
>  	 * convert from the global kernel_cap_t to the ulong array to print to
>  	 * userspace if this is a read.
> +	 *
> +	 * Legacy format: capabilities are exposed as two 32-bit values
>  	 */
> +	cap = table->data;
>  	spin_lock(&umh_sysctl_lock);
> -	if (table->data == CAP_BSET)
> -		cap = &usermodehelper_bset;
> -	else if (table->data == CAP_PI)
> -		cap = &usermodehelper_inheritable;
> -	else
> -		BUG();
> -
> -	/* Legacy format: capabilities are exposed as two 32-bit values */
>  	cap_array[0] = (u32) cap->val;
>  	cap_array[1] = cap->val >> 32;
>  	spin_unlock(&umh_sysctl_lock);
> @@ -555,14 +547,14 @@ static int proc_cap_handler(struct ctl_table *table, int write,
>  struct ctl_table usermodehelper_table[] = {
>  	{
>  		.procname	= "bset",
> -		.data		= CAP_BSET,
> +		.data		= &usermodehelper_bset,
>  		.maxlen		= 2 * sizeof(unsigned long),
>  		.mode		= 0600,
>  		.proc_handler	= proc_cap_handler,
>  	},
>  	{
>  		.procname	= "inheritable",
> -		.data		= CAP_PI,
> +		.data		= &usermodehelper_inheritable,
>  		.maxlen		= 2 * sizeof(unsigned long),
>  		.mode		= 0600,
>  		.proc_handler	= proc_cap_handler,

Feel free to add:

Reviewed-by: Luis Chamberlain <mcgrof@...nel.org>

  Luis