| Message ID | CAH9xa6dmxzcooYYya5kH=KwfhhKUJSq9LYVKiwxj1sxsDB3h-w@mail.gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [RFC] genfscon wildcard support for faster sysfs labeling | expand |
Dec 5, 2024 09:53:33 Takaya Saeki <takayas@chromium.org>: > Hello SELinux developers, > > I propose wildcard match support in genfscon to improve sysfs labeling > performance, and I would like to hear your opinions on it. > > Currently, labeling numerous dynamic sysfs entries (e.g., > `/sys/devices/pci0000:00/0000:00:03.1/<...>/0000:04:00.1/wakeup`) > requires either listing all specific PCI paths in genfscon entries, > which are hard to maintain, or using slow regex rules in file_contexts > with restorecon. On our test device, `restorecon -R /sys` takes 2.7 > seconds with regular expression rules. Out of curiosity: can you give libselinux 3.8-rc1 a try, which might/should improve the runtime? > Wildcard matching offers a good balance here. The patch below allows us > to avoid the slowdowns of regex while keeping genfscon maintainable for > diverse hardware. > > This requires defining precedence when multiple wildcards match. I > suggest prioritizing longer matches, which is the existing behavior. For > example, given the rules `/sys/devices/*/wakeup` and > `/sys/devices/*/wakeup/*/uevent` (note `*` also matches `/`), the path > `/devices/LNXSYSTM:00/PNP0C14:01/wakeup/wakeup57/uevent` would match > both, but the second rule would be applied. Users can control the > precedence by writing concrete parent directories. > > There are also cases where multiple rules of the same length match > against a path. To remove this ambiguity, we can document the current > behavior that the first matching rule in the genfscon file takes > precedence. Users should not rely on this rule but should specify paths > that are concrete enough, though. > > I'd appreciate your feedback. > > Signed-off-by: Takaya Saeki <takayas@chromium.org> > --- > security/selinux/include/policycap.h | 1 + > security/selinux/include/policycap_names.h | 1 + > security/selinux/include/security.h | 6 +++ > security/selinux/ss/policydb.c | 56 ++++++++++++++++++---- > security/selinux/ss/services.c | 13 +++-- > 5 files changed, 66 insertions(+), 11 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..17e5c51f3cf4 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_wildcard", > }; > /* clang-format on */ > > diff --git a/security/selinux/include/security.h > b/security/selinux/include/security.h > index c7f2731abd03..ccd80fb037d5 100644 > --- a/security/selinux/include/security.h > +++ b/security/selinux/include/security.h > @@ -201,6 +201,12 @@ static inline bool selinux_policycap_netlink_xperm(void) > selinux_state.policycap[POLICYDB_CAP_NETLINK_XPERM]); > } > > +static inline bool selinux_policycap_genfs_seclabel_wildcard(void) > +{ > + return READ_ONCE( > + selinux_state.policycap[POLICYDB_CAP_GENFS_SECLABEL_WILDCARD]); > +} > + > struct selinux_policy_convert_data; > > struct selinux_load_state { > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index 383f3ae82a73..959e98fae3d5 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -1090,29 +1090,59 @@ static int context_read_and_validate(struct > context *c, struct policydb *p, > * binary representation file. > */ > > -static int str_read(char **strp, gfp_t flags, void *fp, u32 len) > +static int entry_read(char **bufp, gfp_t flags, void *fp, u32 entry_len, > + u32 buf_len) > { > int rc; > - char *str; > + char *buf; > > - if ((len == 0) || (len == (u32)-1)) > + if ((buf_len == 0) || (buf_len == (u32)-1) || (buf_len < entry_len)) > return -EINVAL; > > - str = kmalloc(len + 1, flags | __GFP_NOWARN); > - if (!str) > + buf = kmalloc(buf_len, flags | __GFP_NOWARN); > + if (!buf) > return -ENOMEM; > > - rc = next_entry(str, fp, len); > + rc = next_entry(buf, fp, entry_len); > if (rc) { > - kfree(str); > + kfree(buf); > return rc; > } > > + *bufp = buf; > + return 0; > +} > + > +static int str_read(char **strp, gfp_t flags, void *fp, u32 len) > +{ > + int rc; > + char *str; > + > + rc = entry_read(&str, flags, fp, len, len + 1); > + if (rc) > + return rc; > + > str[len] = '\0'; > *strp = str; > return 0; > } > > +static int str_read_with_padding(char **strp, gfp_t flags, void *fp, u32 len, > + char padding) > +{ > + int rc; > + char *str; > + > + rc = entry_read(&str, flags, fp, len, len + 2); > + if (rc) > + return rc; > + > + str[len] = padding; > + str[len + 1] = '\0'; > + *strp = str; > + return 0; > +} > + > static int perm_read(struct policydb *p, struct symtab *s, void *fp) > { > char *key = NULL; > @@ -2193,7 +2223,17 @@ static int genfs_read(struct policydb *p, void *fp) > if (!newc) > goto out; > > - rc = str_read(&newc->u.name, GFP_KERNEL, fp, len); > + if (ebitmap_get_bit( > + &p->policycaps, > + POLICYDB_CAP_GENFS_SECLABEL_WILDCARD)) > + /* Append a wildcard '*' to make it a wildcard pattern */ > + rc = str_read_with_padding(&newc->u.name, > + GFP_KERNEL, fp, len, > + '*'); > + else > + /* Prefix pattern */ > + rc = str_read(&newc->u.name, GFP_KERNEL, fp, > + len); > if (rc) > goto out; > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index 971c45d576ba..da4d22220fe8 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" > @@ -2861,9 +2862,15 @@ static inline int __security_genfs_sid(struct > selinux_policy *policy, > > 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 (selinux_policycap_genfs_seclabel_wildcard()) { > + if ((!c->v.sclass || sclass == c->v.sclass) && > + (match_wildcard(c->u.name, path))) > + break; > + } else { > + if ((!c->v.sclass || sclass == c->v.sclass) && > + (strncmp(c->u.name, path, len))) > + break; > + } > } > > if (!c) > -- > 2.47.0.338.g60cca15819-goog
> Out of curiosity: can you give libselinux 3.8-rc1 a try, which might/should > improve the runtime? Yes, we are excited to see the latest rework on the file_label structure. However, we have a few hundreds of non-trivial regular expression rules instead of literal rules. So, the latest rework is still not enough for us. By the way, I found a bug in the latest libselinux which breaks our existing rules. I'll share it in another thread. In addition, it's not enough even if restorecon is improved from 2.7 seconds to a few hundred milliseconds, which is the time of `restorecon -R /sys` in a clean Debian with the latest libselinux. On Android, restorecon runs for `/sys` when a device wakes up. Spending a few hundred milliseconds CPU time every time hurts the battery life a lot. Thus, we want to eliminate this overhead entirely by genfscon. Actually, we have another PoC to further improve the restorecon performance, but for the reason above we want to improve genfscon instead.
Hi Christian, I'm sorry for being late, but I reported the incompatible behavior I mentioned in https://lore.kernel.org/selinux/CAH9xa6eFO6BNeGko90bsq8CuDba9eO+qdDoF+7zfyAUHEDpH9g@mail.gmail.com/T/#u As for my RFC of this thread, I'll turn this into a decent patch instead of a RFC, and send it to reviewers for further discussion. Thanks, Takaya
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..17e5c51f3cf4 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_wildcard", }; /* clang-format on */ diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index c7f2731abd03..ccd80fb037d5 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -201,6 +201,12 @@ static inline bool selinux_policycap_netlink_xperm(void) selinux_state.policycap[POLICYDB_CAP_NETLINK_XPERM]); } +static inline bool selinux_policycap_genfs_seclabel_wildcard(void) +{ + return READ_ONCE( + selinux_state.policycap[POLICYDB_CAP_GENFS_SECLABEL_WILDCARD]); +} + struct selinux_policy_convert_data; struct selinux_load_state { diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 383f3ae82a73..959e98fae3d5 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -1090,29 +1090,59 @@ static int context_read_and_validate(struct context *c, struct policydb *p, * binary representation file. */ -static int str_read(char **strp, gfp_t flags, void *fp, u32 len) +static int entry_read(char **bufp, gfp_t flags, void *fp, u32 entry_len, + u32 buf_len) { int rc; - char *str; + char *buf; - if ((len == 0) || (len == (u32)-1)) + if ((buf_len == 0) || (buf_len == (u32)-1) || (buf_len < entry_len)) return -EINVAL; - str = kmalloc(len + 1, flags | __GFP_NOWARN); - if (!str) + buf = kmalloc(buf_len, flags | __GFP_NOWARN); + if (!buf) return -ENOMEM; - rc = next_entry(str, fp, len); + rc = next_entry(buf, fp, entry_len); if (rc) { - kfree(str); + kfree(buf); return rc; } + *bufp = buf; + return 0; +} + +static int str_read(char **strp, gfp_t flags, void *fp, u32 len) +{ + int rc; + char *str; + + rc = entry_read(&str, flags, fp, len, len + 1); + if (rc) + return rc; + str[len] = '\0'; *strp = str; return 0; } +static int str_read_with_padding(char **strp, gfp_t flags, void *fp, u32 len, + char padding) +{ + int rc; + char *str; + + rc = entry_read(&str, flags, fp, len, len + 2); + if (rc) + return rc; + + str[len] = padding; + str[len + 1] = '\0'; + *strp = str; + return 0; +} + static int perm_read(struct policydb *p, struct symtab *s, void *fp) { char *key = NULL; @@ -2193,7 +2223,17 @@ static int genfs_read(struct policydb *p, void *fp) if (!newc) goto out; - rc = str_read(&newc->u.name, GFP_KERNEL, fp, len); + if (ebitmap_get_bit( + &p->policycaps, + POLICYDB_CAP_GENFS_SECLABEL_WILDCARD)) + /* Append a wildcard '*' to make it a wildcard pattern */ + rc = str_read_with_padding(&newc->u.name, + GFP_KERNEL, fp, len, + '*'); + else + /* Prefix pattern */ + rc = str_read(&newc->u.name, GFP_KERNEL, fp, + len); if (rc) goto out; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 971c45d576ba..da4d22220fe8 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" @@ -2861,9 +2862,15 @@ static inline int __security_genfs_sid(struct selinux_policy *policy, 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 (selinux_policycap_genfs_seclabel_wildcard()) { + if ((!c->v.sclass || sclass == c->v.sclass) && + (match_wildcard(c->u.name, path))) + break; + } else { + if ((!c->v.sclass || sclass == c->v.sclass) && + (strncmp(c->u.name, path, len))) + break; + } }
Hello SELinux developers, I propose wildcard match support in genfscon to improve sysfs labeling performance, and I would like to hear your opinions on it. Currently, labeling numerous dynamic sysfs entries (e.g., `/sys/devices/pci0000:00/0000:00:03.1/<...>/0000:04:00.1/wakeup`) requires either listing all specific PCI paths in genfscon entries, which are hard to maintain, or using slow regex rules in file_contexts with restorecon. On our test device, `restorecon -R /sys` takes 2.7 seconds with regular expression rules. Wildcard matching offers a good balance here. The patch below allows us to avoid the slowdowns of regex while keeping genfscon maintainable for diverse hardware. This requires defining precedence when multiple wildcards match. I suggest prioritizing longer matches, which is the existing behavior. For example, given the rules `/sys/devices/*/wakeup` and `/sys/devices/*/wakeup/*/uevent` (note `*` also matches `/`), the path `/devices/LNXSYSTM:00/PNP0C14:01/wakeup/wakeup57/uevent` would match both, but the second rule would be applied. Users can control the precedence by writing concrete parent directories. There are also cases where multiple rules of the same length match against a path. To remove this ambiguity, we can document the current behavior that the first matching rule in the genfscon file takes precedence. Users should not rely on this rule but should specify paths that are concrete enough, though. I'd appreciate your feedback. Signed-off-by: Takaya Saeki <takayas@chromium.org> --- security/selinux/include/policycap.h | 1 + security/selinux/include/policycap_names.h | 1 + security/selinux/include/security.h | 6 +++ security/selinux/ss/policydb.c | 56 ++++++++++++++++++---- security/selinux/ss/services.c | 13 +++-- 5 files changed, 66 insertions(+), 11 deletions(-) if (!c)