On 5/15/24 4:45 PM, [email protected] wrote:
> From: Baokun Li <[email protected]>
>
> Even with CACHEFILES_DEAD set, we can still read the requests, so in the
> following concurrency the request may be used after it has been freed:
>
> 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)
>
> Hence remove requests from cache->reqs when flushing them to avoid
> accessing freed requests.
>
> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
> Signed-off-by: Baokun Li <[email protected]>
> Reviewed-by: Jia Zhu <[email protected]>

Reviewed-by: Jingbo Xu <[email protected]>

> ---
> 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

2024-05-20 07:36:58

by Jingbo Xu

[permalink] [raw]

On 2024/5/20 17:39, Jingbo Xu wrote:
>
> On 5/15/24 4:45 PM, [email protected] wrote:
>> From: Baokun Li <[email protected]>
>>
>> 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.
>>
>> So grab the cache reference count before fd_install(). In addition, by
>> kernel convention, fd is taken over by the user land after fd_install(),
>> and the kernel should not call close_fd() after that, i.e., it should call
>> fd_install() after everything is ready, thus fd_install() is called after
>> copy_to_user() succeeds.
>>
>> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
>> Suggested-by: Hou Tao <[email protected]>
>> Signed-off-by: Baokun Li <[email protected]>
>> ---
>> 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 d2d4e27fca6f..3a36613e00a7 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))
>> @@ -263,14 +268,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)
>
> How about:
>
> int cachefiles_ondemand_get_fd(struct cachefiles_req *req, int *fd,
> struct file *file) ?
>
> It isn't worth introducing a new structure as it is used only for
> parameter passing.
>
It's just a different code style preference, and internally we think

it makes the code look clearer when encapsulated this way.

>> {
>> 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);
>> @@ -282,16 +287,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;
>> }
>>
>> @@ -299,16 +304,15 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
>> if (object->ondemand->ondemand_id > 0) {
>> spin_unlock(&object->ondemand->lock);
>> /* Pair with check in cachefiles_ondemand_fd_release(). */
>> - file->private_data = NULL;
>> + anon_file->file->private_data = NULL;
>> ret = -EEXIST;
>> 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);
>>
>> @@ -317,9 +321,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;
> When cachefiles_ondemand_get_fd() returns failure, anon_file->file is
> not used, and thus I don't think it is worth resetting anon_file->file
> to NULL. Or we could assign fd and struct file at the very end when all
> succeed.
Nulling pointers that are no longer in use is a safer coding convention,
which goes some way to avoiding double free or use-after-free.
Moreover it's in the error branch, so it doesn't cost anything.
>> err_put_fd:
>> - put_unused_fd(fd);
>> + put_unused_fd(anon_file->fd);
>> + anon_file->fd = ret;
> Ditto.
>
>> err_free_id:
>> xa_erase(&cache->ondemand_ids, object_id);
>> err:
>> @@ -376,6 +382,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);
>> @@ -409,7 +416,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;
>> }
>> @@ -417,10 +424,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);

--
With Best Regards,
Baokun Li

2024-05-20 12:22:42

by Baokun Li

[permalink] [raw]

Subject: Re: [PATCH v2 03/12] cachefiles: fix slab-use-after-free in cachefiles_ondemand_get_fd()

On 2024/5/20 17:10, Jingbo Xu wrote:
>
> On 5/20/24 4:38 PM, Baokun Li wrote:
>> Hi Jingbo,
>>
>> Thanks for your review!
>>
>> On 2024/5/20 15:24, Jingbo Xu wrote:
>>> On 5/15/24 4:45 PM, [email protected] wrote:
>>>> From: Baokun Li <[email protected]>
>>>>
>>>> We got the following issue in a fuzz test of randomly issuing the
>>>> restore
>>>> command:
>>>>
>>>> ==================================================================
>>>> BUG: KASAN: slab-use-after-free in
>>>> cachefiles_ondemand_daemon_read+0x609/0xab0
>>>> Write of size 4 at addr ffff888109164a80 by task ondemand-04-dae/4962
>>>>
>>>> CPU: 11 PID: 4962 Comm: ondemand-04-dae Not tainted 6.8.0-rc7-dirty #542
>>>> Call Trace:
>>>> kasan_report+0x94/0xc0
>>>> cachefiles_ondemand_daemon_read+0x609/0xab0
>>>> vfs_read+0x169/0xb50
>>>> ksys_read+0xf5/0x1e0
>>>>
>>>> Allocated by task 626:
>>>> __kmalloc+0x1df/0x4b0
>>>> cachefiles_ondemand_send_req+0x24d/0x690
>>>> cachefiles_create_tmpfile+0x249/0xb30
>>>> cachefiles_create_file+0x6f/0x140
>>>> cachefiles_look_up_object+0x29c/0xa60
>>>> cachefiles_lookup_cookie+0x37d/0xca0
>>>> fscache_cookie_state_machine+0x43c/0x1230
>>>> [...]
>>>>
>>>> Freed by task 626:
>>>> kfree+0xf1/0x2c0
>>>> cachefiles_ondemand_send_req+0x568/0x690
>>>> cachefiles_create_tmpfile+0x249/0xb30
>>>> cachefiles_create_file+0x6f/0x140
>>>> cachefiles_look_up_object+0x29c/0xa60
>>>> cachefiles_lookup_cookie+0x37d/0xca0
>>>> fscache_cookie_state_machine+0x43c/0x1230
>>>> [...]
>>>> ==================================================================
>>>>
>>>> Following is the process that triggers the issue:
>>>>
>>>>       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)
>>>>                                    ------ restore ------
>>>>                                    cachefiles_ondemand_restore
>>>>                                    xas_for_each(&xas, req, ULONG_MAX)
>>>>                                     xas_set_mark(&xas,
>>>> CACHEFILES_REQ_NEW);
>>>>
>>>>                                    cachefiles_daemon_read
>>>>                                     cachefiles_ondemand_daemon_read
>>>>                                      REQ_A =
>>>> cachefiles_ondemand_select_req
>>>>
>>>>               write(devfd, ("copen %u,%llu", msg->msg_id, size));
>>>>               cachefiles_ondemand_copen
>>>>                xa_erase(&cache->reqs, id)
>>>>                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 !!!
>>>>
>>>> This issue is caused by issuing a restore command when the daemon is
>>>> still
>>>> alive, which results in a request being processed multiple times thus
>>>> triggering a UAF. So to avoid this problem, add an additional reference
>>>> count to cachefiles_req, which is held while waiting and reading, and
>>>> then
>>>> released when the waiting and reading is over.
>>>>
>>>>
>>>> Note that since there is only one reference count for waiting, we
>>>> need to
>>>> avoid the same request being completed multiple times, so we can only
>>>> complete the request if it is successfully removed from the xarray.
>>> Sorry the above description makes me confused. As the same request may
>>> be got by different daemon threads multiple times, the introduced
>>> refcount mechanism can't protect it from being completed multiple times
>>> (which is expected). The refcount only protects it from being freed
>>> multiple times.
>> The idea here is that because the wait only holds one reference count,
>> complete(&req->done) can only be called when the req has been
>> successfully removed from the xarry, otherwise the following UAF may
>> occur:
>
> "complete(&req->done) can only be called when the req has been
> successfully removed from the xarry ..."
>
> How this is done? since the following xarray_erase() following the first
> xarray_erase() will fail as the xarray slot referred by the same id has
> already been erased?

Sorry just forgot to reply to this！

Yes, after loading the xas, the entry (aka req) is checked to see if it
meets
expectations, and only when it does do we null the xas and complete the
request.

--
With Best Regards,
Baokun Li