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] [thread-next>] [day] [month] [year] [list]
Date: Thu, 25 Jan 2024 00:46:26 +0100
From: Valentin Obst <kernel@...entinobst.de>
To: aliceryhl@...gle.com
Cc: a.hindborg@...sung.com,
	akpm@...ux-foundation.org,
	alex.gaynor@...il.com,
	arnd@...db.de,
	arve@...roid.com,
	benno.lossin@...ton.me,
	bjorn3_gh@...tonmail.com,
	boqun.feng@...il.com,
	brauner@...nel.org,
	cmllamas@...gle.com,
	gary@...yguo.net,
	gregkh@...uxfoundation.org,
	joel@...lfernandes.org,
	keescook@...omium.org,
	linux-kernel@...r.kernel.org,
	linux-mm@...ck.org,
	maco@...roid.com,
	ojeda@...nel.org,
	rust-for-linux@...r.kernel.org,
	surenb@...gle.com,
	tkjos@...roid.com,
	viro@...iv.linux.org.uk,
	wedsonaf@...il.com,
	Valentin Obst <kernel@...entinobst.de>
Subject: Re: [PATCH 2/3] rust: add typed accessors for userspace pointers

> +/*

nit: this would be the first comment in the kernel crate to use this
style, not sure if there is a rule about that though. Maybe still
preferable to keep it consistent.

> + * These methods skip the `check_object_size` check that `copy_[to|from]_user`
> + * normally performs.

nit: They skip the (stronger, and also present without usercopy
hardening) `check_copy_size` wrapping that one.

>                        In C, these checks are skipped whenever the length is a
> + * compile-time constant, since when that is the case, the kernel pointer
> + * usually points at a local variable that is being initialized

Question: I thought that this exemption is about dynamic size
calculations being more susceptible to bugs than hard-coded ones. Does
someone recall the original rationale for that?

>                                                                  and the kernel
> + * pointer is trivially non-dangling.

As far as I know the hardened usercopy checks are not meant to catch
UAFs but rather about OOB accesses (and some info leaks). For example,
if the object is on the heap they check if the copy size exceeds the
allocation size, or, if the object is on the stack, they verify the copy
size does not leave the stack frame.

> + *
> + * These helpers serve the same purpose in Rust. Whenever the length is known at
> + * compile-time, we call this helper to skip the check.
> + */
> +unsigned long rust_helper_copy_from_user_unsafe_skip_check_object_size(void *to, const void __user *from, unsigned long n)
> +{
> +	unsigned long res;
> +
> +	might_fault();
> +	instrument_copy_from_user_before(to, from, n);
> +	if (should_fail_usercopy())
> +		return n;
> +	res = raw_copy_from_user(to, from, n);
> +	instrument_copy_from_user_after(to, from, n, res);
> +	return res;
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_copy_from_user_unsafe_skip_check_object_size);
> +
> +unsigned long rust_helper_copy_to_user_unsafe_skip_check_object_size(void __user *to, const void *from, unsigned long n)
> +{
> +	might_fault();
> +	if (should_fail_usercopy())
> +		return n;
> +	instrument_copy_to_user(to, from, n);
> +	return raw_copy_to_user(to, from, n);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_copy_to_user_unsafe_skip_check_object_size);

Could those be wrapping `_copy_[to|from]_user` instead?

	- Valentin

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ