* [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 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
* 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
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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.