linux-kernel - Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set

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]

Message-ID: <20251031165417.4490941a@pumpkin>
Date: Fri, 31 Oct 2025 16:54:17 +0000
From: David Laight <david.laight.linux@...il.com>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
 "Serge E. Hallyn" <serge@...lyn.com>,
 linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin

On Fri, 31 Oct 2025 16:23:02 +0100
Thorsten Blum <thorsten.blum@...ux.dev> wrote:

> On 31. Oct 2025, at 13:59, David Laight wrote:
> > Even if ex->major can be ~0 there are much cleaner ways of writing this code.  
> 
> Thanks for pointing this out. Looking at the bigger picture makes it
> clear that most of the code can actually be removed. What do you think
> of this change?

That is sort of what I was thinking about, but it doesn't quite work.

> 
> Thanks,
> Thorsten
> 
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index a41f558f6fdd..cb845b1fad6b 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -244,7 +244,6 @@ static void devcgroup_css_free(struct cgroup_subsys_state *css)
> #define DEVCG_DENY 2
> #define DEVCG_LIST 3
> 
> -#define MAJMINLEN 13
> #define ACCLEN 4
> 
> static void set_access(char *acc, short access)
> @@ -270,19 +269,11 @@ static char type_to_char(short type)
> 	return 'X';
> }
> 
> -static void set_majmin(char *str, unsigned m)
> -{
> -	if (m == ~0)
> -		strscpy(str, "*", MAJMINLEN);
> -	else
> -		snprintf(str, MAJMINLEN, "%u", m);
> -}
> -
> static int devcgroup_seq_show(struct seq_file *m, void *v)
> {
> 	struct dev_cgroup *devcgroup = css_to_devcgroup(seq_css(m));
> 	struct dev_exception_item *ex;
> -	char maj[MAJMINLEN], min[MAJMINLEN], acc[ACCLEN];
> +	char acc[ACCLEN];
> 
> 	rcu_read_lock();
> 	/*
> @@ -293,17 +284,12 @@ static int devcgroup_seq_show(struct seq_file *m, void *v)
> 	 */
> 	if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
> 		set_access(acc, DEVCG_ACC_MASK);
> -		set_majmin(maj, ~0);
> -		set_majmin(min, ~0);
> -		seq_printf(m, "%c %s:%s %s\n", type_to_char(DEVCG_DEV_ALL),
> -			   maj, min, acc);
> +		seq_printf(m, "%c *:* %s\n", type_to_char(DEVCG_DEV_ALL), acc);

type_to_char(DEVCG_DEV_ALL) is 'a' and this is the only place it happens,
also acc is "rwm".
So that could be:
		seq_puts(m, "a *:* rwm\n");

> 	} else {
> 		list_for_each_entry_rcu(ex, &devcgroup->exceptions, list) {
> 			set_access(acc, ex->access);
> -			set_majmin(maj, ex->major);
> -			set_majmin(min, ex->minor);
> -			seq_printf(m, "%c %s:%s %s\n", type_to_char(ex->type),
> -				   maj, min, acc);
> +			seq_printf(m, "%c %u:%u %s\n", type_to_char(ex->type),
> +				   ex->major, ex->minor, acc);

It looks like both ex->major and ex->minor can be ~0.
(I'm not sure it makes any sense to have major == ~0 and minor != ~0).
However this should be ok:
			seq_putc(m, type_to_char(ex->type);
			if (ex->major == ~0)
				seq_puts(m, " *:");
			else
				seq_printf(m, " %u:", ex->major);
			if (ex->minor == ~0)
				seq_puts(m, "* ");
			else
				seq_printf(m, "%u ", ex->minor);
			if (ex->access & DEVCG_ACC_READ)
				seq_putc(m, 'r');
			if (ex->access & DEVCG_ACC_WRITE)
				seq_putc(m, 'w');
			if (ex->access & DEV_ACC_MKNOD)
				seq_putc(m. 'm');
			seq_putc(m, '\n');

A less intrusive change would be to pass 'm' the the set_xxx() functions
and add the separators between the calls.

	David


> 		}
> 	}
> 	rcu_read_unlock();
>