Re: [PATCH v4 1/8] efi: efivars: don't crash in efivar_set_variable{,_locked} in r/o case

[Johan Hovold <johan@kernel.org>]

On Thu, Jun 26, 2025 at 03:54:11PM +0300, Dmitry Baryshkov wrote:
> On Thu, 26 Jun 2025 at 15:51, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Jun 26, 2025 at 02:03:44PM +0300, Dmitry Baryshkov wrote:
> > > On Thu, Jun 26, 2025 at 12:04:30PM +0200, Johan Hovold wrote:
> > > > On Wed, Jun 25, 2025 at 01:53:20AM +0300, Dmitry Baryshkov wrote:
> > > > > If efivar implementation doesn't provide write support, then calling
> > > > > efivar_set_variable() (e.g. when PM8xxx RTC driver tries to update the
> > > > > RTC offset) will crash the system. Prevent that by checking that
> > > > > set_variable callback is actually provided and fail with an
> > > > > EFI_WRITE_PROTECTED if it is not.
> > > > >
> > > > > Fixes: 472831d4c4b2 ("efi: vars: Add thin wrapper around EFI get/set variable interface")
> > > >
> > > > I don't think a fixes tag is warranted here as it currently appears to
> > > > be expected that the callers check if setvar is supported before calling
> > > > this helper (e.g. by calling efivar_supports_writes() as efivarfs does).
> > >
> > > It is not documented as such. So, I think, we'd better not crash the
> > > callers.
> >
> > You need to look at the backstory to determine that before jumping to
> > conclusions (e.g. start by looking at f88814cc2578 ("efi/efivars: Expose
> > RT service availability via efivars abstraction")).
> 
> _documented_. I'll update documentation for efivar_set_variable() in
> the next iteration and add a check to the RTC driver. However I still
> think that this patch is valid.

Still depends on *how* we want to address this.

> > > > So should perhaps be fixed in the RTC driver if we agree that supporting
> > > > read-only offsets is indeed something we want.
> > > >
> > > > Are there any other current user that may possibly benefit from
> > > > something like this?
> > >
> > > efi-pstore comes to my mind.
> >
> > No, that driver is also disabled when efivar_supports_writes() returns
> > false.
> 
> Good.

Ok, so then there are no current drivers that will benefit from your
change, but you may (or may not) need it if you enable RO efivars on
this particular platform. That is, this patch is not actually fixing
anything that is broken currently.

Johan