linux-kernel - Re: [PATCH] mm: Return early in truncate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20230323144053.68add73fe29ee56fa5c628c6@linux-foundation.org>
Date:   Thu, 23 Mar 2023 14:40:53 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Wupeng Ma <mawupeng1@...wei.com>
Cc:     <willy@...radead.org>, <linux-fsdevel@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>
Subject: Re: [PATCH] mm: Return early in truncate_pagecache if newsize
 overflows

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

> From: Ma Wupeng <mawupeng1@...wei.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.