On Tue, Mar 11, 2025 at 2:55 PM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> On Tue, 11 Mar 2025 at 17:23, Stephen Smalley
> <stephen.smalley.work@xxxxxxxxx> wrote:
> >
> > On Tue, Mar 11, 2025 at 6:52 AM Christian Göttsche
> > <cgzones@xxxxxxxxxxxxxx> wrote:
> > >
> > > Mar 11, 2025 10:29:42 Takaya Saeki <takayas@xxxxxxxxxxxx>:
> > >
> > > > 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(-)
> > > >
> > > > 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
> > > > };
> > > > #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;
> > > >
> > > > 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);
> > >
> > > Commonly policy capabilities are gathered via a helper from security.h from selinux_state, shouldn't it here too?
> >
> > Those are for accessing the cached policy capabilities in the
> > selinux_state from outside of the security server (and without holding
> > any locks), e.g. from the hook functions.
> > No need for that here, and this is more correct - using the policy we
> > were passed rather than whatever selinux_state might reference at the
> > time (e.g. during a policy reload).
>
> Should this also be used in security_netif_sid()?
Technically, yes. Not as critical there since security_netif_sid() is
not called on the new policy during policy reload, unlike
selinux_policy_genfs_sid(), but would be safer / more correct.
>
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 1b11648d9b85..720a69fb4234 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2587,14 +2587,13 @@ int security_netif_sid(const char *name, u32 *if_sid)
> return 0;
> }
>
> - wildcard_support = selinux_policycap_netif_wildcard();
> -
> retry:
> rc = 0;
> rcu_read_lock();
> policy = rcu_dereference(selinux_state.policy);
> policydb = &policy->policydb;
> sidtab = policy->sidtab;
> + wildcard_support = ebitmap_get_bit(policydb->policycaps,
> POLICYDB_CAP_NETIF_WILDCARD);
>
> c = policydb->ocontexts[OCON_NETIF];
> while (c) {
