| Message ID | 202005051626.7648DC65@keescook (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | securityfs: Add missing d_delete() call on removal | expand |
On Tue, May 05, 2020 at 04:40:35PM -0700, Kees Cook wrote: > After using simple_unlink(), a call to d_delete() is needed in addition > to dput(). > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > Is this correct? I went looking around and there are a lot of variations > on the simple_unlink() pattern... > > Many using explicit locking and combinations of d_drop(), __d_drop(), etc. Quite a few of those should switch to simple_recursive_removal(). As for securityfs... d_drop() is _probably_ a saner variant, but looking at the callers of that thing... at least IMA ones seem to be garbage. They _might_ work since nobody else is around that early to screw them over, but... I'm not too optimistic about that. And the lack of d_delete()/d_drop() here is the least of the problems, really - look at e.g. the bulk of those suckers in ima_fs_init(). And check the callchain - it'll lead you to this if (error && strcmp(hash_algo_name[ima_hash_algo], CONFIG_IMA_DEFAULT_HASH) != 0) { pr_info("Allocating %s failed, going to use default hash algorithm %s\n", hash_algo_name[ima_hash_algo], CONFIG_IMA_DEFAULT_HASH); hash_setup_done = 0; hash_setup(CONFIG_IMA_DEFAULT_HASH); error = ima_init(); } error = register_blocking_lsm_notifier(&ima_lsm_policy_notifier); And the other IMA caller (in ima_release_policy()) is... misguided, to put it politely. This kind of "unlink on final close" makes no sense - if nothing else, it can be looked up until that very point. Moreover, this inode->i_mode &= ~S_IWUSR; is obviously racy wrt permission(), and there's no damn reason to do it there. These checks belong in ->open() and they shouldn't rely upon the damn thing disappearing from directory or permission() failing, etc.. And looking at their ->open()... ouch. ->f_flags & O_ACCMODE is almost never the right thing to check. It should be looking at ->f_mode & FMODE_{READ,WRITE}. I hadn't looked into details for EVM, but at the first glance it's similar to IMA failure handling. And probably relies upon nobody being able to open that stuff while the things are being set up. There seems to be something in TPM as well - and by the look of it they are trying to work around the use of unsuitable primitive, and none too well, at that. I mean, int err; struct seq_file *seq; struct tpm_chip_seqops *chip_seqops; const struct seq_operations *seqops; struct tpm_chip *chip; inode_lock(inode); if (!inode->i_private) { inode_unlock(inode); return -ENODEV; } chip_seqops = (struct tpm_chip_seqops *)inode->i_private; seqops = chip_seqops->seqops; chip = chip_seqops->chip; get_device(&chip->dev); inode_unlock(inode); /* now register seq file */ err = seq_open(file, seqops); if (!err) { seq = file->private_data; seq->private = chip; } return err; doesn't look sane - late error would appear to leak &chip->dev...
On Wed, May 06, 2020 at 02:14:31AM +0100, Al Viro wrote: > On Tue, May 05, 2020 at 04:40:35PM -0700, Kees Cook wrote: > > After using simple_unlink(), a call to d_delete() is needed in addition > > to dput(). > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > Is this correct? I went looking around and there are a lot of variations > > on the simple_unlink() pattern... > > > > Many using explicit locking and combinations of d_drop(), __d_drop(), etc. > > Quite a few of those should switch to simple_recursive_removal(). As for > securityfs... d_drop() is _probably_ a saner variant, but looking at the > callers of that thing... at least IMA ones seem to be garbage. Hmm, I dunno. I hadn't looked at these yet. I'm not sure what's needed for those cases. Is my patch to add d_delete() correct, though? I'm trying to construct the right set of calls for pstore's filesystem, and I noticed that most will do simple_unlink(), d_delete(), dput(), but securityfs seemed to be missing it.
On Tue, May 05, 2020 at 08:28:33PM -0700, Kees Cook wrote: > On Wed, May 06, 2020 at 02:14:31AM +0100, Al Viro wrote: > > On Tue, May 05, 2020 at 04:40:35PM -0700, Kees Cook wrote: > > > After using simple_unlink(), a call to d_delete() is needed in addition > > > to dput(). > > > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > --- > > > Is this correct? I went looking around and there are a lot of variations > > > on the simple_unlink() pattern... > > > > > > Many using explicit locking and combinations of d_drop(), __d_drop(), etc. > > > > Quite a few of those should switch to simple_recursive_removal(). As for > > securityfs... d_drop() is _probably_ a saner variant, but looking at the > > callers of that thing... at least IMA ones seem to be garbage. > > Hmm, I dunno. I hadn't looked at these yet. I'm not sure what's needed > for those cases. > > Is my patch to add d_delete() correct, though? I'm trying to construct > the right set of calls for pstore's filesystem, and I noticed that most > will do simple_unlink(), d_delete(), dput(), but securityfs seemed to be > missing it. d_drop(). d_delete() is for the situations when you want the sucker to become a hashed negative, if at all possible. Re pstore: context, please.
On Wed, May 06, 2020 at 05:02:52AM +0100, Al Viro wrote: > On Tue, May 05, 2020 at 08:28:33PM -0700, Kees Cook wrote: > > On Wed, May 06, 2020 at 02:14:31AM +0100, Al Viro wrote: > > > On Tue, May 05, 2020 at 04:40:35PM -0700, Kees Cook wrote: > > > > After using simple_unlink(), a call to d_delete() is needed in addition > > > > to dput(). > > > > > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > > --- > > > > Is this correct? I went looking around and there are a lot of variations > > > > on the simple_unlink() pattern... > > > > > > > > Many using explicit locking and combinations of d_drop(), __d_drop(), etc. > > > > > > Quite a few of those should switch to simple_recursive_removal(). As for > > > securityfs... d_drop() is _probably_ a saner variant, but looking at the > > > callers of that thing... at least IMA ones seem to be garbage. > > > > Hmm, I dunno. I hadn't looked at these yet. I'm not sure what's needed > > for those cases. > > > > Is my patch to add d_delete() correct, though? I'm trying to construct > > the right set of calls for pstore's filesystem, and I noticed that most > > will do simple_unlink(), d_delete(), dput(), but securityfs seemed to be > > missing it. > > d_drop(). d_delete() is for the situations when you want the sucker > to become a hashed negative, if at all possible. I'm not sure what that means. :) Should stuff like apparmorfs be changed to d_drop()? > Re pstore: context, please. Just posted the whole series: https://lore.kernel.org/lkml/20200506152114.50375-1-keescook@chromium.org/ But the specific question was driven by this patch: https://lore.kernel.org/lkml/20200506152114.50375-11-keescook@chromium.org/
On Wed, May 06, 2020 at 08:34:29AM -0700, Kees Cook wrote: > Just posted the whole series: > https://lore.kernel.org/lkml/20200506152114.50375-1-keescook@chromium.org/ > > But the specific question was driven by this patch: > https://lore.kernel.org/lkml/20200506152114.50375-11-keescook@chromium.org/ Yecchh... First of all, you are leaving a dangling pointer in your struct pstore_private ->dentry. What's more, in your case d_delete() is definitely wrong - either there are other references to dentry (in which case d_delete() is the same as d_drop()), or dput() right after it will drive ->d_count to zero and since you end up using simple_dentry_operations, dentry will be freed immediately after that. I have not looked at the locking in that series yet, so no comments on the races, but in any case - that d_delete() is a misspelled d_drop().
On Wed, May 06, 2020 at 07:49:20PM +0100, Al Viro wrote: > On Wed, May 06, 2020 at 08:34:29AM -0700, Kees Cook wrote: > > > Just posted the whole series: > > https://lore.kernel.org/lkml/20200506152114.50375-1-keescook@chromium.org/ > > > > But the specific question was driven by this patch: > > https://lore.kernel.org/lkml/20200506152114.50375-11-keescook@chromium.org/ > > Yecchh... First of all, you are leaving a dangling pointer in your > struct pstore_private ->dentry. What's more, in your case d_delete() Yeah, good idea: I can wipe out the pstore_private->dentry at this point just for robustness. From what I could tell the evict got immediately called after the dput(). > is definitely wrong - either there are other references to dentry > (in which case d_delete() is the same as d_drop()), or dput() right > after it will drive ->d_count to zero and since you end up using > simple_dentry_operations, dentry will be freed immediately after > that. Do you mean the d_drop() isn't needed? What happens if someone has the file open during this routine? The goal here is to make these files disappear so they'll go through evict. > I have not looked at the locking in that series yet, so no comments Yeah, I would not be surprised by some more locking issues, but I think it's an improvement over what was there. Most of the code seems to have been designed to be non-modular. :P > on the races, but in any case - that d_delete() is a misspelled d_drop(). I'll change it; thanks!
diff --git a/security/inode.c b/security/inode.c index 6c326939750d..606f390d21d2 100644 --- a/security/inode.c +++ b/security/inode.c @@ -306,6 +306,7 @@ void securityfs_remove(struct dentry *dentry) simple_rmdir(dir, dentry); else simple_unlink(dir, dentry); + d_delete(dentry); dput(dentry); } inode_unlock(dir);
After using simple_unlink(), a call to d_delete() is needed in addition to dput(). Signed-off-by: Kees Cook <keescook@chromium.org> --- Is this correct? I went looking around and there are a lot of variations on the simple_unlink() pattern... Many using explicit locking and combinations of d_drop(), __d_drop(), etc. Some missing d_delete()? security/inode.c: simple_unlink(dir, dentry); security/inode.c- d_delete(dentry); security/inode.c- dput(dentry); -- arch/powerpc/platforms/cell/spufs/inode.c: simple_unlink(d_inode(dir), dentry); arch/powerpc/platforms/cell/spufs/inode.c- /* XXX: what was dcache_lock protecting here? Other arch/powerpc/platforms/cell/spufs/inode.c- * filesystems (IB, configfs) release dcache_lock arch/powerpc/platforms/cell/spufs/inode.c- * before unlink */ arch/powerpc/platforms/cell/spufs/inode.c- dput(dentry); Should use d_delete() instead of d_drop()? arch/s390/hypfs/inode.c: simple_unlink(d_inode(parent), dentry); arch/s390/hypfs/inode.c- } arch/s390/hypfs/inode.c- d_drop(dentry); arch/s390/hypfs/inode.c- dput(dentry); arch/s390/hypfs/inode.c- inode_unlock(d_inode(parent)); arch/s390/hypfs/inode.c-} Correct? drivers/android/binderfs.c: simple_unlink(parent_inode, dentry); drivers/android/binderfs.c- d_delete(dentry); drivers/android/binderfs.c- dput(dentry); -- fs/nfsd/nfsctl.c: ret = simple_unlink(dir, dentry); fs/nfsd/nfsctl.c- d_delete(dentry); fs/nfsd/nfsctl.c- dput(dentry); -- net/sunrpc/rpc_pipe.c: ret = simple_unlink(dir, dentry); net/sunrpc/rpc_pipe.c- if (!ret) net/sunrpc/rpc_pipe.c- fsnotify_unlink(dir, dentry); net/sunrpc/rpc_pipe.c- d_delete(dentry); net/sunrpc/rpc_pipe.c- dput(dentry); -- security/apparmor/apparmorfs.c: simple_unlink(dir, dentry); security/apparmor/apparmorfs.c- d_delete(dentry); security/apparmor/apparmorfs.c- dput(dentry); --- security/inode.c | 1 + 1 file changed, 1 insertion(+)