| Message ID | 20240515084601.3240503-10-libaokun@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cachefiles: some bugfixes and cleanups for ondemand requests | expand |
On 5/15/24 4:45 PM, 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. > > 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 <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 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. > { > 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. > 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);
On 2024/5/20 17:39, Jingbo Xu wrote: > > On 5/15/24 4:45 PM, 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. >> >> 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 <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 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);
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) { 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; 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: @@ -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);