[PATCH v2] copy_file_range: limit size if in compat mode

[PATCH v2] copy_file_range: limit size if in compat mode

[Miklos Szeredi]

If the process runs in 32-bit compat mode, copy_file_range results can be
in the in-band error range.  In this case limit copy length to MAX_RW_COUNT
to prevent a signed overflow.

Reported-by: Florian Weimer <fweimer@redhat.com>
Closes: https://lore.kernel.org/all/lhuh5ynl8z5.fsf@oldenburg.str.redhat.com/
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
v2:
 - simplified logic (Amir)

 fs/read_write.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index c5b6265d984b..833bae068770 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1576,6 +1576,13 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	if (len == 0)
 		return 0;
 
+	/*
+	 * Make sure return value doesn't overflow in 32bit compat mode.  Also
+	 * limit the size for all cases except when calling ->copy_file_range().
+	 */
+	if (splice || !file_out->f_op->copy_file_range || in_compat_syscall())
+		len = min_t(size_t, MAX_RW_COUNT, len);
+
 	file_start_write(file_out);
 
 	/*
@@ -1589,9 +1596,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 						      len, flags);
 	} else if (!splice && file_in->f_op->remap_file_range && samesb) {
 		ret = file_in->f_op->remap_file_range(file_in, pos_in,
-				file_out, pos_out,
-				min_t(loff_t, MAX_RW_COUNT, len),
-				REMAP_FILE_CAN_SHORTEN);
+				file_out, pos_out, len, REMAP_FILE_CAN_SHORTEN);
 		/* fallback to splice */
 		if (ret <= 0)
 			splice = true;
@@ -1624,8 +1629,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	 * to splicing from input file, while file_start_write() is held on
 	 * the output file on a different sb.
 	 */
-	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
-			       min_t(size_t, len, MAX_RW_COUNT), 0);
+	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out, len, 0);
 done:
 	if (ret > 0) {
 		fsnotify_access(file_in);
-- 
2.49.0

Re: [PATCH v2] copy_file_range: limit size if in compat mode

[Amir Goldstein]

On Wed, Aug 13, 2025 at 5:11 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> If the process runs in 32-bit compat mode, copy_file_range results can be
> in the in-band error range.  In this case limit copy length to MAX_RW_COUNT
> to prevent a signed overflow.
>
> Reported-by: Florian Weimer <fweimer@redhat.com>
> Closes: https://lore.kernel.org/all/lhuh5ynl8z5.fsf@oldenburg.str.redhat.com/
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
> v2:
>  - simplified logic (Amir)
>
>  fs/read_write.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index c5b6265d984b..833bae068770 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1576,6 +1576,13 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>         if (len == 0)
>                 return 0;
>
> +       /*
> +        * Make sure return value doesn't overflow in 32bit compat mode.  Also
> +        * limit the size for all cases except when calling ->copy_file_range().
> +        */
> +       if (splice || !file_out->f_op->copy_file_range || in_compat_syscall())
> +               len = min_t(size_t, MAX_RW_COUNT, len);
> +
>         file_start_write(file_out);
>
>         /*
> @@ -1589,9 +1596,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>                                                       len, flags);
>         } else if (!splice && file_in->f_op->remap_file_range && samesb) {
>                 ret = file_in->f_op->remap_file_range(file_in, pos_in,
> -                               file_out, pos_out,
> -                               min_t(loff_t, MAX_RW_COUNT, len),
> -                               REMAP_FILE_CAN_SHORTEN);
> +                               file_out, pos_out, len, REMAP_FILE_CAN_SHORTEN);
>                 /* fallback to splice */
>                 if (ret <= 0)
>                         splice = true;
> @@ -1624,8 +1629,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>          * to splicing from input file, while file_start_write() is held on
>          * the output file on a different sb.
>          */
> -       ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
> -                              min_t(size_t, len, MAX_RW_COUNT), 0);
> +       ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out, len, 0);
>  done:
>         if (ret > 0) {
>                 fsnotify_access(file_in);
> --
> 2.49.0
>

Re: [PATCH v2] copy_file_range: limit size if in compat mode

[Chunsheng Luo]

On Wed, Aug 13, 2025 at 5:11 PM Miklos Szeredi wrote:
> @@ -1624,8 +1629,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>  	 * to splicing from input file, while file_start_write() is held on
>  	 * the output file on a different sb.
>  	 */
> -	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
> -			       min_t(size_t, len, MAX_RW_COUNT), 0);
> +	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out, len, 0);
>  done:
>  	if (ret > 0) {
>  		fsnotify_access(file_in);

There is no problem with submission, but I have a doubt in the call chain:
`do_splice_direct -> do_splice_direct_actor:`
static ssize_t do_splice_direct_actor(struct file *in, loff_t *ppos,
				      struct file *out, loff_t *opos,
				      size_t len, unsigned int flags,
				      splice_direct_actor *actor)
{
	struct splice_desc sd = {
		.len		= len,  //unsigned int len
		.total_len	= len, 
		...
	};
	
The len member in the struct splice_desc is of type unsigned int. 
The assignment here may cause truncation, but in reality, this len
won't be used. Can we directly delete it? 
Otherwise, it's very confusing here.

Thanks
Chunsheng Luo

Re: [PATCH v2] copy_file_range: limit size if in compat mode

[Miklos Szeredi]

On Thu, 14 Aug 2025 at 10:28, Chunsheng Luo <luochunsheng@ustc.edu> wrote:
>
> On Wed, Aug 13, 2025 at 5:11 PM Miklos Szeredi wrote:
> > @@ -1624,8 +1629,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> >        * to splicing from input file, while file_start_write() is held on
> >        * the output file on a different sb.
> >        */
> > -     ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
> > -                            min_t(size_t, len, MAX_RW_COUNT), 0);
> > +     ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out, len, 0);
> >  done:
> >       if (ret > 0) {
> >               fsnotify_access(file_in);
>
> There is no problem with submission, but I have a doubt in the call chain:
> `do_splice_direct -> do_splice_direct_actor:`
> static ssize_t do_splice_direct_actor(struct file *in, loff_t *ppos,
>                                       struct file *out, loff_t *opos,
>                                       size_t len, unsigned int flags,
>                                       splice_direct_actor *actor)
> {
>         struct splice_desc sd = {
>                 .len            = len,  //unsigned int len
>                 .total_len      = len,
>                 ...
>         };
>
> The len member in the struct splice_desc is of type unsigned int.
> The assignment here may cause truncation, but in reality, this len
> won't be used. Can we directly delete it?

Yes, looks safe.  Goes back to commit introducing splice_desc
c66ab6fa705e ("splice: abstract out actor data").

Thanks,
Miklos

Re: [PATCH v2] copy_file_range: limit size if in compat mode

[Christian Brauner]

On Wed, 13 Aug 2025 17:11:05 +0200, Miklos Szeredi wrote:
> If the process runs in 32-bit compat mode, copy_file_range results can be
> in the in-band error range.  In this case limit copy length to MAX_RW_COUNT
> to prevent a signed overflow.
> 
> 

Applied to the vfs-6.18.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.18.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.18.misc

[1/1] copy_file_range: limit size if in compat mode
      https://git.kernel.org/vfs/vfs/c/f8f59a2c05dc