linux-kernel - Re: [PATCH 1/2] rust: add initial scatterlist bindings

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: <20250529004550.GB192517@ziepe.ca>
Date: Wed, 28 May 2025 21:45:50 -0300
From: Jason Gunthorpe <jgg@...pe.ca>
To: Abdiel Janulgue <abdiel.janulgue@...il.com>
Cc: acourbot@...dia.com, dakr@...nel.org, lyude@...hat.com,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Valentin Obst <kernel@...entinobst.de>,
	open list <linux-kernel@...r.kernel.org>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Robin Murphy <robin.murphy@....com>, airlied@...hat.com,
	rust-for-linux@...r.kernel.org,
	"open list:DMA MAPPING HELPERS" <iommu@...ts.linux.dev>,
	Petr Tesarik <petr@...arici.cz>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Sui Jingfeng <sui.jingfeng@...ux.dev>,
	Randy Dunlap <rdunlap@...radead.org>,
	Michael Kelley <mhklinux@...look.com>
Subject: Re: [PATCH 1/2] rust: add initial scatterlist bindings

On Thu, May 29, 2025 at 01:14:05AM +0300, Abdiel Janulgue wrote:
> +impl SGEntry<Unmapped> {
> +    /// Set this entry to point at a given page.
> +    pub fn set_page(&mut self, page: &Page, length: u32, offset: u32) {
> +        let c: *mut bindings::scatterlist = self.0.get();
> +        // SAFETY: according to the `SGEntry` invariant, the scatterlist pointer is valid.
> +        // `Page` invariant also ensures the pointer is valid.
> +        unsafe { bindings::sg_set_page(c, page.as_ptr(), length, offset) };
> +    }
> +}

Wrong safety statement. sg_set_page captures the page.as_ptr() inside
the C datastructure so the caller must ensure it holds a reference on
the page while it is contained within the scatterlist.

Which this API doesn't force to happen.

Most likely for this to work for rust you have to take a page
reference here and ensure the page reference is put back during sg
destruction. A typical normal pattern would 'move' the reference from
the caller into the scatterlist.

I also think set_page should not be exposed to rust at all, it should
probably only build scatterlists using the append APIs inside scatter
tables where the entire model is much cleaner.

Because this is also wrong in the sense that it destroys whatever
sg_page was already there, which may have been holding a page refcount
and thus it would leak it.

Jason