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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ