On Tue, Mar 18, 2025 at 6:17 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>
> On Mar 11, 2025 Takaya Saeki <takayas@xxxxxxxxxxxx> wrote:
> >
> > Currently, genfscon only supports string prefix match to label files.
> > Thus, labeling numerous dynamic sysfs entries requires many specific
> > path rules. For example, labeling device paths such as
> > `/sys/devices/pci0000:00/0000:00:03.1/<...>/0000:04:00.1/wakeup`
> > requires listing all specific PCI paths, which is challenging to
> > maintain. While user-space restorecon can handle these paths with
> > regular expression rules, but relabeling thousands of paths under sysfs
> > after it is mounted is inefficient compared to using genfscon.
> >
> > This commit adds wildcard match to support rules efficient but
> > expressive enough. This allows users to create fine-grained sysfs rules
> > without burden of listing specific paths. When multiple wildcard rules
> > match against a path, then the longest rule (determined by the length of
> > the rule string) will be applied. If multiple rules of the same length
> > match, the first matching rule encountered in the genfscon policy will
> > be applied. However, users are encouraged to write longer, more explicit
> > path rules to avoid relying on this behavior.
> >
> > This change resulted in nice real-world performance improvements. For
> > example, boot times on test Android devices were reduced by 15%. This
> > improvement is due to the elimination of the "restorecon -R /sys" step
> > during boot, which takes more than two seconds in the worst case.
> >
> > Signed-off-by: Takaya Saeki <takayas@xxxxxxxxxxxx>
> > ---
> > Changelog between v2 and v1
> > - Use given genfs rules by the userspace as is, instead of appending "*".
> > - Fix __security_genfs_sid hadn't checked caps of the given argument.
> > - Fix the wrong strncmp usage bug.
> >
> > security/selinux/include/policycap.h | 1 +
> > security/selinux/include/policycap_names.h | 1 +
> > security/selinux/ss/services.c | 20 ++++++++++++++++----
> > 3 files changed, 18 insertions(+), 4 deletions(-)
>
> I would mention in the commit description above that the new
> functionality is gated by the "genfs_seclabel_wildcard" policy
> capability. Otherwise this looks much better, thank you for the
> revision; there are a couple of comments below, but they are minor
> and you already mentioned one of them ;)
>
> > diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h
> > index 079679fe7254..2b4014a826f0 100644
> > --- a/security/selinux/include/policycap.h
> > +++ b/security/selinux/include/policycap.h
> > @@ -15,6 +15,7 @@ enum {
> > POLICYDB_CAP_IOCTL_SKIP_CLOEXEC,
> > POLICYDB_CAP_USERSPACE_INITIAL_CONTEXT,
> > POLICYDB_CAP_NETLINK_XPERM,
> > + POLICYDB_CAP_GENFS_SECLABEL_WILDCARD,
> > __POLICYDB_CAP_MAX
> > };
>
> As far as piggy-backing on top of NETIF_WILDCARD, it's generally best to
> use individual policy capabilities unless there is some reason why making
> one change to the policy would also require you to change the other.
>
> You could consider adding a
> selinux_policycap_genfs_seclabel_wildcard() function, but since there
> isn't a need for it in this patch, and I question where else we might
> ever need it, it's fine if you want to omit the helper function and leave
> it as-is.
>
> > #define POLICYDB_CAP_MAX (__POLICYDB_CAP_MAX - 1)
> > diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h
> > index e080827408c4..1053f2c95ff3 100644
> > --- a/security/selinux/include/policycap_names.h
> > +++ b/security/selinux/include/policycap_names.h
> > @@ -18,6 +18,7 @@ const char *const selinux_policycap_names[__POLICYDB_CAP_MAX] = {
> > "ioctl_skip_cloexec",
> > "userspace_initial_context",
> > "netlink_xperm",
> > + "genfs_seclabel_wildcard",
> > };
> > /* clang-format on */
> >
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 8478842fbf9e..9f98c9dc71f6 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -48,6 +48,7 @@
> > #include <linux/audit.h>
> > #include <linux/vmalloc.h>
> > #include <linux/lsm_hooks.h>
> > +#include <linux/parser.h>
> > #include <net/netlabel.h>
> >
> > #include "flask.h"
> > @@ -2863,6 +2864,7 @@ static inline int __security_genfs_sid(struct selinux_policy *policy,
> > struct genfs *genfs;
> > struct ocontext *c;
> > int cmp = 0;
> > + bool wildcard = 0;
>
> As you mentioned earlier, I think we can get rid of this assignment.
>
> > while (path[0] == '/' && path[1] == '/')
> > path++;
> > @@ -2879,11 +2881,21 @@ static inline int __security_genfs_sid(struct selinux_policy *policy,
> > if (!genfs || cmp)
> > return -ENOENT;
> >
> > +
> > + wildcard = ebitmap_get_bit(&policy->policydb.policycaps,
> > + POLICYDB_CAP_GENFS_SECLABEL_WILDCARD);
>
> It looks like you're adding an extra blank line above the wildcard
> assignment, please don't do that, a single blank line is sufficient
> vertical whitespace.
>
> > for (c = genfs->head; c; c = c->next) {
> > - size_t len = strlen(c->u.name);
> > - if ((!c->v.sclass || sclass == c->v.sclass) &&
> > - (strncmp(c->u.name, path, len) == 0))
> > - break;
> > + if (!c->v.sclass || sclass == c->v.sclass) {
> > + if (wildcard) {
> > + if (match_wildcard(c->u.name, path))
> > + break;
> > + } else {
> > + size_t len = strlen(c->u.name);
> > +
> > + if ((strncmp(c->u.name, path, len)) == 0)
> > + break;
> > + }
> > + }
> > }
> >
> > if (!c)
> > --
> > 2.49.0.rc1.451.g8f38331e32-goog
>
> --
> paul-moore.com
Thank you for the review, Paul! I've sent a v3 patch that fixes the
extra blank line and assignment.
