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 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 ... -- paul-moore.com
