linux-kernel - [RFC PATCH 3/3] debugfs: make __debugfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1449751634-7887-4-git-send-email-r.peniaev@gmail.com>
Date:	Thu, 10 Dec 2015 13:47:14 +0100
From:	Roman Pen <r.peniaev@...il.com>
To:	unlisted-recipients:; (no To-header on input)
Cc:	Roman Pen <r.peniaev@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: [RFC PATCH 3/3] debugfs: make __debugfs_remove wait for dentry release

__debugfs_remove does not wait for dentry release, thus dentry can still be
alive and file operations can still be invoked after the function returns.

>From debugfs point of view this behaviour is definitely ok, but that can be
critical for users of debugfs and lead to usage-after-free: file operations
can be called after dentry is considered as removed.

Simple grep over the sources shows that dynamic debugfs file creation and
removal is exactly the case, and common usage is the following:

  create_dev():
      dev = kmalloc();
      dev->debugfs_dentry = debugfs_create_file("my_dev", , dev, dev_fops);
                                                            ^^^
                                            !! pointer is passed to file
                                            !! operations as private data

  remove_dev(dev):
      debugfs_remove(dev->debugfs_dentry);
      kfree(dev);
            ^^^
      !! memory is freed, but fops->open/read/write
      !! can still be called and lead to usage-after-free

Here is quick grep output of the case described above:

   *** drivers/block/pktcdvd.c:
       pkt_debugfs_dev_remove[489]    debugfs_remove(pd->dfs_f_info);
       pkt_debugfs_dev_remove[490]    debugfs_remove(pd->dfs_d_root);

   *** drivers/char/virtio_console.c:
       unplug_port[1595]              debugfs_remove(port->debugfs_file);

   *** drivers/crypto/qat/qat_common/adf_cfg.c:
       adf_cfg_dev_remove[187]        debugfs_remove(dev_cfg_data->debug);

   *** drivers/gpu/drm/drm_debugfs.c:
       drm_debugfs_remove_files[203]  debugfs_remove(tmp->dent);

  .... and more and more and more ...

In my grep output each third line is exactly this case: people expect that
debugfs_remove() is a barrier and file operations won't be invoked after it
(same behaviour as kobject_del(),kobject_put() tuple).

So in this patch debugfs_remove() waits for completion of final dentry release
callback.

BUT! I am not sure that nobody tries to remove the dentry from it's own file
operation (dentry suicide).  And if so - deadlock will happen.

Probably, dentry_remove_self() should be implemented for such cases, which is
similar to sysfs_remove_file_self().  But for now I do not want to add new
function which can be useless in the nearest future.

Signed-off-by: Roman Pen <r.peniaev@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org
---
 fs/debugfs/inode.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index a1d077a..2525158 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -193,9 +193,23 @@ static struct vfsmount *debugfs_automount(struct path *path)
 	return p->func(p->data);
 }
 
+static void debugfs_release(struct dentry *dentry)
+{
+	struct completion *comp;
+
+	/* Paired with __debugfs_remove */
+	smp_rmb();
+	comp = dentry->d_fsdata;
+	if (likely(comp)) {
+		dentry->d_fsdata = NULL;
+		complete(comp);
+	}
+}
+
 static const struct dentry_operations debugfs_dops = {
 	.d_delete = always_delete_dentry,
 	.d_automount = debugfs_automount,
+	.d_release = debugfs_release,
 };
 
 static int debug_fill_super(struct super_block *sb, void *data, int silent)
@@ -542,14 +556,24 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent)
 	int ret = 0;
 
 	if (simple_positive(dentry)) {
+		struct completion comp;
+
+		init_completion(&comp);
 		dget(dentry);
 		if (d_is_dir(dentry))
 			ret = simple_rmdir(d_inode(parent), dentry);
 		else
 			simple_unlink(d_inode(parent), dentry);
-		if (!ret)
+		if (likely(!ret)) {
 			d_delete(dentry);
+			dentry->d_fsdata = &comp;
+			/* Paired with debugfs_release callback */
+			smp_wmb();
+		}
 		dput(dentry);
+
+		if (likely(!ret))
+			wait_for_completion(&comp);
 	}
 	return ret;
 }
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/