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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aQSzcp852yz/Ourm@mail.hallyn.com>
Date: Fri, 31 Oct 2025 08:02:42 -0500
From: "Serge E. Hallyn" <serge@...lyn.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, 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@...ux.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