[1/3] xfs_io: don't pass negative len to copy_file_range_cmd
diff mbox series

Message ID b48c8b5d-9ed3-c4b6-c7fc-2c13df211dd3@redhat.com
State Superseded
Headers show
Series
  • xfsprogs: minor 4.20 fixups
Related show

Commit Message

Eric Sandeen Feb. 19, 2019, 11:17 p.m. UTC
If copy_src_filesize returns an error (-1) we should return that
error, and not pass it to copy_file_range_cmd().

Addresses-Coverity-ID: 1431684 ("Improper use of negative value")
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Comments

Darrick J. Wong Feb. 19, 2019, 11:24 p.m. UTC | #1
On Tue, Feb 19, 2019 at 05:17:49PM -0600, Eric Sandeen wrote:
> If copy_src_filesize returns an error (-1) we should return that
> error, and not pass it to copy_file_range_cmd().
> 
> Addresses-Coverity-ID: 1431684 ("Improper use of negative value")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> index 4e2969c..bc891c9 100644
> --- a/io/copy_file_range.c
> +++ b/io/copy_file_range.c
> @@ -121,6 +121,10 @@ copy_range_f(int argc, char **argv)
>  
>  	if (src == 0 && dst == 0 && len == 0) {
>  		len = copy_src_filesize(fd);

len is size_t but this function returns off64_t, so if src happened to
be larger than 4GB on a 32-bit system we'll just rip the upper bits off
the number and use that as the file size.

Granted I don't know that we care about 32-bit systems.

> +		if (len < 0) {

But that doesn't fix the problem, because len is size_t, which is
unsigned, so this test is never true.

> +			close(fd);
> +			return 0;
> +		}
>  		copy_dst_truncate();

Ugh, nobody checked the return value of copy_dst_truncate, so if we
can't truncate the destination file we just ignore that and keep
going...

>  	}

...totally untested patch fixing all that nonsense below.

--D

From: Darrick J. Wong <darrick.wong@oracle.com>
Subject: [PATCH] xfs_io: actually check copy file range helper return values

We need to check the return value of copy_src_filesize and
copy_dst_truncate because either could return -1 due to fstat/ftruncate
failure.

Fixes: 628e112afdd98c5 ("xfs_io: implement 'copy_range' command")
Cc: schumaker.anna@gmail.com
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 io/copy_file_range.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/io/copy_file_range.c b/io/copy_file_range.c
index 4e2969c9..ed6fafb1 100644
--- a/io/copy_file_range.c
+++ b/io/copy_file_range.c
@@ -120,11 +120,26 @@ copy_range_f(int argc, char **argv)
 		return 0;
 
 	if (src == 0 && dst == 0 && len == 0) {
-		len = copy_src_filesize(fd);
-		copy_dst_truncate();
+		off64_t	sz;
+
+		sz = copy_src_filesize(fd);
+		if (sz < 0) {
+			ret = sz;
+			goto out;
+		}
+		if ((unsigned long)sz > SIZE_MAX) {
+			ret = -EOVERFLOW;
+			goto out;
+		}
+		len = sz;
+
+		ret = copy_dst_truncate();
+		if (ret < 0)
+			goto out;
 	}
 
 	ret = copy_file_range_cmd(fd, &src, &dst, len);
+out:
 	close(fd);
 	return ret;
 }
Eric Sandeen Feb. 19, 2019, 11:50 p.m. UTC | #2
On 2/19/19 5:24 PM, Darrick J. Wong wrote:
> On Tue, Feb 19, 2019 at 05:17:49PM -0600, Eric Sandeen wrote:
>> If copy_src_filesize returns an error (-1) we should return that
>> error, and not pass it to copy_file_range_cmd().
>>
>> Addresses-Coverity-ID: 1431684 ("Improper use of negative value")
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
>> index 4e2969c..bc891c9 100644
>> --- a/io/copy_file_range.c
>> +++ b/io/copy_file_range.c
>> @@ -121,6 +121,10 @@ copy_range_f(int argc, char **argv)
>>  
>>  	if (src == 0 && dst == 0 && len == 0) {
>>  		len = copy_src_filesize(fd);
> 
> len is size_t but this function returns off64_t, so if src happened to
> be larger than 4GB on a 32-bit system we'll just rip the upper bits off
> the number and use that as the file size.
> 
> Granted I don't know that we care about 32-bit systems.
> 
>> +		if (len < 0) {
> 
> But that doesn't fix the problem, because len is size_t, which is
> unsigned, so this test is never true.
> 
>> +			close(fd);
>> +			return 0;
>> +		}
>>  		copy_dst_truncate();
> 
> Ugh, nobody checked the return value of copy_dst_truncate, so if we
> can't truncate the destination file we just ignore that and keep
> going...
> 
>>  	}
> 
> ...totally untested patch fixing all that nonsense below.
> 
> --D
> 
> From: Darrick J. Wong <darrick.wong@oracle.com>
> Subject: [PATCH] xfs_io: actually check copy file range helper return values
> 
> We need to check the return value of copy_src_filesize and
> copy_dst_truncate because either could return -1 due to fstat/ftruncate
> failure.
> 
> Fixes: 628e112afdd98c5 ("xfs_io: implement 'copy_range' command")
> Cc: schumaker.anna@gmail.com
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  io/copy_file_range.c |   19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> index 4e2969c9..ed6fafb1 100644
> --- a/io/copy_file_range.c
> +++ b/io/copy_file_range.c
> @@ -120,11 +120,26 @@ copy_range_f(int argc, char **argv)
>  		return 0;
>  
>  	if (src == 0 && dst == 0 && len == 0) {
> -		len = copy_src_filesize(fd);
> -		copy_dst_truncate();
> +		off64_t	sz;
> +
> +		sz = copy_src_filesize(fd);
> +		if (sz < 0) {
> +			ret = sz;

urgh this is the "what do we return in xfs_io functions?" dilemma,
but every other failure in this function returns 0 to the caller...


> +			goto out;
> +		}
> +		if ((unsigned long)sz > SIZE_MAX) {

as mentioned on IRC this still has the same truncation
problem on a 32-bit box.

-Eric

> +			ret = -EOVERFLOW;
> +			goto out;
> +		}
> +		len = sz;
> +
> +		ret = copy_dst_truncate();
> +		if (ret < 0)

ditto here, ret -1 or 0?

> +			goto out;
>  	}
>  
>  	ret = copy_file_range_cmd(fd, &src, &dst, len);
> +out:
>  	close(fd);
>  	return ret;
>  }
>

Patch
diff mbox series

diff --git a/io/copy_file_range.c b/io/copy_file_range.c
index 4e2969c..bc891c9 100644
--- a/io/copy_file_range.c
+++ b/io/copy_file_range.c
@@ -121,6 +121,10 @@  copy_range_f(int argc, char **argv)
 
 	if (src == 0 && dst == 0 && len == 0) {
 		len = copy_src_filesize(fd);
+		if (len < 0) {
+			close(fd);
+			return 0;
+		}
 		copy_dst_truncate();
 	}