| Message ID | 20250311092920.1114210-1-takayas@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v2] selinux: support wildcard match in genfscon | expand |
Hello, now this patch no longer appends "*" in the kernel space.
I tested this patch on Debian by creating a modified SELinux policy
where all genfs rules were followed by a trailing '*" and the new
genfs_seclabel_wildcard cap were enabled. Both the new policy with the
capability enabled and Debian's default policy without that policy
capability made correct labels.
> + bool wildcard = 0;
I overlooked that this should be `= true`. I can fix it.
Mar 11, 2025 10:42:22 Takaya Saeki <takayas@chromium.org>: > Hello, now this patch no longer appends "*" in the kernel space. > I tested this patch on Debian by creating a modified SELinux policy > where all genfs rules were followed by a trailing '*" and the new > genfs_seclabel_wildcard cap were enabled. Both the new policy with the > capability enabled and Debian's default policy without that policy > capability made correct labels. > >> + bool wildcard = 0; > I overlooked that this should be `= true`. I can fix it. Or maybe drop this assignment, since tge variable is always assigned later on (and modern compilers are good at warning about uninitialized local variables). On another point maybe this feature can be combined under the new policy capability netif_wildcard, to avoid adding two?
Mar 11, 2025 10:29:42 Takaya Saeki <takayas@chromium.org>: > 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@chromium.org> > --- > 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? > 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
On Tue, Mar 11, 2025 at 6:52 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > Mar 11, 2025 10:29:42 Takaya Saeki <takayas@chromium.org>: > > > 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@chromium.org> > > --- > > 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).
On Tue, 11 Mar 2025 at 17:23, Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Tue, Mar 11, 2025 at 6:52 AM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > Mar 11, 2025 10:29:42 Takaya Saeki <takayas@chromium.org>: > > > > > 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@chromium.org> > > > --- > > > 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()? 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) {
On Tue, Mar 11, 2025 at 2:55 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > On Tue, 11 Mar 2025 at 17:23, Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > > > On Tue, Mar 11, 2025 at 6:52 AM Christian Göttsche > > <cgzones@googlemail.com> wrote: > > > > > > Mar 11, 2025 10:29:42 Takaya Saeki <takayas@chromium.org>: > > > > > > > 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@chromium.org> > > > > --- > > > > 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) {
Thank you for feedbacks. On Tue, Mar 11, 2025 at 7:39 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > Mar 11, 2025 10:42:22 Takaya Saeki <takayas@chromium.org>: > > > Hello, now this patch no longer appends "*" in the kernel space. > > I tested this patch on Debian by creating a modified SELinux policy > > where all genfs rules were followed by a trailing '*" and the new > > genfs_seclabel_wildcard cap were enabled. Both the new policy with the > > capability enabled and Debian's default policy without that policy > > capability made correct labels. > > > >> + bool wildcard = 0; > > I overlooked that this should be `= true`. I can fix it. > > Or maybe drop this assignment, since tge variable is always assigned later on (and modern compilers are good at warning about uninitialized local variables). I agree. Let me drop the initialization. > > On another point maybe this feature can be combined under the new policy capability netif_wildcard, to avoid adding two? So, do we rename POLICYDB_CAP_NETIF_WILDCARD to POLICYDB_CAP_WILDCARD to control both wildcard capabilities? That should be fine for Android's use cases. However, it will mean users who want to enable the wildcard feature for network cards also have to take care of incompatibility of genfscon at the same time. I'd like to ask for opinions from maintainers.
On Wed, Mar 12, 2025 at 3:56 AM Takaya Saeki <takayas@chromium.org> wrote: > > Thank you for feedbacks. > > On Tue, Mar 11, 2025 at 7:39 PM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > Mar 11, 2025 10:42:22 Takaya Saeki <takayas@chromium.org>: > > > > > Hello, now this patch no longer appends "*" in the kernel space. > > > I tested this patch on Debian by creating a modified SELinux policy > > > where all genfs rules were followed by a trailing '*" and the new > > > genfs_seclabel_wildcard cap were enabled. Both the new policy with the > > > capability enabled and Debian's default policy without that policy > > > capability made correct labels. > > > > > >> + bool wildcard = 0; > > > I overlooked that this should be `= true`. I can fix it. > > > > Or maybe drop this assignment, since tge variable is always assigned later on (and modern compilers are good at warning about uninitialized local variables). > > I agree. Let me drop the initialization. > > > > > On another point maybe this feature can be combined under the new policy capability netif_wildcard, to avoid adding two? > > So, do we rename POLICYDB_CAP_NETIF_WILDCARD to POLICYDB_CAP_WILDCARD > to control both wildcard capabilities? That should be fine for > Android's use cases. > However, it will mean users who want to enable the wildcard feature > for network cards also have to take care of incompatibility of > genfscon at the same time. I'd like to ask for opinions from > maintainers. It is Paul's call to make, but I would recommend keeping them separate.
On Wed, Mar 12, 2025 at 9:34 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Wed, Mar 12, 2025 at 3:56 AM Takaya Saeki <takayas@chromium.org> wrote: > > > > Thank you for feedbacks. > > > > On Tue, Mar 11, 2025 at 7:39 PM Christian Göttsche > > <cgzones@googlemail.com> wrote: > > > > > > Mar 11, 2025 10:42:22 Takaya Saeki <takayas@chromium.org>: > > > > > > > Hello, now this patch no longer appends "*" in the kernel space. > > > > I tested this patch on Debian by creating a modified SELinux policy > > > > where all genfs rules were followed by a trailing '*" and the new > > > > genfs_seclabel_wildcard cap were enabled. Both the new policy with the > > > > capability enabled and Debian's default policy without that policy > > > > capability made correct labels. > > > > > > > >> + bool wildcard = 0; > > > > I overlooked that this should be `= true`. I can fix it. > > > > > > Or maybe drop this assignment, since tge variable is always assigned later on (and modern compilers are good at warning about uninitialized local variables). > > > > I agree. Let me drop the initialization. > > > > > > > > On another point maybe this feature can be combined under the new policy capability netif_wildcard, to avoid adding two? > > > > So, do we rename POLICYDB_CAP_NETIF_WILDCARD to POLICYDB_CAP_WILDCARD > > to control both wildcard capabilities? That should be fine for > > Android's use cases. > > However, it will mean users who want to enable the wildcard feature > > for network cards also have to take care of incompatibility of > > genfscon at the same time. I'd like to ask for opinions from > > maintainers. > > It is Paul's call to make, but I would recommend keeping them separate. I agree that the idea of separating it is more reasonable. I will wait for Paul's feedback and will send a v3 patch that will at least do a minor fix of the variable initialization.
On Mar 11, 2025 Takaya Saeki <takayas@chromium.org> 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@chromium.org> > --- > 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
On Tue, Mar 18, 2025 at 6:17 AM Paul Moore <paul@paul-moore.com> wrote: > > On Mar 11, 2025 Takaya Saeki <takayas@chromium.org> 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@chromium.org> > > --- > > 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.
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); 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)
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@chromium.org> --- 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(-)