| 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
| ||
|
Date: Mon, 6 May 2024 11:34:53 +0800
From: Baokun Li <libaokun@...weicloud.com>
To: Jingbo Xu <jefflexu@...ux.alibaba.com>, netfs@...ts.linux.dev
Cc: dhowells@...hat.com, jlayton@...nel.org, zhujia.zj@...edance.com,
linux-erofs@...ts.ozlabs.org, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, Hou Tao <houtao1@...wei.com>,
libaokun@...weicloud.com, yangerkun <yangerkun@...wei.com>
Subject: Re: [PATCH 09/12] cachefiles: defer exposing anon_fd until after
copy_to_user() succeeds
On 2024/5/6 11:24, Jingbo Xu wrote:
>
> On 4/24/24 11:39 AM, libaokun@...weicloud.com wrote:
>> From: Baokun Li <libaokun1@...wei.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@...wei.com>
>> Signed-off-by: Baokun Li <libaokun1@...wei.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);
Powered by blists - more mailing lists