On Fri, Dec 13, 2024 at 5:18 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > On Thu, Dec 12, 2024 at 12:26 AM Takaya Saeki <takayas@xxxxxxxxxxxx> 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.
