| Message ID | 20241210115551.1225204-1-takayas@chromium.org (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: support wildcard match in genfscon | expand |
On Dec 10, 2024 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> > --- > This patch is based on the RFC: > https://lore.kernel.org/selinux/CAH9xa6ct0Zf+vg6H6aN9aYzsAPjq8dYM7aF5Sw2eD31cFQ9BZA@mail.gmail.com/T/#t > 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(-) I would like to hear from the policy and toolchain folks on this idea before we go too much further with this, but I did take a quick look at the patch and left my comments below. > 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); More on this below, but it isn't immediately clear to me why we need to have the special handling above, can you help me understand why these changes are necessary? I understand you are "marking" the wildcard entries with a trailing '*', but since we are calling match_wildcard() in __security_genfs_sid(), why not fully embrace the match_wildcard() capabilities and allow arbitrary '?' and '*' wildcard matching if present in the policy's genfscon path entries? If we do that, we can drop most (all?) of the str_read() changes and simply check for the new policy capability when reading the policy, yes? > 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); We don't need to do the strlen() computation in the wildcard case. > - if ((!c->v.sclass || sclass == c->v.sclass) && > - (strncmp(c->u.name, path, len) == 0)) > - break; > + if (selinux_policycap_genfs_seclabel_wildcard()) { We should pull the policy capability check out of the loop. > + 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))) Shouldn't this be 'strcmp() == 0'? Did you test this change both with and without the policy capability enabled? > + break; > + } > } Completely untested, but here is what I'm thinking for the __security_genfs_sid() changes: diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 971c45d576ba..f9b045b4aa11 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2843,6 +2843,7 @@ static inline int __security_genfs_sid(struct selinux_policy *policy, struct genfs *genfs; struct ocontext *c; int cmp = 0; + bool wildcard; while (path[0] == '/' && path[1] == '/') path++; @@ -2859,11 +2860,18 @@ static inline int __security_genfs_sid(struct selinux_policy *policy, if (!genfs || cmp) return -ENOENT; + wildcard = selinux_policycap_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 { + if (strncmp(c->u.name, path, + strlen(c->u.name)) == 0) + break; + } + } } if (!c) -- paul-moore.com
> I would like to hear from the policy and toolchain folks on this idea before > we go too much further with this, but I did take a quick look at the patch > and left my comments below. Thank you for reviewing this patch. Could you let me know the relevant folks who could provide feedback from the policy and toolchain perspective? > > +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); > > More on this below, but it isn't immediately clear to me why we need to > have the special handling above, can you help me understand why these > changes are necessary? Sure. Thank you very much for the comments. > I understand you are "marking" the wildcard entries with a trailing '*', but > since we are calling match_wildcard() in __security_genfs_sid(), why not > fully embrace the match_wildcard() capabilities... Currently, genfscon rules perform prefix matching (e.g., `/sys/dev` matches `/sys/devices`). Directly using `match_wildcard()` would not preserve this behavior, as it does full match. To maintain compatibility with this existing prefix-matching behavior, the trailing '*' is added. > capabilities and allow arbitrary '?' and '*' wildcard matching if > present in the policy's genfscon path entries? If we do that, we can > drop most (all?) of the str_read() changes and simply check for the new > policy capability when reading the policy, yes? It allows arbitrary '?' and '*' in entries. The purpose of the trailing '*' is to keep the prefix match behavior as I explained, which does not conflict with user's metacharacters. > > for (c = genfs->head; c; c = c->next) { > > size_t len = strlen(c->u.name); > > We don't need to do the strlen() computation in the wildcard case. > > > - if ((!c->v.sclass || sclass == c->v.sclass) && > > - (strncmp(c->u.name, path, len) == 0)) > > - break; > > + if (selinux_policycap_genfs_seclabel_wildcard()) { > > We should pull the policy capability check out of the loop. I totally missed it. Thanks. I will update my patch based on your draft. > > + 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))) > > Shouldn't this be 'strcmp() == 0'? > > Did you test this change both with and without the policy capability > enabled? Thank you for catching that. My testing was insufficient. I will update this as well and retest.
On Thu, Dec 12, 2024 at 12:26 AM Takaya Saeki <takayas@chromium.org> wrote: > > > I would like to hear from the policy and toolchain folks on this idea before > > we go too much further with this, but I did take a quick look at the patch > > and left my comments below. > > Thank you for reviewing this patch. Could you let me know the relevant folks > who could provide feedback from the policy and toolchain perspective? The toolchain people are already on this email as they belong to the normal selinux@vger mailing list. From a practical perspective I believe the policy people are on this mailing list as well, but I just CC'd the Reference Policy mailing too just in case. > > > @@ -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); > > > > More on this below, but it isn't immediately clear to me why we need to > > have the special handling above, can you help me understand why these > > changes are necessary? > > Sure. Thank you very much for the comments. > > > I understand you are "marking" the wildcard entries with a trailing '*', but > > since we are calling match_wildcard() in __security_genfs_sid(), why not > > fully embrace the match_wildcard() capabilities... > > Currently, genfscon rules perform prefix matching (e.g., `/sys/dev` matches > `/sys/devices`). Directly using `match_wildcard()` would not preserve this > behavior, as it does full match. To maintain compatibility with this existing > prefix-matching behavior, the trailing '*' is added. Yes, the existing genfscon handling does prefix matching, likely as an easy workaround for the lack of wildcard matching, so if we move to properly supporting wildcard matching, and the policy explicitly opts into using this new capability, I'd rather just see the policy do the right thing with wildcards. It might mean more work to convert the policy, but this should be easier to understand and more consistent than silently adding a '*' wildcard at the end of every path when the path matching supports explicit wildcards anywhere in the path.
> > Currently, genfscon rules perform prefix matching (e.g., `/sys/dev` matches > > `/sys/devices`). Directly using `match_wildcard()` would not preserve this > > behavior, as it does full match. To maintain compatibility with this existing > > prefix-matching behavior, the trailing '*' is added. > > Yes, the existing genfscon handling does prefix matching, likely as an > easy workaround for the lack of wildcard matching, so if we move to > properly supporting wildcard matching, and the policy explicitly opts > into using this new capability, I'd rather just see the policy do the > right thing with wildcards. It might mean more work to convert the > policy, but this should be easier to understand and more consistent > than silently adding a '*' wildcard at the end of every path when the > path matching supports explicit wildcards anywhere in the path. Thanks for clarification. Backward incompatibility will make it challenging to enable the new wildcard feature for Android or any systems that allow users to define genfscon rules in addition to the system policy, since enabling it breaks existing user-defined policies. It would be nice we can keep the existing rules working. On second thought, we might need the POLICYDB_CAP_GENFS_SECLABEL_WILDCARD in the first place. Instead, we could make genfscon just support wildcard, since this does not make breaking changes actually. Genfscon will be applied to either pseudo filesystems, or old filesystems such as DOS and ISO9660. Pseudo filesystems will not contain '*' or '?' in the file names, and old filesystems do not allow these characters in file names either. Thus, there should be no "*" or "?" in genfscon policies in practice. I'd appreciate your opinion. Or alternatively, we can add POLICYDB_CAP_GENFS_SECLABEL_FULL_MATCH capability for users to choose the full match behavior. Adding a statement like `genfscon_wildcard` that does full match would be another idea as well.
On Fri, Dec 13, 2024 at 5:18 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Dec 12, 2024 at 12:26 AM Takaya Saeki <takayas@chromium.org> wrote: > > > > > I would like to hear from the policy and toolchain folks on this idea before > > > we go too much further with this, but I did take a quick look at the patch > > > and left my comments below. > > > > Thank you for reviewing this patch. Could you let me know the relevant folks > > who could provide feedback from the policy and toolchain perspective? > > The toolchain people are already on this email as they belong to the > normal selinux@vger mailing list. From a practical perspective I > believe the policy people are on this mailing list as well, but I just > CC'd the Reference Policy mailing too just in case. > > > > > @@ -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); > > > > > > More on this below, but it isn't immediately clear to me why we need to > > > have the special handling above, can you help me understand why these > > > changes are necessary? > > > > Sure. Thank you very much for the comments. > > > > > I understand you are "marking" the wildcard entries with a trailing '*', but > > > since we are calling match_wildcard() in __security_genfs_sid(), why not > > > fully embrace the match_wildcard() capabilities... > > > > Currently, genfscon rules perform prefix matching (e.g., `/sys/dev` matches > > `/sys/devices`). Directly using `match_wildcard()` would not preserve this > > behavior, as it does full match. To maintain compatibility with this existing > > prefix-matching behavior, the trailing '*' is added. > > Yes, the existing genfscon handling does prefix matching, likely as an > easy workaround for the lack of wildcard matching, so if we move to > properly supporting wildcard matching, and the policy explicitly opts > into using this new capability, I'd rather just see the policy do the > right thing with wildcards. It might mean more work to convert the > policy, but this should be easier to understand and more consistent > than silently adding a '*' wildcard at the end of every path when the > path matching supports explicit wildcards anywhere in the path. I think the problem with that approach is that it assumes that the entire policy can be updated at one time. If as is the case in Fedora you have independent policy modules shipped in individual packages, or as in the case of Android you have multiple distinct policy module providers (platform vs vendor vs odm vs product ... - seemingly they add another new one every release or so), if/when the base policy enables the new policy capability, there may still be policy modules present using the old genfscon rules. And since the kernel has no concept of policy modules, enabling the capability is necessarily global in its effect.
On Mon, Dec 16, 2024 at 1:50 AM Takaya Saeki <takayas@chromium.org> wrote: > > > > Currently, genfscon rules perform prefix matching (e.g., `/sys/dev` matches > > > `/sys/devices`). Directly using `match_wildcard()` would not preserve this > > > behavior, as it does full match. To maintain compatibility with this existing > > > prefix-matching behavior, the trailing '*' is added. > > > > Yes, the existing genfscon handling does prefix matching, likely as an > > easy workaround for the lack of wildcard matching, so if we move to > > properly supporting wildcard matching, and the policy explicitly opts > > into using this new capability, I'd rather just see the policy do the > > right thing with wildcards. It might mean more work to convert the > > policy, but this should be easier to understand and more consistent > > than silently adding a '*' wildcard at the end of every path when the > > path matching supports explicit wildcards anywhere in the path. > > Thanks for clarification. Backward incompatibility will make it challenging to > enable the new wildcard feature for Android or any systems that allow users to > define genfscon rules in addition to the system policy, since enabling it > breaks existing user-defined policies. It would be nice we can keep the > existing rules working. On Mon, Dec 16, 2024 at 9:06 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > I think the problem with that approach is that it assumes that the > entire policy can be updated at one time. A policy capability applies to the entire loaded policy, not just a portion. If one is going to enable a new policy capability, there is a certain responsibility to ensure that the entirety of the policy has been updated and is correct. If all that is required to convert older policies is to add a '*' at the end of genfscon pathnames, that can be easily done in userspace (and should be done in userspace). We really shouldn't have compatibility hacks when enabling policy capabilities, policy capabilities *are* the compatibility hack by allowing systems to continue to operate in the legacy mode until such time as the policy has been converted.
> We really shouldn't have compatibility hacks when enabling policy > capabilities, policy capabilities *are* the compatibility hack by > allowing systems to continue to operate in the legacy mode until such > time as the policy has been converted. While this makes sense, as Stephen pointed out, neither Fedora nor Android will be able to quickly enable this capability in reality. What do you think about two alternative ideas for right things; just start to interpret wildcards without introducing a new capability, or introducing a new syntax that does wildcard full match such as `genfsconwildcard`? I made a typo in my previous mail, but the rationale of supporting wildcards without a new capability is that wildcard metacharacters have actually backward compatibility in the field of genfs. Pseudo filesystems don't contain "*" or "?" in file names, and supported non-pseudo filesystems, DOS and ISO 9660 doesn't allow these characters either.
On Tue, Dec 17, 2024 at 2:13 AM Takaya Saeki <takayas@chromium.org> wrote: > > > We really shouldn't have compatibility hacks when enabling policy > > capabilities, policy capabilities *are* the compatibility hack by > > allowing systems to continue to operate in the legacy mode until such > > time as the policy has been converted. > > While this makes sense, as Stephen pointed out, neither Fedora nor Android will > be able to quickly enable this capability in reality. The speed at which a new nice-to-have feature can be adopted is generally not something I worry about, it's a new *feature*, not a bug fix so if it takes some time to be fully adopted that is okay. What I do concern myself about is the quality and long term maintainability of the kernel code, especially when user visible changes are concerned. Adding kernel complexity for changes like this, especially when they can be handled in userspace is almost always going to be a no-go as far as I'm concerned. > What do you think about > two alternative ideas for right things; just start to interpret wildcards > without introducing a new capability ... No. > or introducing a new syntax that does > wildcard full match such as `genfsconwildcard`? That seems pretty awful to me too. If you can't be bothered to actually update the policy as you should be doing when enabling a new policy capability, add the same hack you were proposing for the kernel to the compiler/linker toolchain and just start adding the '*' wildcard at the end of the paths.
> > The speed at which a new nice-to-have feature can be adopted is > generally not something I worry about, it's a new *feature*, not a bug > fix so if it takes some time to be fully adopted that is okay. What I > do concern myself about is the quality and long term maintainability > of the kernel code, especially when user visible changes are > concerned. Adding kernel complexity for changes like this, especially > when they can be handled in userspace is almost always going to be a > no-go as far as I'm concerned. The perspective of long term maintainability being more important is completely understandable. Also, your comments on the other alternatives are well-taken. Thank you very much for your input. Then, I will update my patch based on the full match, also reflecting your review comments. In the meantime, I'd like to confirm one remaining option that we haven't yet discussed, just to consider all possibilities. If the concern is primarily about the implementation rather than the behavior itself, would it be feasible to implement prefix matching using a dedicated helper function instead of using a trailing wildcard character like '*'?"
On Tue, Dec 17, 2024 at 8:38 PM Takaya Saeki <takayas@chromium.org> wrote: > > > > The speed at which a new nice-to-have feature can be adopted is > > generally not something I worry about, it's a new *feature*, not a bug > > fix so if it takes some time to be fully adopted that is okay. What I > > do concern myself about is the quality and long term maintainability > > of the kernel code, especially when user visible changes are > > concerned. Adding kernel complexity for changes like this, especially > > when they can be handled in userspace is almost always going to be a > > no-go as far as I'm concerned. > > The perspective of long term maintainability being more important is completely > understandable. Also, your comments on the other alternatives are well-taken. > Thank you very much for your input. Then, I will update my patch based on the > full match, also reflecting your review comments. > > In the meantime, I'd like to confirm one remaining option that we haven't yet > discussed, just to consider all possibilities. If the concern is primarily > about the implementation rather than the behavior itself, would it be feasible > to implement prefix matching using a dedicated helper function instead of using > a trailing wildcard character like '*'?" While adding a helper function instead of a direct wildcard concatenation would change the implementation slightly, the higher level concerns around added complexity remain, and for that reason I remain opposed to such an approach.
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)
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> --- This patch is based on the RFC: https://lore.kernel.org/selinux/CAH9xa6ct0Zf+vg6H6aN9aYzsAPjq8dYM7aF5Sw2eD31cFQ9BZA@mail.gmail.com/T/#t 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(-)