| Message ID | 20201009124113.a723e46a677a.Ib6576679bb8db01eb34d3dce77c4c6899c28ce26@changeid (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC] debugfs: protect against rmmod while files are open | expand |
On Fri, 2020-10-09 at 12:41 +0200, Johannes Berg wrote: > If the fops doesn't have a release method, we don't even need > to keep a reference to the real_fops, we can just fops_put() > them already in debugfs remove, and a later full_proxy_release() > won't call anything anyway - this just crashed/UAFed because it > used real_fops, not because there was actually a (now invalid) > release() method. I actually implemented something a bit better than what I described - we never need a reference to the real_fops for the release method alone, and that means if the release method is in the kernel image, rather than a module, it can still be called. That together should reduce the ~117 places you changed in the large patchset to around a handful. johannes
From: Johannes Berg > Sent: 09 October 2020 11:48 > > On Fri, 2020-10-09 at 12:41 +0200, Johannes Berg wrote: > > > If the fops doesn't have a release method, we don't even need > > to keep a reference to the real_fops, we can just fops_put() > > them already in debugfs remove, and a later full_proxy_release() > > won't call anything anyway - this just crashed/UAFed because it > > used real_fops, not because there was actually a (now invalid) > > release() method. > > I actually implemented something a bit better than what I described - we > never need a reference to the real_fops for the release method alone, > and that means if the release method is in the kernel image, rather than > a module, it can still be called. > > That together should reduce the ~117 places you changed in the large > patchset to around a handful. Is there an equivalent problem for normal cdev opens in any modules? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, 2020-10-09 at 10:56 +0000, David Laight wrote: > From: Johannes Berg > > Sent: 09 October 2020 11:48 > > > > On Fri, 2020-10-09 at 12:41 +0200, Johannes Berg wrote: > > > > > If the fops doesn't have a release method, we don't even need > > > to keep a reference to the real_fops, we can just fops_put() > > > them already in debugfs remove, and a later full_proxy_release() > > > won't call anything anyway - this just crashed/UAFed because it > > > used real_fops, not because there was actually a (now invalid) > > > release() method. > > > > I actually implemented something a bit better than what I described - we > > never need a reference to the real_fops for the release method alone, > > and that means if the release method is in the kernel image, rather than > > a module, it can still be called. > > > > That together should reduce the ~117 places you changed in the large > > patchset to around a handful. > > Is there an equivalent problem for normal cdev opens > in any modules? I guess so, but since there's no proxy_fops infrastructure and no revoke(), you can't really do anything else other than adding .owner properly, afaict. johannes
On Fri, Oct 09, 2020 at 10:56:16AM +0000, David Laight wrote: > From: Johannes Berg > > Sent: 09 October 2020 11:48 > > > > On Fri, 2020-10-09 at 12:41 +0200, Johannes Berg wrote: > > > > > If the fops doesn't have a release method, we don't even need > > > to keep a reference to the real_fops, we can just fops_put() > > > them already in debugfs remove, and a later full_proxy_release() > > > won't call anything anyway - this just crashed/UAFed because it > > > used real_fops, not because there was actually a (now invalid) > > > release() method. > > > > I actually implemented something a bit better than what I described - we > > never need a reference to the real_fops for the release method alone, > > and that means if the release method is in the kernel image, rather than > > a module, it can still be called. > > > > That together should reduce the ~117 places you changed in the large > > patchset to around a handful. > > Is there an equivalent problem for normal cdev opens > in any modules? What does cdev have to do with debugfs?
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c index ae49a55bda00..addacefc356e 100644 --- a/fs/debugfs/file.c +++ b/fs/debugfs/file.c @@ -94,6 +94,7 @@ int debugfs_file_get(struct dentry *dentry) fsd->real_fops = (void *)((unsigned long)d_fsd & ~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT); + fsd->fop_release = fsd->real_fops->release; refcount_set(&fsd->active_users, 1); init_completion(&fsd->active_users_drained); if (cmpxchg(&dentry->d_fsdata, d_fsd, fsd) != d_fsd) { @@ -258,8 +259,8 @@ static __poll_t full_proxy_poll(struct file *filp, static int full_proxy_release(struct inode *inode, struct file *filp) { - const struct dentry *dentry = F_DENTRY(filp); - const struct file_operations *real_fops = debugfs_real_fops(filp); + struct dentry *dentry = F_DENTRY(filp); + struct debugfs_fsdata *fsd = dentry->d_fsdata; const struct file_operations *proxy_fops = filp->f_op; int r = 0; @@ -268,13 +269,26 @@ static int full_proxy_release(struct inode *inode, struct file *filp) * original releaser should be called unconditionally in order * not to leak any resources. Releasers must not assume that * ->i_private is still being meaningful here. + * + * Note, however, that we don't reference real_fops (unless we + * can guarantee it's still around). We made a copy of release() + * before, in case it was NULL we then will not call anything and + * don't need to use real_fops at all. This allows us to allow + * module unloading of modules exposing debugfs files if they + * don't have release() methods. */ - if (real_fops->release) - r = real_fops->release(inode, filp); + if (fsd->fop_release) + r = fsd->fop_release(inode, filp); replace_fops(filp, d_inode(dentry)->i_fop); kfree((void *)proxy_fops); - fops_put(real_fops); + + /* fops_put() only if not already gone */ + if (refcount_inc_not_zero(&fsd->active_users)) { + fops_put(fsd->real_fops); + debugfs_file_put(dentry); + } + return r; } diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c index b7f2e971ecbc..25fd95f79c3b 100644 --- a/fs/debugfs/inode.c +++ b/fs/debugfs/inode.c @@ -377,6 +377,13 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode, struct dentry *dentry; struct inode *inode; + if (WARN(real_fops->release && + is_module_text_address((unsigned long)real_fops->release) && + !real_fops->owner, + "%ps is in a module but %ps doesn't have an owner", + real_fops->release, real_fops)) + return ERR_PTR(-EINVAL); + if (!(mode & S_IFMT)) mode |= S_IFREG; BUG_ON(!S_ISREG(mode)); @@ -672,6 +679,8 @@ static void __debugfs_file_removed(struct dentry *dentry) return; if (!refcount_dec_and_test(&fsd->active_users)) wait_for_completion(&fsd->active_users_drained); + fops_put(fsd->real_fops); + fsd->real_fops = NULL; } static void remove_one(struct dentry *victim) diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h index 034e6973cead..160a77abcfab 100644 --- a/fs/debugfs/internal.h +++ b/fs/debugfs/internal.h @@ -17,6 +17,7 @@ extern const struct file_operations debugfs_full_proxy_file_operations; struct debugfs_fsdata { const struct file_operations *real_fops; + int (*fop_release)(struct inode *, struct file *); refcount_t active_users; struct completion active_users_drained; };