On 5/3/2024 9:26 AM, Paul Moore wrote: > On Fri, May 3, 2024 at 11:51 AM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote: >> On 5/3/2024 8:36 AM, Paul Moore wrote: >>> On Fri, May 3, 2024 at 11:26 AM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote: >>>> On 5/2/2024 5:58 PM, Paul Moore wrote: >>>>> The current security_inode_setxattr() and security_inode_removexattr() >>>>> hooks rely on individual LSMs to either call into the associated >>>>> capability hooks (cap_inode_setxattr() or cap_inode_removexattr()), or >>>>> return a magic value of 1 to indicate that the LSM layer itself should >>>>> perform the capability checks. Unfortunately, with the default return >>>>> value for these LSM hooks being 0, an individual LSM hook returning a >>>>> 1 will cause the LSM hook processing to exit early, potentially >>>>> skipping a LSM. Thankfully, with the exception of the BPF LSM, none >>>>> of the LSMs which currently register inode xattr hooks should end up >>>>> returning a value of 1, and in the BPF LSM case, with the BPF LSM hooks >>>>> executing last there should be no real harm in stopping processing of >>>>> the LSM hooks. However, the reliance on the individual LSMs to either >>>>> call the capability hooks themselves, or signal the LSM with a return >>>>> value of 1, is fragile and relies on a specific set of LSMs being >>>>> enabled. This patch is an effort to resolve, or minimize, these >>>>> issues. >>>>> >>>>> Before we discuss the solution, >>>> https://lore.kernel.org/lkml/20231215221636.105680-1-casey@xxxxxxxxxxxxxxxx/T/#mac61625dc1983d13ee5e8015fd22e1165381f079 >>>> >>>> ... or am I missing something? >>> Yes, that patch, as well as some of the others that have been posted, >>> change the ordering of the access control checks, moving the LSM-based >>> checks ahead of the capability-based checks. The patch I'm proposing >>> here not only preserves the current ordering, but it sticks with our >>> access control convention of DAC-before-LSM. >> Fair enough. > Are you okay with the patch otherwise? It's too late for v6.9, but if > everyone is okay with this approach I'd like to merge this after the > v6.10 merge window closes. I'm not real happy with the shear size of the change, but I don't see a better approach that meets the stated objectives. You can add my Acked-by. > I also need to track down the appropriate commits for the 'Fixes:' > tag(s); I'm not entirely convinced that some of the other patches were > targeting the proper commit ... >
