diff mbox series

[v2,01/16] ovl: add permission hooks outside of do_splice_direct()

Message ID 20231122122715.2561213-2-amir73il@gmail.com (mailing list archive)
State New
Headers show
Series Tidy up file permission hooks | expand

Commit Message

Amir Goldstein Nov. 22, 2023, 12:27 p.m. UTC 
The main callers of do_splice_direct() also call rw_verify_area() for
the entire range that is being copied, e.g. by vfs_copy_file_range()
or do_sendfile() before calling do_splice_direct().

The only caller that does not have those checks for entire range is
ovl_copy_up_file().  In preparation for removing the checks inside
do_splice_direct(), add rw_verify_area() call in ovl_copy_up_file().

For extra safety, perform minimal sanity checks from rw_verify_area()
for non negative offsets also in the copy up do_splice_direct() loop
without calling the file permission hooks.

This is needed for fanotify "pre content" events.

Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Comments

Christoph Hellwig Nov. 23, 2023, 7:35 a.m. UTC | #1 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Jan Kara Nov. 23, 2023, 4:28 p.m. UTC | #2 
On Wed 22-11-23 14:27:00, Amir Goldstein wrote:
> The main callers of do_splice_direct() also call rw_verify_area() for
> the entire range that is being copied, e.g. by vfs_copy_file_range()
> or do_sendfile() before calling do_splice_direct().
> 
> The only caller that does not have those checks for entire range is
> ovl_copy_up_file().  In preparation for removing the checks inside
> do_splice_direct(), add rw_verify_area() call in ovl_copy_up_file().
> 
> For extra safety, perform minimal sanity checks from rw_verify_area()
> for non negative offsets also in the copy up do_splice_direct() loop
> without calling the file permission hooks.
> 
> This is needed for fanotify "pre content" events.
> 
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> ---
>  fs/overlayfs/copy_up.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 4382881b0709..106f8643af3b 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -230,6 +230,19 @@ static int ovl_copy_fileattr(struct inode *inode, const struct path *old,
>  	return ovl_real_fileattr_set(new, &newfa);
>  }
>  
> +static int ovl_verify_area(loff_t pos, loff_t pos2, loff_t len, loff_t totlen)
> +{
> +	loff_t tmp;
> +
> +	if (WARN_ON_ONCE(pos != pos2))
> +		return -EIO;
> +	if (WARN_ON_ONCE(pos < 0 || len < 0 || totlen < 0))
> +		return -EIO;
> +	if (WARN_ON_ONCE(check_add_overflow(pos, len, &tmp)))
> +		return -EIO;
> +	return 0;
> +}
> +
>  static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
>  			    struct file *new_file, loff_t len)
>  {
> @@ -244,13 +257,20 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
>  	int error = 0;
>  
>  	ovl_path_lowerdata(dentry, &datapath);
> -	if (WARN_ON(datapath.dentry == NULL))
> +	if (WARN_ON_ONCE(datapath.dentry == NULL) ||
> +	    WARN_ON_ONCE(len < 0))
>  		return -EIO;
>  
>  	old_file = ovl_path_open(&datapath, O_LARGEFILE | O_RDONLY);
>  	if (IS_ERR(old_file))
>  		return PTR_ERR(old_file);
>  
> +	error = rw_verify_area(READ, old_file, &old_pos, len);
> +	if (!error)
> +		error = rw_verify_area(WRITE, new_file, &new_pos, len);
> +	if (error)
> +		goto out_fput;
> +
>  	/* Try to use clone_file_range to clone up within the same fs */
>  	ovl_start_write(dentry);
>  	cloned = do_clone_file_range(old_file, 0, new_file, 0, len, 0);
> @@ -309,6 +329,10 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
>  			}
>  		}
>  
> +		error = ovl_verify_area(old_pos, new_pos, this_len, len);
> +		if (error)
> +			break;
> +
>  		ovl_start_write(dentry);
>  		bytes = do_splice_direct(old_file, &old_pos,
>  					 new_file, &new_pos,
> -- 
> 2.34.1
>
diff mbox series

Patch

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 4382881b0709..106f8643af3b 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -230,6 +230,19 @@  static int ovl_copy_fileattr(struct inode *inode, const struct path *old,
 	return ovl_real_fileattr_set(new, &newfa);
 }
 
+static int ovl_verify_area(loff_t pos, loff_t pos2, loff_t len, loff_t totlen)
+{
+	loff_t tmp;
+
+	if (WARN_ON_ONCE(pos != pos2))
+		return -EIO;
+	if (WARN_ON_ONCE(pos < 0 || len < 0 || totlen < 0))
+		return -EIO;
+	if (WARN_ON_ONCE(check_add_overflow(pos, len, &tmp)))
+		return -EIO;
+	return 0;
+}
+
 static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
 			    struct file *new_file, loff_t len)
 {
@@ -244,13 +257,20 @@  static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
 	int error = 0;
 
 	ovl_path_lowerdata(dentry, &datapath);
-	if (WARN_ON(datapath.dentry == NULL))
+	if (WARN_ON_ONCE(datapath.dentry == NULL) ||
+	    WARN_ON_ONCE(len < 0))
 		return -EIO;
 
 	old_file = ovl_path_open(&datapath, O_LARGEFILE | O_RDONLY);
 	if (IS_ERR(old_file))
 		return PTR_ERR(old_file);
 
+	error = rw_verify_area(READ, old_file, &old_pos, len);
+	if (!error)
+		error = rw_verify_area(WRITE, new_file, &new_pos, len);
+	if (error)
+		goto out_fput;
+
 	/* Try to use clone_file_range to clone up within the same fs */
 	ovl_start_write(dentry);
 	cloned = do_clone_file_range(old_file, 0, new_file, 0, len, 0);
@@ -309,6 +329,10 @@  static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
 			}
 		}
 
+		error = ovl_verify_area(old_pos, new_pos, this_len, len);
+		if (error)
+			break;
+
 		ovl_start_write(dentry);
 		bytes = do_splice_direct(old_file, &old_pos,
 					 new_file, &new_pos,