linux-hardening - Re: [PATCH v3] fs: fix unintentional arithmetic wraparound in offset calculation

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: <20240520115851.gcwj3nwicvr2c4j3@quack3>
Date: Mon, 20 May 2024 13:58:51 +0200
From: Jan Kara <jack@...e.cz>
To: Justin Stitt <justinstitt@...gle.com>
Cc: Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	Nathan Chancellor <nathan@...nel.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Bill Wendling <morbo@...gle.com>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH v3] fs: fix unintentional arithmetic wraparound in offset
 calculation

On Fri 17-05-24 00:29:06, Justin Stitt wrote:
> When running syzkaller with the newly reintroduced signed integer
> overflow sanitizer we encounter this report:
> 
> UBSAN: signed-integer-overflow in ../fs/read_write.c:91:10
> 9223372036854775807 + 4096 cannot be represented in type 'loff_t' (aka 'long long')
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x93/0xd0
>  handle_overflow+0x171/0x1b0
>  generic_file_llseek_size+0x35b/0x380
> 
> ... amongst others:
> UBSAN: signed-integer-overflow in ../fs/read_write.c:1657:12
> 142606336 - -9223372036854775807 cannot be represented in type 'loff_t' (aka 'long long')
> ...
> UBSAN: signed-integer-overflow in ../fs/read_write.c:1666:11
> 9223372036854775807 - -9223231299366420479 cannot be represented in type 'loff_t' (aka 'long long')
> 
> Fix the accidental overflow in these position and offset calculations
> by checking for negative position values, using check_add_overflow()
> helpers and clamping values to expected ranges.
> 
> Link: https://github.com/llvm/llvm-project/pull/82432 [1]
> Closes: https://github.com/KSPP/linux/issues/358
> Cc: linux-hardening@...r.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@...gle.com>

Except for the unfortunate wording in the changelog, the code actually
looks easier to grasp to me and if it helps the compiler as well, I'm in
favor of this change (but I definitely don't want to overrule Al if he
hates it ;)).

Regarding the code:

> @@ -1467,8 +1470,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
>  
>  	/* Don't allow overlapped copying within the same file. */
>  	if (inode_in == inode_out &&
> -	    pos_out + count > pos_in &&
> -	    pos_out < pos_in + count)
> +	    out_sum > pos_in &&
> +	    pos_out < in_sum)
>  		return -EINVAL;

This is actually subtly wrong becaue 'count' could have been updated
(shrinked) between the check_add_overflow() and this place. So please keep
the old checks here.

> @@ -1649,6 +1652,9 @@ int generic_write_check_limits(struct file *file, loff_t pos, loff_t *count)
>  	loff_t max_size = inode->i_sb->s_maxbytes;
>  	loff_t limit = rlimit(RLIMIT_FSIZE);
>  
> +	if (pos < 0)
> +		return -EINVAL;
> +
>  	if (limit != RLIM_INFINITY) {
>  		if (pos >= limit) {
>  			send_sig(SIGXFSZ, current, 0);

Here I'm a bit confused. How is this related to the signed overflow
handling?

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR