Dec 5, 2024 09:53:33 Takaya Saeki <takayas@xxxxxxxxxxxx>:
> 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@xxxxxxxxxxxx>
> ---
> 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
