| Message ID | 20240424033916.2748488-2-libaokun@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cachefiles: some bugfixes and cleanups for ondemand requests | expand |
在 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); >
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); >
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); >>> >
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); >>>>
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);