* [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
@ 2025-10-31 11:06 Thorsten Blum
2025-10-31 12:59 ` David Laight
2025-10-31 13:02 ` Serge E. Hallyn
0 siblings, 2 replies; 6+ messages in thread
From: Thorsten Blum @ 2025-10-31 11:06 UTC (permalink / raw)
To: Paul Moore, James Morris, Serge E. Hallyn
Cc: Thorsten Blum, linux-security-module, linux-kernel
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)
--
2.51.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
2025-10-31 11:06 [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin Thorsten Blum
@ 2025-10-31 12:59 ` David Laight
2025-10-31 15:23 ` Thorsten Blum
2025-10-31 13:02 ` Serge E. Hallyn
1 sibling, 1 reply; 6+ messages in thread
From: David Laight @ 2025-10-31 12:59 UTC (permalink / raw)
To: Thorsten Blum
Cc: Paul Moore, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
2025-10-31 11:06 [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin Thorsten Blum
2025-10-31 12:59 ` David Laight
@ 2025-10-31 13:02 ` Serge E. Hallyn
1 sibling, 0 replies; 6+ messages in thread
From: Serge E. Hallyn @ 2025-10-31 13:02 UTC (permalink / raw)
To: Thorsten Blum
Cc: Paul Moore, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
2025-10-31 12:59 ` David Laight
@ 2025-10-31 15:23 ` Thorsten Blum
2025-10-31 16:54 ` David Laight
0 siblings, 1 reply; 6+ messages in thread
From: Thorsten Blum @ 2025-10-31 15:23 UTC (permalink / raw)
To: David Laight
Cc: Paul Moore, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
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?
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);
} 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);
}
}
rcu_read_unlock();
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
2025-10-31 15:23 ` Thorsten Blum
@ 2025-10-31 16:54 ` David Laight
2025-11-01 17:00 ` Paul Moore
0 siblings, 1 reply; 6+ messages in thread
From: David Laight @ 2025-10-31 16:54 UTC (permalink / raw)
To: Thorsten Blum
Cc: Paul Moore, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
On Fri, 31 Oct 2025 16:23:02 +0100
Thorsten Blum <thorsten.blum@linux.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();
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
2025-10-31 16:54 ` David Laight
@ 2025-11-01 17:00 ` Paul Moore
0 siblings, 0 replies; 6+ messages in thread
From: Paul Moore @ 2025-11-01 17:00 UTC (permalink / raw)
To: David Laight
Cc: Thorsten Blum, James Morris, Serge E. Hallyn,
linux-security-module, linux-kernel
On Fri, Oct 31, 2025 at 12:54 PM David Laight
<david.laight.linux@gmail.com> wrote:
> On Fri, 31 Oct 2025 16:23:02 +0100
> Thorsten Blum <thorsten.blum@linux.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.
Yes, just pass the seq_file pointer up to set_majmin() and have it do
the writes/puts directly. Might as well rename if from set_majmin()
to put_majmin() while you are at it.
--
paul-moore.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-11-01 17:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-31 11:06 [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin Thorsten Blum
2025-10-31 12:59 ` David Laight
2025-10-31 15:23 ` Thorsten Blum
2025-10-31 16:54 ` David Laight
2025-11-01 17:00 ` Paul Moore
2025-10-31 13:02 ` Serge E. Hallyn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).