Re: [PATCH 09/12] cachefiles: defer exposing anon_fd until after copy_to_user() succeeds

[Baokun Li <libaokun@xxxxxxxxxxxxxxx>]

On 2024/5/6 11:24, Jingbo Xu wrote:
> On 4/24/24 11:39 AM, libaokun@xxxxxxxxxxxxxxx wrote:
> > From: Baokun Li <libaokun1@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
> > ---
> >   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);