On Mon, Sep 26, 2022 at 5:05 AM Christian Brauner <brauner@xxxxxxxxxx> wrote: > On Fri, Sep 23, 2022 at 01:35:08PM -0400, Paul Moore wrote: > > On Fri, Sep 23, 2022 at 10:35 AM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > On Fri, Sep 23, 2022 at 10:26:35AM -0400, Paul Moore wrote: > > > > On Fri, Sep 23, 2022 at 3:57 AM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > > > On Fri, Sep 23, 2022 at 08:47:07AM +0200, Christoph Hellwig wrote: > > > > > > On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote: > > > > > > > properly review the changes, but one thing immediately jumped out at > > > > > > > me when looking at this: why is the LSM hook > > > > > > > "security_inode_set_acl()" when we are passing a dentry instead of an > > > > > > > inode? We don't have a lot of them, but there are > > > > > > > `security_dentry_*()` LSM hooks in the existing kernel code. > > > > > > > > > > > > I'm no LSM expert, but isn't the inode vs dentry for if it is > > > > > > related to an inode operation or dentry operation, not about that > > > > > > the first argument is? > > > > > > > > > > Indeed. For example ... > > > > > > > > If the goal is for this LSM hook to operate on an inode and not a > > > > dentry, let's pass it an inode instead. This should help prevent > > > > > > I would be ok with that but EVM requires a dentry being passed and as > > > evm is called from security_inode_set_acl() exactly like it is from > > > security_inode_setxattr() and similar the hook has to take a dentry. > > > > If a dentry is truly needed by EVM (a quick look indicates that it may > > just be for the VFS getxattr API, but I haven't traced the full code > > path), then I'm having a hard time reconciling that this isn't a > > dentry operation. Yes, I get that the ACLs belong to the inode and > > not the dentry, but then why do we need the dentry? It seems like the > > interfaces are broken slightly, or at least a little odd ... <shrug> > > There's multiple reasons for the generic xattr api to take a dentry. For > example, there are quite a few filesystems that require dentry access > during (specific or all) xattr operations. So ideally, we'd just want to > pass the dentry I'd say ... Independent of the naming issue discussed above, I want to make it clear that in general I believe that the dentry is more useful to the LSMs if for no other reason that it is pretty trivial to go from a dentry to an inode, whereas the opposite is not true. Of course there are cases where the dentry is not always available, or the connectivity between the dentry and the inode is not yet present. In those cases we would obviously need to pass the inode, or both the inode and the dentry, which gets me to my next point and my motivation behind bringing all this up once we started down the rathole ... My main concern is making sure that the LSM hooks are declared in such a way that they are fairly resistant to misuse; not that I expect any malice, but I do believe accidental misuse of the hooks is legitimate concern. With respect to the various VFS related hooks, one of the things that has always caught my attention in this respect has been hooks which pass both a dentry and a inode for the same file. Of course there are cases where this will always be necessary, but I hate to see bad interfaces continued forward simply because "that's the way we've always done it". In this particular patch the hook prototype is reasonably well defined as far as I'm concerned, with no worries of both a dentry and an inode, so that's good. I still don't really like the name disconnect between the function name and the parameter list (inode vs dentry), but que sera sera; my appetite for argument here is rapidly waning. -- paul-moore.com
