| Message ID | 20240424033916.2748488-10-libaokun@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cachefiles: some bugfixes and cleanups for ondemand requests | expand |
On 4/24/24 11:39 AM, libaokun@huaweicloud.com wrote: > From: Baokun Li <libaokun1@huawei.com> > > 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. Good catch! > > To avoid this, we will make the anonymous fd accessible to the userland by > executing fd_install() after copy_to_user() has succeeded, and by this > point we must have already grabbed the reference count of the cache. Why we must execute fd_install() after copy_to_user() has succeeded? Why not grab a reference to the cache before fd_install()? > > Suggested-by: Hou Tao <houtao1@huawei.com> > Signed-off-by: Baokun Li <libaokun1@huawei.com> > --- > 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 0cf63bfedc9e..7c2d43104120 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)) > @@ -244,14 +249,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) > { > 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); > @@ -263,16 +268,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; > } > > @@ -281,15 +286,14 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req) > spin_unlock(&object->ondemand->lock); > ret = -EEXIST; > /* Avoid performing cachefiles_ondemand_fd_release(). */ > - file->private_data = NULL; > + anon_file->file->private_data = NULL; > 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); > > @@ -298,9 +302,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; > err_put_fd: > - put_unused_fd(fd); > + put_unused_fd(anon_file->fd); > + anon_file->fd = ret; > err_free_id: > xa_erase(&cache->ondemand_ids, object_id); > err: > @@ -357,6 +363,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); > @@ -390,7 +397,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; > } > @@ -398,10 +405,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);
On 2024/5/6 11:24, Jingbo Xu wrote: > > On 4/24/24 11:39 AM, libaokun@huaweicloud.com wrote: >> From: Baokun Li <libaokun1@huawei.com> >> >> 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. > Good catch! > >> To avoid this, we will make the anonymous fd accessible to the userland by >> executing fd_install() after copy_to_user() has succeeded, and by this >> point we must have already grabbed the reference count of the cache. > Why we must execute fd_install() after copy_to_user() has succeeded? > Why not grab a reference to the cache before fd_install()? Two things are actually done here: 1) Grab a reference to the cache before fd_install() 2) By kernel convention, fd is taken over by the user land after fd_install() is executed, and the kernel does not call close_fd() after this, so fd_install() is called after everything is ready. Thanks, Baokun > >> Suggested-by: Hou Tao <houtao1@huawei.com> >> Signed-off-by: Baokun Li <libaokun1@huawei.com> >> --- >> 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 0cf63bfedc9e..7c2d43104120 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)) >> @@ -244,14 +249,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) >> { >> 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); >> @@ -263,16 +268,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; >> } >> >> @@ -281,15 +286,14 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req) >> spin_unlock(&object->ondemand->lock); >> ret = -EEXIST; >> /* Avoid performing cachefiles_ondemand_fd_release(). */ >> - file->private_data = NULL; >> + anon_file->file->private_data = NULL; >> 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); >> >> @@ -298,9 +302,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; >> err_put_fd: >> - put_unused_fd(fd); >> + put_unused_fd(anon_file->fd); >> + anon_file->fd = ret; >> err_free_id: >> xa_erase(&cache->ondemand_ids, object_id); >> err: >> @@ -357,6 +363,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); >> @@ -390,7 +397,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; >> } >> @@ -398,10 +405,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);
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c index 0cf63bfedc9e..7c2d43104120 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)) @@ -244,14 +249,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) { 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); @@ -263,16 +268,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; } @@ -281,15 +286,14 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req) spin_unlock(&object->ondemand->lock); ret = -EEXIST; /* Avoid performing cachefiles_ondemand_fd_release(). */ - file->private_data = NULL; + anon_file->file->private_data = NULL; 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); @@ -298,9 +302,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; err_put_fd: - put_unused_fd(fd); + put_unused_fd(anon_file->fd); + anon_file->fd = ret; err_free_id: xa_erase(&cache->ondemand_ids, object_id); err: @@ -357,6 +363,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); @@ -390,7 +397,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; } @@ -398,10 +405,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);