> 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.
