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

Message-ID: <CAHC9VhQ3nhAjupyENXnZN0WXsbibjeA6OT6fCXOfXBjWaw0=6A@mail.gmail.com>
Date: Sat, 1 Nov 2025 13:00:34 -0400
From: Paul Moore <paul@...l-moore.com>
To: David Laight <david.laight.linux@...il.com>
Cc: Thorsten Blum <thorsten.blum@...ux.dev>, 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:54 PM David Laight
<david.laight.linux@...il.com> wrote:
> On Fri, 31 Oct 2025 16:23:02 +0100
> Thorsten Blum <thorsten.blum@...ux.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