From: Baokun Li <libaokun1@huawei.com>
This prevents concurrency from causing access to a freed req.
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
fs/cachefiles/daemon.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
index 6465e2574230..ccb7b707ea4b 100644
--- a/fs/cachefiles/daemon.c
+++ b/fs/cachefiles/daemon.c
@@ -159,6 +159,7 @@ static void cachefiles_flush_reqs(struct cachefiles_cache *cache)
xa_for_each(xa, index, req) {
req->error = -EIO;
complete(&req->done);
+ __xa_erase(xa, index);
}
xa_unlock(xa);
--
2.39.2
On 4/24/24 11:39 AM, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
>
> This prevents concurrency from causing access to a freed req.
Could you give more details on how the concurrent access will happen?
How could another process access the &cache->reqs xarray after it has
been flushed?
> ---
> fs/cachefiles/daemon.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
> index 6465e2574230..ccb7b707ea4b 100644
> --- a/fs/cachefiles/daemon.c
> +++ b/fs/cachefiles/daemon.c
> @@ -159,6 +159,7 @@ static void cachefiles_flush_reqs(struct cachefiles_cache *cache)
> xa_for_each(xa, index, req) {
> req->error = -EIO;
> complete(&req->done);
> + __xa_erase(xa, index);
> }
> xa_unlock(xa);
>
--
Thanks,
Jingbo
On 2024/5/6 11:48, Jingbo Xu wrote:
>
> On 4/24/24 11:39 AM, libaokun@huaweicloud.com wrote:
>> From: Baokun Li <libaokun1@huawei.com>
>>
>> This prevents concurrency from causing access to a freed req.
> Could you give more details on how the concurrent access will happen?
> How could another process access the &cache->reqs xarray after it has
> been flushed?
Similar logic to restore leading to UAF:
mount | daemon_thread1 | daemon_thread2
------------------------------------------------------------
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
copy_to_user(_buffer, msg, n)
process_open_req(REQ_A)
// close dev fd
cachefiles_flush_reqs
complete(&REQ_A->done)
kfree(REQ_A)
cachefiles_ondemand_get_fd(REQ_A)
fd = get_unused_fd_flags
file = anon_inode_getfile
fd_install(fd, file)
load = (void *)REQ_A->msg.data;
load->fd = fd;
// load UAF !!!
>> ---
>> fs/cachefiles/daemon.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
>> index 6465e2574230..ccb7b707ea4b 100644
>> --- a/fs/cachefiles/daemon.c
>> +++ b/fs/cachefiles/daemon.c
>> @@ -159,6 +159,7 @@ static void cachefiles_flush_reqs(struct cachefiles_cache *cache)
>> xa_for_each(xa, index, req) {
>> req->error = -EIO;
>> complete(&req->done);
>> + __xa_erase(xa, index);
>> }
>> xa_unlock(xa);
>>
On 5/6/24 11:57 AM, Baokun Li wrote:
> On 2024/5/6 11:48, Jingbo Xu wrote:
>>
>> On 4/24/24 11:39 AM, libaokun@huaweicloud.com wrote:
>>> From: Baokun Li <libaokun1@huawei.com>
>>>
>>> This prevents concurrency from causing access to a freed req.
>> Could you give more details on how the concurrent access will happen?
>> How could another process access the &cache->reqs xarray after it has
>> been flushed?
>
> Similar logic to restore leading to UAF:
>
> mount | daemon_thread1 | daemon_thread2
> ------------------------------------------------------------
> 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
> copy_to_user(_buffer, msg, n)
> process_open_req(REQ_A)
> // close dev fd
> cachefiles_flush_reqs
> complete(&REQ_A->done)
> kfree(REQ_A)
> cachefiles_ondemand_get_fd(REQ_A)
> fd = get_unused_fd_flags
> file = anon_inode_getfile
> fd_install(fd, file)
> load = (void *)REQ_A->msg.data;
> load->fd = fd;
> // load UAF !!!
How could the second cachefiles_ondemand_get_fd() get called here, given
the cache has been flushed and flagged as DEAD?
>
>>> ---
>>> fs/cachefiles/daemon.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
>>> index 6465e2574230..ccb7b707ea4b 100644
>>> --- a/fs/cachefiles/daemon.c
>>> +++ b/fs/cachefiles/daemon.c
>>> @@ -159,6 +159,7 @@ static void cachefiles_flush_reqs(struct
>>> cachefiles_cache *cache)
>>> xa_for_each(xa, index, req) {
>>> req->error = -EIO;
>>> complete(&req->done);
>>> + __xa_erase(xa, index);
>>> }
>>> xa_unlock(xa);
>>>
>
--
Thanks,
Jingbo
Hi Jingbo,
Sorry for the late reply.
On 2024/5/6 13:50, Jingbo Xu wrote:
>
> On 5/6/24 11:57 AM, Baokun Li wrote:
>> On 2024/5/6 11:48, Jingbo Xu wrote:
>>> On 4/24/24 11:39 AM, libaokun@huaweicloud.com wrote:
>>>> From: Baokun Li <libaokun1@huawei.com>
>>>>
>>>> This prevents concurrency from causing access to a freed req.
>>> Could you give more details on how the concurrent access will happen?
>>> How could another process access the &cache->reqs xarray after it has
>>> been flushed?
>> Similar logic to restore leading to UAF:
>>
>> mount | daemon_thread1 | daemon_thread2
>> ------------------------------------------------------------
>> 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
>> copy_to_user(_buffer, msg, n)
>> process_open_req(REQ_A)
>> // close dev fd
>> cachefiles_flush_reqs
>> complete(&REQ_A->done)
>> kfree(REQ_A)
>
>> cachefiles_ondemand_get_fd(REQ_A)
>> fd = get_unused_fd_flags
>> file = anon_inode_getfile
>> fd_install(fd, file)
>> load = (void *)REQ_A->msg.data;
>> load->fd = fd;
>> // load UAF !!!
> How could the second cachefiles_ondemand_get_fd() get called here, given
> the cache has been flushed and flagged as DEAD?
>
I was in a bit of a rush to reply earlier, and that graph above is
wrong. Please see the one below:
mount | daemon_thread1 | daemon_thread2
------------------------------------------------------------
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
// close dev fd
cachefiles_flush_reqs
complete(&REQ_A->done)
kfree(REQ_A)
xa_lock(&cache->reqs);
cachefiles_ondemand_select_req
req->msg.opcode != CACHEFILES_OP_READ
// req use-after-free !!!
xa_unlock(&cache->reqs);
xa_destroy(&cache->reqs)
Even with CACHEFILES_DEAD set, we can still read the requests, so
accessing it after the request has been freed will trigger use-after-free.
Thanks,
Baokun
>>>> ---
>>>> fs/cachefiles/daemon.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
>>>> index 6465e2574230..ccb7b707ea4b 100644
>>>> --- a/fs/cachefiles/daemon.c
>>>> +++ b/fs/cachefiles/daemon.c
>>>> @@ -159,6 +159,7 @@ static void cachefiles_flush_reqs(struct
>>>> cachefiles_cache *cache)
>>>> xa_for_each(xa, index, req) {
>>>> req->error = -EIO;
>>>> complete(&req->done);
>>>> + __xa_erase(xa, index);
>>>> }
>>>> xa_unlock(xa);
>>>>
在 2024/4/24 11:39, libaokun@huaweicloud.com 写道:
> From: Baokun Li <libaokun1@huawei.com>
>
> This prevents concurrency from causing access to a freed req.
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Jia Zhu <zhujia.zj@bytedance.com>
> ---
> fs/cachefiles/daemon.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
> index 6465e2574230..ccb7b707ea4b 100644
> --- a/fs/cachefiles/daemon.c
> +++ b/fs/cachefiles/daemon.c
> @@ -159,6 +159,7 @@ static void cachefiles_flush_reqs(struct cachefiles_cache *cache)
> xa_for_each(xa, index, req) {
> req->error = -EIO;
> complete(&req->done);
> + __xa_erase(xa, index);
> }
> xa_unlock(xa);
>
© 2016 - 2026 Red Hat, Inc.