| 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
| ||
|
Message-ID: <d62b162d-acb3-2fa7-085e-79da3278091a@huaweicloud.com>
Date: Mon, 20 May 2024 17:07:24 +0800
From: Baokun Li <libaokun@...weicloud.com>
To: Jingbo Xu <jefflexu@...ux.alibaba.com>, netfs@...ts.linux.dev,
dhowells@...hat.com, jlayton@...nel.org
Cc: hsiangkao@...ux.alibaba.com, zhujia.zj@...edance.com,
linux-erofs@...ts.ozlabs.org, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, yangerkun@...wei.com, houtao1@...wei.com,
yukuai3@...wei.com, wozizhi@...wei.com, Baokun Li <libaokun1@...wei.com>,
libaokun@...weicloud.com
Subject: Re: [PATCH v2 08/12] cachefiles: never get a new anonymous fd if
ondemand_id is valid
On 2024/5/20 16:43, Jingbo Xu wrote:
>
> On 5/15/24 4:45 PM, libaokun@...weicloud.com wrote:
>> From: Baokun Li <libaokun1@...wei.com>
>>
>> Now every time the daemon reads an open request, it gets a new anonymous fd
>> and ondemand_id. With the introduction of "restore", it is possible to read
>> the same open request more than once, and therefore an object can have more
>> than one anonymous fd.
>>
>> If the anonymous fd is not unique, the following concurrencies will result
>> in an fd leak:
>>
>> t1 | t2 | t3
>> ------------------------------------------------------------
>> cachefiles_ondemand_init_object
>> cachefiles_ondemand_send_req
>> REQ_A = kzalloc(sizeof(*req) + data_len)
>> wait_for_completion(&REQ_A->done)
>> cachefiles_daemon_read
>> cachefiles_ondemand_daemon_read
>> REQ_A = cachefiles_ondemand_select_req
>> cachefiles_ondemand_get_fd
>> load->fd = fd0
>> ondemand_id = object_id0
>> ------ restore ------
>> cachefiles_ondemand_restore
>> // restore REQ_A
>> cachefiles_daemon_read
>> cachefiles_ondemand_daemon_read
>> REQ_A = cachefiles_ondemand_select_req
>> cachefiles_ondemand_get_fd
>> load->fd = fd1
>> ondemand_id = object_id1
>> process_open_req(REQ_A)
>> write(devfd, ("copen %u,%llu", msg->msg_id, size))
>> cachefiles_ondemand_copen
>> xa_erase(&cache->reqs, id)
>> complete(&REQ_A->done)
>> kfree(REQ_A)
>> process_open_req(REQ_A)
>> // copen fails due to no req
>> // daemon close(fd1)
>> cachefiles_ondemand_fd_release
>> // set object closed
>> -- umount --
>> cachefiles_withdraw_cookie
>> cachefiles_ondemand_clean_object
>> cachefiles_ondemand_init_close_req
>> if (!cachefiles_ondemand_object_is_open(object))
>> return -ENOENT;
>> // The fd0 is not closed until the daemon exits.
>>
>> However, the anonymous fd holds the reference count of the object and the
>> object holds the reference count of the cookie. So even though the cookie
>> has been relinquished, it will not be unhashed and freed until the daemon
>> exits.
>>
>> In fscache_hash_cookie(), when the same cookie is found in the hash list,
>> if the cookie is set with the FSCACHE_COOKIE_RELINQUISHED bit, then the new
>> cookie waits for the old cookie to be unhashed, while the old cookie is
>> waiting for the leaked fd to be closed, if the daemon does not exit in time
>> it will trigger a hung task.
>>
>> To avoid this, allocate a new anonymous fd only if no anonymous fd has
>> been allocated (ondemand_id == 0) or if the previously allocated anonymous
>> fd has been closed (ondemand_id == -1). Moreover, returns an error if
>> ondemand_id is valid, letting the daemon know that the current userland
>> restore logic is abnormal and needs to be checked.
>>
>> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
>> Signed-off-by: Baokun Li <libaokun1@...wei.com>
> The LOCs of this fix is quite under control. But still it seems that
> the worst consequence is that the (potential) malicious daemon gets
> hung. No more effect to the system or other processes. Or does a
> non-malicious daemon have any chance having the same issue?
If we enable hung_task_panic, it may cause panic to crash the server.
>> ---
>> fs/cachefiles/ondemand.c | 34 ++++++++++++++++++++++++++++------
>> 1 file changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>> index d04ddc6576e3..d2d4e27fca6f 100644
>> --- a/fs/cachefiles/ondemand.c
>> +++ b/fs/cachefiles/ondemand.c
>> @@ -14,11 +14,18 @@ static int cachefiles_ondemand_fd_release(struct inode *inode,
>> struct file *file)
>> {
>> struct cachefiles_object *object = file->private_data;
>> - struct cachefiles_cache *cache = object->volume->cache;
>> - struct cachefiles_ondemand_info *info = object->ondemand;
>> + struct cachefiles_cache *cache;
>> + struct cachefiles_ondemand_info *info;
>> int object_id;
>> struct cachefiles_req *req;
>> - XA_STATE(xas, &cache->reqs, 0);
>> + XA_STATE(xas, NULL, 0);
>> +
>> + if (!object)
>> + return 0;
>> +
>> + info = object->ondemand;
>> + cache = object->volume->cache;
>> + xas.xa = &cache->reqs;
>>
>> xa_lock(&cache->reqs);
>> spin_lock(&info->lock);
>> @@ -288,22 +295,39 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
>> goto err_put_fd;
>> }
>>
>> + spin_lock(&object->ondemand->lock);
>> + if (object->ondemand->ondemand_id > 0) {
>> + spin_unlock(&object->ondemand->lock);
>> + /* Pair with check in cachefiles_ondemand_fd_release(). */
>> + file->private_data = NULL;
>> + ret = -EEXIST;
>> + goto err_put_file;
>> + }
>> +
>> file->f_mode |= FMODE_PWRITE | FMODE_LSEEK;
>> fd_install(fd, file);
>>
>> load = (void *)req->msg.data;
>> load->fd = fd;
>> object->ondemand->ondemand_id = object_id;
>> + spin_unlock(&object->ondemand->lock);
>>
>> cachefiles_get_unbind_pincount(cache);
>> trace_cachefiles_ondemand_open(object, &req->msg, load);
>> return 0;
>>
>> +err_put_file:
>> + fput(file);
>> err_put_fd:
>> put_unused_fd(fd);
>> err_free_id:
>> xa_erase(&cache->ondemand_ids, object_id);
>> err:
>> + spin_lock(&object->ondemand->lock);
>> + /* Avoid marking an opened object as closed. */
>> + if (object->ondemand->ondemand_id <= 0)
>> + cachefiles_ondemand_set_object_close(object);
>> + spin_unlock(&object->ondemand->lock);
>> cachefiles_put_object(object, cachefiles_obj_put_ondemand_fd);
>> return ret;
>> }
>> @@ -386,10 +410,8 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>>
>> if (msg->opcode == CACHEFILES_OP_OPEN) {
>> ret = cachefiles_ondemand_get_fd(req);
>> - if (ret) {
>> - cachefiles_ondemand_set_object_close(req->object);
>> + if (ret)
>> goto out;
>> - }
>> }
>>
>> msg->msg_id = xas.xa_index;
--
With Best Regards,
Baokun Li
Powered by blists - more mailing lists