linux-kernel - Re: [PATCH net-next v2 5/6] rust: Add read_poll

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]

Message-ID: <c3955011-e131-45c9-bf74-da944e336842@lunn.ch>
Date: Tue, 8 Oct 2024 14:12:51 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Alice Ryhl <aliceryhl@...gle.com>,
	FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org,
	rust-for-linux@...r.kernel.org, hkallweit1@...il.com,
	tmgross@...ch.edu, ojeda@...nel.org, alex.gaynor@...il.com,
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	a.hindborg@...sung.com, anna-maria@...utronix.de,
	frederic@...nel.org, tglx@...utronix.de, arnd@...db.de,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

> Because of the might_resched() in might_sleep(), it will report the
> quiescent state of the current CPU, and RCU will pass a grace period if
> all CPUs have passed a quiescent state. So for example if someone writes
> the following:
> 
>     <reader>			<updater>
>     rcu_read_lock();
>     p = rcu_dereference(gp);
>     might_sleep():
>       might_resched():
> 				todo = gp;
> 				rcu_assign_pointer(gp, NULL);
> 				synchronize_rcu();
> 
>         rcu_all_qs(); // report a quiescent state inside RCU read-side
> 	              // critical section, which may make a grace period
> 		      // pass even there is an active RCU reader
> 
> 				kfree(todo);
> 

You are obviously missing something here. The call that actually sleeps

      mutex_lock(&lock)

>     a = READ_ONCE(p->a); // UAF
>     rcu_read_unlock();

A might_sleep() should be paired with something which does actually
sleep, under some condition.  At least, that is how it is used in C.
The iopoll being re-implemented here is an example of that.

So take the might_sleep out above, just leaving the mutex_lock. If the
mutex is uncontested, the code does not sleep and everything is O.K?
If it needs to wait for the mutex, it triggers a UAF.

The might_sleep() will also trigger a stack trace, if its is enabled,
because you are not allowed to sleep inside rcu_read_lock(), it is an
example of atomic context.

As far as i see, might_sleep() will cause UAF where there is going to
be a UAF anyway. If you are using it correctly, it does not cause UAF.

> We probably call the reader side code a "wrong annotation", however,
> it's still unsafe code because of the UAF. Also you seems to assume that
> might_sleep() is always attached to a sleepable function, which is not
> an invalid assumption, but we couldn't use it for reasoning the
> safe/unsafe property of Rust functions unless we can encode this in the
> type system.

How are any of the sleeping call encoded in the type system? I assume
any use of a mutex lock, sleep, wait for completion, etc are not all
marked as unsafe? There is some sort of wrapper around them? Why not
just extend that wrapper to might_sleep().

> For Rust code, without klint rule, might_sleep() needs to
> be unsafe. So we have two options for might_sleep().
> 
> * Since we rely on klint for atomic context detection, we can mark the
>   trivial wrapper (as what Alice presented in the other email) as safe,
>   but we need to begin to add klint annotation for that function, unless
>   Gary finds a smart way to auto-annotate functions.

Are there klint annotations for all sleeping functions?

> * Instead of might_sleep(), we provide the wrapper of __might_sleep(),
>   since it doesn't have might_resched() in it, it should be safe. And
>   all we care about here is the debugging rather than voluntary context
>   switch. (Besides I think preempt=volunatry is eventually going to be
>   gone because of PREEMPT_AUTO [1], if that happens I think the
>   might_resched() might be dropped entirely).

__might_sleep() might be safe, but your code is still broken and going
to UAF at some point. Don't you want that UAF to happen more reliably
and faster so you can find the issue? That would be the advantage of
might_sleep() over __might_sleep().

	Andrew