[PATCH] mm: Return early in truncate_pagecache if newsize overflows

[PATCH] mm: Return early in truncate_pagecache if newsize overflows

[Wupeng Ma]

From: Ma Wupeng <mawupeng1@huawei.com>

Our own test reports a UBSAN in truncate_pagecache:

UBSAN: Undefined behaviour in mm/truncate.c:788:9
signed integer overflow:
9223372036854775807 + 1 cannot be represented in type 'long long int'

Call Trace:
  truncate_pagecache+0xd4/0xe0
  truncate_setsize+0x70/0x88
  simple_setattr+0xdc/0x100
  notify_change+0x654/0xb00
  do_truncate+0x108/0x1a8
  do_sys_ftruncate+0x2ec/0x4a0
  __arm64_sys_ftruncate+0x5c/0x80

For huge file which pass LONG_MAX to ftruncate, truncate_pagecache() will
be called to truncate with newsize be LONG_MAX which will lead to
overflow for holebegin:

  loff_t holebegin = round_up(newsize, PAGE_SIZE);

Since there is no meaning to truncate a file to LONG_MAX, return here
to avoid burn a bunch of cpu cycles.

Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
---
 mm/truncate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/truncate.c b/mm/truncate.c
index 7b4ea4c4a46b..99b6ce2d669b 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -730,6 +730,9 @@ void truncate_pagecache(struct inode *inode, loff_t newsize)
 	struct address_space *mapping = inode->i_mapping;
 	loff_t holebegin = round_up(newsize, PAGE_SIZE);
 
+	if (holebegin < 0)
+		return;
+
 	/*
 	 * unmap_mapping_range is called twice, first simply for
 	 * efficiency so that truncate_inode_pages does fewer
-- 
2.25.1

Re: [PATCH] mm: Return early in truncate_pagecache if newsize overflows

[mawupeng]

Hi maintainers.

Kindly ping.

On 2023/3/6 19:33, Wupeng Ma wrote:
> From: Ma Wupeng <mawupeng1@huawei.com>
> 
> Our own test reports a UBSAN in truncate_pagecache:
> 
> UBSAN: Undefined behaviour in mm/truncate.c:788:9
> signed integer overflow:
> 9223372036854775807 + 1 cannot be represented in type 'long long int'
> 
> Call Trace:
>   truncate_pagecache+0xd4/0xe0
>   truncate_setsize+0x70/0x88
>   simple_setattr+0xdc/0x100
>   notify_change+0x654/0xb00
>   do_truncate+0x108/0x1a8
>   do_sys_ftruncate+0x2ec/0x4a0
>   __arm64_sys_ftruncate+0x5c/0x80
> 
> For huge file which pass LONG_MAX to ftruncate, truncate_pagecache() will
> be called to truncate with newsize be LONG_MAX which will lead to
> overflow for holebegin:
> 
>   loff_t holebegin = round_up(newsize, PAGE_SIZE);
> 
> Since there is no meaning to truncate a file to LONG_MAX, return here
> to avoid burn a bunch of cpu cycles.
> 
> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
> ---
>  mm/truncate.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/truncate.c b/mm/truncate.c
> index 7b4ea4c4a46b..99b6ce2d669b 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -730,6 +730,9 @@ void truncate_pagecache(struct inode *inode, loff_t newsize)
>  	struct address_space *mapping = inode->i_mapping;
>  	loff_t holebegin = round_up(newsize, PAGE_SIZE);
>  
> +	if (holebegin < 0)
> +		return;
> +
>  	/*
>  	 * unmap_mapping_range is called twice, first simply for
>  	 * efficiency so that truncate_inode_pages does fewer

Re: [PATCH] mm: Return early in truncate_pagecache if newsize overflows

[Andrew Morton]

On Mon, 6 Mar 2023 19:33:17 +0800 Wupeng Ma <mawupeng1@huawei.com> wrote:

> From: Ma Wupeng <mawupeng1@huawei.com>
> 
> Our own test reports a UBSAN in truncate_pagecache:
> 
> UBSAN: Undefined behaviour in mm/truncate.c:788:9
> signed integer overflow:
> 9223372036854775807 + 1 cannot be represented in type 'long long int'
> 
> Call Trace:
>   truncate_pagecache+0xd4/0xe0
>   truncate_setsize+0x70/0x88
>   simple_setattr+0xdc/0x100
>   notify_change+0x654/0xb00
>   do_truncate+0x108/0x1a8
>   do_sys_ftruncate+0x2ec/0x4a0
>   __arm64_sys_ftruncate+0x5c/0x80
> 
> For huge file which pass LONG_MAX to ftruncate, truncate_pagecache() will
> be called to truncate with newsize be LONG_MAX which will lead to
> overflow for holebegin:
> 
>   loff_t holebegin = round_up(newsize, PAGE_SIZE);
> 
> Since there is no meaning to truncate a file to LONG_MAX, return here
> to avoid burn a bunch of cpu cycles.
> 
> ...
>
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -730,6 +730,9 @@ void truncate_pagecache(struct inode *inode, loff_t newsize)
>  	struct address_space *mapping = inode->i_mapping;
>  	loff_t holebegin = round_up(newsize, PAGE_SIZE);
>  
> +	if (holebegin < 0)
> +		return;
> +

It's awkward to perform an operation which might experience overflow
and to then test the possibly-overflowed result!  In fact it might
still generate the UBSAN warning, depending on what the compiler
decides to do with it all.

So wouldn't it be better to check the input argument *before*
performing these operations on it?  Preferably with a code comment
which explains the reason for the check, please.