Message-Id: <D9WRBS02A1RW.2LEBA1AI0IVSS@nvidia.com>
Date: Thu, 15 May 2025 22:09:47 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Greg KH" <gregkh@...uxfoundation.org>
Cc: "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>, "Danilo
 Krummrich" <dakr@...nel.org>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH] rust: add basic ELF sections parser

On Thu May 15, 2025 at 8:42 PM JST, Greg KH wrote:
> On Thu, May 15, 2025 at 08:25:33PM +0900, Alexandre Courbot wrote:
>> On Thu May 15, 2025 at 5:32 PM JST, Alexandre Courbot wrote:
>> > Hi Greg,
>> >
>> > On Thu May 15, 2025 at 4:38 PM JST, Greg KH wrote:
>> >> On Thu, May 15, 2025 at 03:03:51PM +0900, Alexandre Courbot wrote:
>> >>> Add a simple ELF sections parser for unpacking loaded binaries from
>> >>> user-space. This is not intended to become a fully-fledged ELF parser,
>> >>> just a helper to parse firmwares packaged in that format.
>> >>> 
>> >>> This parser is notably helpful for NVIDIA's GSP firmware, which is
>> >>> provided as an ELF binary using sections to separate the firmware code
>> >>> to its other components like chipset-specific signatures.
>> >>> 
>> >>> Since the data source is likely to be user-space, checked arithmetic
>> >>> operations and strict bound checking are used.
>> >>> 
>> >>> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
>> >>> ---
>> >>> This will soon be needed in order to load the GSP firmware in nova-core,
>> >>> so sending this early for separate review.
>> >>> ---
>> >>>  rust/kernel/elf.rs | 322 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >>
>> >> Why is this not just done in userspace and then have userspace feed the
>> >> proper elf sections to the kernel through the firmware interface?
>> >> Having to parse elf seems crazy for the kernel to be forced to do here
>> >> as the kernel should NOT be touching anything in a firmware blob other
>> >> than passing it off to the firmware directly.
>> >
>> > FWIW, the GSP firmware in question is already in linux-firmware and
>> > loaded by e.g. Nouveau.
>> >
>> > I am not sure how userspace could feed the proper ELF sections otherwise
>> > than by splitting the ELF binary into as many files as there are
>> > sections. Is that what you imply, or is there another means that would
>> > preserve the current firmware format?
>> >
>> > Note also that in this particular case, the kernel cannot just pass the
>> > firmware without modifying it anyway since the signatures relevant to
>> > the chipset need to be patched into the right place before it is loaded.
>> 
>> Quick nit, as that last statement was not entirely correct: while we do
>> patch some loaded firmware with signatures, this is not the case for the
>> GSP (the one in ELF format). Not that it changes the point, but for the
>> sake of accuracy. :)
>> 
>> The point being that even without using ELF as a container format, we do
>> need to parse header structures in loaded firmware files anyway, so the
>> kernel cannot simply act as a dumb pipe for firmware. And since we need
>> to add structure, let's at least use a format that is simple, well
>> accepted and which layout is already in the kernel.
>> 
>> Or if ELF is the problem, I don't mind introducing a WAD loader. ;)
>
> The "problem" I'm not understanding is why does the kernel have to do
> any of this parsing at all?  What does it do with these segments that
> userspace can't do instead?  Why does patching have to be done within
> the kernel at all?  What prevents all of this from being done elsewhere?

I don't understand how userspace could do that unless we either 1) split
the firmwares into a multitude of files, or 2) rely on a daemon to do the
trivial processing that the kernel is currently doing.

A firmware like Booter (which loads the GSP) comes with a set of
signatures, one of which needs to be patched into the firmware data
segment. Which one to patch is inferred from a fuse register that is
read at probe time. So if we do 2), this means the kernel needs to
somehow communicate which signature to patch to the user-space daemon,
before it can get the patched firmware (which will still have some kind
of header with information like the entry point and values to write into
the boot ROM registers). This adds more dependencies just to bring up
the GPU and the processing that is done is so trivial that I don't see
the benefit of moving it to user-space, even though I do agree with the
general principle.

> And ELF parsing is "tricky" in places, and you aren't using the existing
> elf parser, as proof of needing a new parser in rust :)

We could also write an abstraction above one of the parsers that exist
on the C side, but I am not sure whether that would improve the safety
argument. :) This code is simple and easy to check (and pinky-swear
won't turn into something complex). Rust also helps with the safety
aspect - to some degree only, of course.

I have checked a few other drivers and many do at least some header
parsing from the firmware. I don't think what we are doing here is
particularly complex.

Powered by blists - more mailing lists