From: Jingbo Xu <jefflexu@linux.alibaba.com>
To: libaokun@huaweicloud.com, netfs@lists.linux.dev
Cc: dhowells@redhat.com, jlayton@kernel.org, zhujia.zj@bytedance.com,
linux-erofs@lists.ozlabs.org, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, Baokun Li <libaokun1@huawei.com>,
Hou Tao <houtao1@huawei.com>
Subject: Re: [PATCH 09/12] cachefiles: defer exposing anon_fd until after copy_to_user() succeeds
Date: Mon, 6 May 2024 11:24:52 +0800 [thread overview]
Message-ID: <e0fc24d5-49c5-4a75-86f9-43adc763066f@linux.alibaba.com> (raw)
In-Reply-To: <20240424033916.2748488-10-libaokun@huaweicloud.com>
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()?
>
> 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);
--
Thanks,
Jingbo
next prev parent reply other threads:[~2024-05-06 3:24 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-24 3:39 [PATCH 00/12] cachefiles: some bugfixes and cleanups for ondemand requests libaokun
2024-04-24 3:39 ` [PATCH 01/12] cachefiles: remove request from xarry during flush requests libaokun
2024-04-25 3:13 ` Jia Zhu
2024-05-06 3:48 ` Jingbo Xu
2024-05-06 3:57 ` Baokun Li
2024-05-06 5:50 ` Jingbo Xu
2024-05-07 6:52 ` Baokun Li
2024-04-24 3:39 ` [PATCH 02/12] cachefiles: remove err_put_fd tag in cachefiles_ondemand_daemon_read() libaokun
2024-04-25 3:17 ` Jia Zhu
2024-05-06 3:55 ` Jingbo Xu
2024-05-06 4:02 ` Baokun Li
2024-04-24 3:39 ` [PATCH 03/12] cachefiles: fix slab-use-after-free in cachefiles_ondemand_get_fd() libaokun
2024-04-24 14:55 ` Jia Zhu
2024-04-25 1:33 ` Baokun Li
2024-04-25 3:39 ` Jia Zhu
2024-04-24 3:39 ` [PATCH 04/12] cachefiles: fix slab-use-after-free in cachefiles_ondemand_daemon_read() libaokun
2024-04-25 3:42 ` [External] " Jia Zhu
2024-04-24 3:39 ` [PATCH 05/12] cachefiles: add output string to cachefiles_obj_[get|put]_ondemand_fd libaokun
2024-04-24 3:39 ` [PATCH 06/12] cachefiles: add consistency check for copen/cread libaokun
2024-05-06 2:31 ` Jingbo Xu
2024-05-06 3:12 ` Baokun Li
2024-04-24 3:39 ` [PATCH 07/12] cachefiles: add spin_lock for cachefiles_ondemand_info libaokun
2024-05-06 2:55 ` Jingbo Xu
2024-05-06 3:23 ` Baokun Li
2024-04-24 3:39 ` [PATCH 08/12] cachefiles: never get a new anon fd if ondemand_id is valid libaokun
2024-05-06 3:09 ` Jingbo Xu
2024-05-07 9:32 ` Baokun Li
2024-04-24 3:39 ` [PATCH 09/12] cachefiles: defer exposing anon_fd until after copy_to_user() succeeds libaokun
2024-05-06 3:24 ` Jingbo Xu [this message]
2024-05-06 3:34 ` Baokun Li
2024-04-24 3:39 ` [PATCH 10/12] cachefiles: Set object to close if ondemand_id < 0 in copen libaokun
2024-04-25 4:56 ` Jia Zhu
2024-04-24 3:39 ` [PATCH 11/12] cachefiles: flush all requests after setting CACHEFILES_DEAD libaokun
2024-04-24 3:39 ` [PATCH 12/12] cachefiles: make on-demand read killable libaokun
2024-04-25 5:15 ` Jia Zhu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e0fc24d5-49c5-4a75-86f9-43adc763066f@linux.alibaba.com \
--to=jefflexu@linux.alibaba.com \
--cc=dhowells@redhat.com \
--cc=houtao1@huawei.com \
--cc=jlayton@kernel.org \
--cc=libaokun1@huawei.com \
--cc=libaokun@huaweicloud.com \
--cc=linux-erofs@lists.ozlabs.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netfs@lists.linux.dev \
--cc=zhujia.zj@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).