linux-kernel - Re: [PATCH] poll: rust: allow 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] [day] [month] [year] [list]

Message-Id: <DASWFQXR9V54.18EU85NWBUC97@kernel.org>
Date: Sun, 22 Jun 2025 09:55:22 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Alice Ryhl" <aliceryhl@...gle.com>, "Alexander Viro"
 <viro@...iv.linux.org.uk>, "Jan Kara" <jack@...e.cz>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Christian Brauner" <brauner@...nel.org>
Cc: "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH] poll: rust: allow poll_table ptrs to be null

On Fri Jun 20, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
> It's possible for a poll_table to be null. This can happen if an
> end-user just wants to know if a resource has events right now without
> registering a waiter for when events become available. Furthermore,
> these null pointers should be handled transparently by the API, so we
> should not change `from_ptr` to return an `Option`. Thus, change
> `PollTable` to wrap a raw pointer rather than use a reference so that
> you can pass null.
>
> Comments mentioning `struct poll_table` are changed to just `poll_table`
> since `poll_table` is a typedef. (It's a typedef because it's supposed
> to be opaque.)
>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
> This issue was discovered from a syzkaller report on Rust Binder.
>
> Intended for Christian Brauner's tree.
> ---
>  rust/helpers/helpers.c   |  1 +
>  rust/helpers/poll.c      | 10 ++++++++
>  rust/kernel/sync/poll.rs | 65 +++++++++++++++++-------------------------------
>  3 files changed, 34 insertions(+), 42 deletions(-)

Looks good, one safety comment concern below, with that fixed:

Reviewed-by: Benno Lossin <lossin@...nel.org>

>      /// Register this [`PollTable`] with the provided [`PollCondVar`], so that it can be notified
>      /// using the condition variable.
> -    pub fn register_wait(&mut self, file: &File, cv: &PollCondVar) {
> -        if let Some(qproc) = self.get_qproc() {
> -            // SAFETY: The pointers to `file` and `self` need to be valid for the duration of this
> -            // call to `qproc`, which they are because they are references.
> -            //
> -            // The `cv.wait_queue_head` pointer must be valid until an rcu grace period after the
> -            // waiter is removed. The `PollCondVar` is pinned, so before `cv.wait_queue_head` can
> -            // be destroyed, the destructor must run. That destructor first removes all waiters,
> -            // and then waits for an rcu grace period. Therefore, `cv.wait_queue_head` is valid for
> -            // long enough.
> -            unsafe { qproc(file.as_ptr() as _, cv.wait_queue_head.get(), self.0.get()) };
> -        }
> +    pub fn register_wait(&self, file: &File, cv: &PollCondVar) {
> +        // SAFETY: The pointers `self.table` and `file` are valid for the duration of this call.

`self.table` might be null, which I think we agreed to is not "valid".

> +        // The `cv.wait_queue_head` pointer must be valid until an rcu grace period after the
> +        // waiter is removed. The `PollCondVar` is pinned, so before `cv.wait_queue_head` can be
> +        // destroyed, the destructor must run. That destructor first removes all waiters, and then
> +        // waits for an rcu grace period. Therefore, `cv.wait_queue_head` is valid for long enough.

Could you use bullet points for the different requirements?

---
Cheers,
Benno

> +        unsafe { bindings::poll_wait(file.as_ptr(), cv.wait_queue_head.get(), self.table) }
>      }
>  }
>  
>
> ---
> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> change-id: 20250620-poll-table-null-bf9a6a6c569e
>
> Best regards,