Re: [PATCH 09/12] cachefiles: defer exposing anon_fd until after copy_to_user() succeeds

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2024/5/6 11:24, Jingbo Xu wrote:

On 4/24/24 11:39 AM, libaokun@xxxxxxxxxxxxxxx wrote:
From: Baokun Li <libaokun1@xxxxxxxxxx>

After installing the anonymous fd, we can now see it in userland and close
it. However, at this point we may not have gotten the reference count of
the cache, but we will put it during colse fd, so this may cause a cache
UAF.
Good catch!

To avoid this, we will make the anonymous fd accessible to the userland by
executing fd_install() after copy_to_user() has succeeded, and by this
point we must have already grabbed the reference count of the cache.
Why we must execute fd_install() after copy_to_user() has succeeded?
Why not grab a reference to the cache before fd_install()?
Two things are actually done here:
1) Grab a reference to the cache before fd_install()
2) By kernel convention, fd is taken over by the user land after
fd_install() is executed, and the kernel does not call close_fd() after
this, so fd_install() is called after everything is ready.

Thanks,
Baokun

Suggested-by: Hou Tao <houtao1@xxxxxxxxxx>
Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
---
  fs/cachefiles/ondemand.c | 53 +++++++++++++++++++++++++---------------
  1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 0cf63bfedc9e..7c2d43104120 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -4,6 +4,11 @@
  #include <linux/uio.h>
  #include "internal.h"
+struct anon_file {
+	struct file *file;
+	int fd;
+};
+
  static inline void cachefiles_req_put(struct cachefiles_req *req)
  {
  	if (refcount_dec_and_test(&req->ref))
@@ -244,14 +249,14 @@ int cachefiles_ondemand_restore(struct cachefiles_cache *cache, char *args)
  	return 0;
  }
-static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
+static int cachefiles_ondemand_get_fd(struct cachefiles_req *req,
+				      struct anon_file *anon_file)
  {
  	struct cachefiles_object *object;
  	struct cachefiles_cache *cache;
  	struct cachefiles_open *load;
-	struct file *file;
  	u32 object_id;
-	int ret, fd;
+	int ret;
object = cachefiles_grab_object(req->object,
  			cachefiles_obj_get_ondemand_fd);
@@ -263,16 +268,16 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
  	if (ret < 0)
  		goto err;
- fd = get_unused_fd_flags(O_WRONLY);
-	if (fd < 0) {
-		ret = fd;
+	anon_file->fd = get_unused_fd_flags(O_WRONLY);
+	if (anon_file->fd < 0) {
+		ret = anon_file->fd;
  		goto err_free_id;
  	}
- file = anon_inode_getfile("[cachefiles]", &cachefiles_ondemand_fd_fops,
-				  object, O_WRONLY);
-	if (IS_ERR(file)) {
-		ret = PTR_ERR(file);
+	anon_file->file = anon_inode_getfile("[cachefiles]",
+				&cachefiles_ondemand_fd_fops, object, O_WRONLY);
+	if (IS_ERR(anon_file->file)) {
+		ret = PTR_ERR(anon_file->file);
  		goto err_put_fd;
  	}
@@ -281,15 +286,14 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
  		spin_unlock(&object->ondemand->lock);
  		ret = -EEXIST;
  		/* Avoid performing cachefiles_ondemand_fd_release(). */
-		file->private_data = NULL;
+		anon_file->file->private_data = NULL;
  		goto err_put_file;
  	}
- file->f_mode |= FMODE_PWRITE | FMODE_LSEEK;
-	fd_install(fd, file);
+	anon_file->file->f_mode |= FMODE_PWRITE | FMODE_LSEEK;
load = (void *)req->msg.data;
-	load->fd = fd;
+	load->fd = anon_file->fd;
  	object->ondemand->ondemand_id = object_id;
  	spin_unlock(&object->ondemand->lock);
@@ -298,9 +302,11 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
  	return 0;
err_put_file:
-	fput(file);
+	fput(anon_file->file);
+	anon_file->file = NULL;
  err_put_fd:
-	put_unused_fd(fd);
+	put_unused_fd(anon_file->fd);
+	anon_file->fd = ret;
  err_free_id:
  	xa_erase(&cache->ondemand_ids, object_id);
  err:
@@ -357,6 +363,7 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
  	struct cachefiles_msg *msg;
  	size_t n;
  	int ret = 0;
+	struct anon_file anon_file;
  	XA_STATE(xas, &cache->reqs, cache->req_id_next);
xa_lock(&cache->reqs);
@@ -390,7 +397,7 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
  	xa_unlock(&cache->reqs);
if (msg->opcode == CACHEFILES_OP_OPEN) {
-		ret = cachefiles_ondemand_get_fd(req);
+		ret = cachefiles_ondemand_get_fd(req, &anon_file);
  		if (ret)
  			goto out;
  	}
@@ -398,10 +405,16 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
  	msg->msg_id = xas.xa_index;
  	msg->object_id = req->object->ondemand->ondemand_id;
- if (copy_to_user(_buffer, msg, n) != 0) {
+	if (copy_to_user(_buffer, msg, n) != 0)
  		ret = -EFAULT;
-		if (msg->opcode == CACHEFILES_OP_OPEN)
-			close_fd(((struct cachefiles_open *)msg->data)->fd);
+
+	if (msg->opcode == CACHEFILES_OP_OPEN) {
+		if (ret < 0) {
+			fput(anon_file.file);
+			put_unused_fd(anon_file.fd);
+			goto out;
+		}
+		fd_install(anon_file.fd, anon_file.file);
  	}
  out:
  	cachefiles_put_object(req->object, cachefiles_obj_put_read_req);






[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux