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>
Subject: Re: [PATCH 06/12] cachefiles: add consistency check for copen/cread
Date: Mon, 6 May 2024 10:31:04 +0800 [thread overview]
Message-ID: <75566e68-bb5f-4458-8140-a59f263cc98a@linux.alibaba.com> (raw)
In-Reply-To: <20240424033916.2748488-7-libaokun@huaweicloud.com>
Hi Baokun,
Thanks for improving on this!
On 4/24/24 11:39 AM, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
>
> This prevents malicious processes from completing random copen/cread
> requests and crashing the system. Added checks are listed below:
>
> * Generic, copen can only complete open requests, and cread can only
> complete read requests.
> * For copen, ondemand_id must not be 0, because this indicates that the
> request has not been read by the daemon.
> * For cread, the object corresponding to fd and req should be the same.
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
> fs/cachefiles/ondemand.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> index bb94ef6a6f61..898fab68332b 100644
> --- a/fs/cachefiles/ondemand.c
> +++ b/fs/cachefiles/ondemand.c
> @@ -82,12 +82,12 @@ static loff_t cachefiles_ondemand_fd_llseek(struct file *filp, loff_t pos,
> }
>
> static long cachefiles_ondemand_fd_ioctl(struct file *filp, unsigned int ioctl,
> - unsigned long arg)
> + unsigned long id)
> {
> struct cachefiles_object *object = filp->private_data;
> struct cachefiles_cache *cache = object->volume->cache;
> struct cachefiles_req *req;
> - unsigned long id;
> + XA_STATE(xas, &cache->reqs, id);
>
> if (ioctl != CACHEFILES_IOC_READ_COMPLETE)
> return -EINVAL;
> @@ -95,10 +95,15 @@ static long cachefiles_ondemand_fd_ioctl(struct file *filp, unsigned int ioctl,
> if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
> return -EOPNOTSUPP;
>
> - id = arg;
> - req = xa_erase(&cache->reqs, id);
> - if (!req)
> + xa_lock(&cache->reqs);
> + req = xas_load(&xas);
> + if (!req || req->msg.opcode != CACHEFILES_OP_READ ||
> + req->object != object) {
> + xa_unlock(&cache->reqs);
> return -EINVAL;
> + }
> + xas_store(&xas, NULL);
> + xa_unlock(&cache->reqs);
>
> trace_cachefiles_ondemand_cread(object, id);
> complete(&req->done);
> @@ -126,6 +131,7 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
> unsigned long id;
> long size;
> int ret;
> + XA_STATE(xas, &cache->reqs, 0);
>
> if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
> return -EOPNOTSUPP;
> @@ -149,9 +155,16 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
> if (ret)
> return ret;
>
> - req = xa_erase(&cache->reqs, id);
> - if (!req)
> + xa_lock(&cache->reqs);
> + xas.xa_index = id;
> + req = xas_load(&xas);
> + if (!req || req->msg.opcode != CACHEFILES_OP_OPEN ||
> + !req->object->ondemand->ondemand_id) {
> + xa_unlock(&cache->reqs);
> return -EINVAL;
> + }
> + xas_store(&xas, NULL);
> + xa_unlock(&cache->reqs);
>
> /* fail OPEN request if copen format is invalid */
> ret = kstrtol(psize, 0, &size);
The code looks good to me, but I still have some questions.
First, what's the worst consequence if the daemon misbehaves like
completing random copen/cread requests? I mean, does that affect other
processes on the system besides the direct users of the ondemand mode,
e.g. will the misbehavior cause system crash?
Besides, it seems that the above security improvement is only "best
effort". It can not completely prevent a malicious misbehaved daemon
from completing random copen/cread requests, right?
--
Thanks,
Jingbo
next prev parent reply other threads:[~2024-05-06 2:31 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 [this message]
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
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=75566e68-bb5f-4458-8140-a59f263cc98a@linux.alibaba.com \
--to=jefflexu@linux.alibaba.com \
--cc=dhowells@redhat.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).