Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin

[David Laight <david.laight.linux@gmail.com>]

On Fri, 31 Oct 2025 12:06:47 +0100
Thorsten Blum <thorsten.blum@linux.dev> wrote:

> strcpy() is deprecated and sprintf() does not perform bounds checking
> either. While the current code works correctly, strscpy() and snprintf()
> are safer alternatives that follow secure coding best practices.
> 
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  security/device_cgroup.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index dc4df7475081..a41f558f6fdd 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -273,9 +273,9 @@ static char type_to_char(short type)
>  static void set_majmin(char *str, unsigned m)
>  {
>  	if (m == ~0)
> -		strcpy(str, "*");
> +		strscpy(str, "*", MAJMINLEN);
>  	else
> -		sprintf(str, "%u", m);
> +		snprintf(str, MAJMINLEN, "%u", m);
>  }
>  
>  static int devcgroup_seq_show(struct seq_file *m, void *v)

There is no point using sting length limits that aren't passed into the function.

In any case the code seems to be crap (why is 'security' code always bad?)
(See https://elixir.bootlin.com/linux/v6.18-rc3/source/security/device_cgroup.c#L247)
I doubt ex->major or ex->minor can ever be ~0.
So there are two sets of calls, one set passes ~0 and the other doesn't.
The output buffers are then passed into another printf().

Even if ex->major can be ~0 there are much cleaner ways of writing this code.

	David