Messages in this thread | | | Date | Mon, 6 May 2024 11:34:53 +0800 | Subject | Re: [PATCH 09/12] cachefiles: defer exposing anon_fd until after copy_to_user() succeeds | From | Baokun Li <> |
| |
On 2024/5/6 11:24, Jingbo Xu wrote: > > On 4/24/24 11:39 AM, libaokun@huaweicloud.com wrote: >> From: Baokun Li <libaokun1@huawei.com> >> >> 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@huawei.com> >> Signed-off-by: Baokun Li <libaokun1@huawei.com> >> --- >> 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);
| |