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

["Serge E. Hallyn" <serge@hallyn.com>]

On Fri, Oct 31, 2025 at 12:06:47PM +0100, Thorsten Blum 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);

I dunno, I'm not saying I would say no to this, but it does look
a little ridiculous.  If it used some macro version which computes
the length of str instead of typing in MAJMINLEN, that might be
better.  But we're just as likely here to get MAJMINLEN out of
sync with the length of strscpy as anything else happening, and
all to copy over two bytes.

>  	else
> -		sprintf(str, "%u", m);
> +		snprintf(str, MAJMINLEN, "%u", m);

Here, to me it always looks suspect to use snprintf without
checking its return value, and in this case, if snprintf cuts
off the value it is in fact a potential security issue, one
which did not exist before this.

So make up your mind: is it worth doing the length check or
not?  If not, then this switch is uncalled for.  If so, then
you should check the return value, else one day I may be able
to whitelist a device which the admin didn't allow.

>  }
>  
>  static int devcgroup_seq_show(struct seq_file *m, void *v)
> -- 
> 2.51.1