linux-kernel - Re: [PATCH v2 6/6] soc: qcom: socinfo: Convert to Rust

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: <CAGSQo01qf4i6LDyi1umH6-6vjiE-VrFc1dd+qLxDXkjiYLcOCA@mail.gmail.com>
Date: Tue, 3 Feb 2026 09:26:23 -0800
From: Matthew Maurer <mmaurer@...gle.com>
To: Gary Guo <gary@...yguo.net>
Cc: Danilo Krummrich <dakr@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, 
	Satya Durga Srinivasu Prabhala <satyap@...cinc.com>, Miguel Ojeda <ojeda@...nel.org>, Boqun Feng <boqun.feng@...il.com>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Daniel Almeida <daniel.almeida@...labora.com>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	Michal Wilczynski <m.wilczynski@...sung.com>, Dave Ertman <david.m.ertman@...el.com>, 
	Ira Weiny <ira.weiny@...el.com>, Leon Romanovsky <leon@...nel.org>, Trilok Soni <tsoni@...cinc.com>, 
	linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, driver-core@...ts.linux.dev, 
	dri-devel@...ts.freedesktop.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v2 6/6] soc: qcom: socinfo: Convert to Rust

On Tue, Feb 3, 2026 at 9:17 AM Gary Guo <gary@...yguo.net> wrote:
>
> On Tue Feb 3, 2026 at 4:56 PM GMT, Danilo Krummrich wrote:
> > On Tue Feb 3, 2026 at 5:48 PM CET, Greg Kroah-Hartman wrote:
> >> On Tue, Feb 03, 2026 at 05:35:24PM +0100, Danilo Krummrich wrote:
> >>> On Tue Feb 3, 2026 at 5:28 PM CET, Greg Kroah-Hartman wrote:
> >>> > On Tue, Feb 03, 2026 at 03:46:35PM +0000, Matthew Maurer wrote:
> >>> >> +impl Smem {
> >>> >> +    pub(crate) fn access<'a>(&'a self, dev: &'a Device<Bound>) -> Option<&'a Mmio> {
> >>> >> +        if *dev != *self.dev {
> >>> >
> >>> > How can this ever happen?
> >>>
> >>> You are right, since this resource is local to the driver, it should not be
> >>> possible to have another devce from somewhere else (especially not a
> >>> &Device<Bound>).
> >>>
> >>> However, from a Rust perspective I think not having this check would be unsound,
> >>> as the method by itself would not be able guarantee correct behavor anymore.
> >>>
> >>
> >> I don't really understand this last sentence, sorry.  If this is "not
> >> possible" why would that at the same time be "unsound"?

The code as it stands doesn't do anything that would grant `Smem`
objects to something that exceeds the lifetime of the device. The goal
of bindings APIs like these are that it should be impossible for
someone to write Rust code without the word `unsafe` in it that breaks
its invariants. Without that check, someone could do:

let smem = Smem::get(dev)?;
let f = move |offset| smem.try_copy(offset);
some_other_driver_or_subsystem(f);

That other region of the code would now have ownership over that
function, and expect to be able to call it safely. We're not doing
that, but we're trying to design the unsafe API in bindings so that
you can't mess it up even if you wanted to.

> >
> > It would be considered unsound because the function itself can not guarantee
> > that it does not produce undefined behavior, i.e. it would otherwise becomes
> > unsafe.
> >
> > But I'm not an expert on this, I'll let Benno and Gary jump in.
>
> Yes, in this code, `Smem::access` is using the passed-in `&'a Device<Bound>` as
> a proof that the device is still bound.
>
> If the check is not present, the user of this API can give in a reference of a
> different device, and now you're pontentially providing access to the MMIO
> region to a device that is not bound anymore. A important property we want to
> enforce is that UB cannot happen no matter how an API is misused (it may still
> oops, which is bad, but better than UB).
>
> This particular code, though, probably should use `Devres` as Danilo suggested.

Mentioned on the other thread, but I didn't use `Devres` because it
being PinInit is problematic and I would need to encode an additional
invariant anyways.

>
> PS. Personally I think this check (and the one in Devres::access) should use
> assertion rather than `EINVAL`, because it is a bad driver bug when this is hit,
> and adding failing code path for something that is a bug is something I really
> dislike... It should at least be a `WARN`.
>
> Best,
> Gary