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")
Reported-by: Johan Hovold <johan@kernel.org>
Closes: https://lore.kernel.org/r/aFlps9iUcD42vN4w@hovoldconsulting.com
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/firmware/efi/vars.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 3700e98697676d8e6f04f061f447391503f9abba..11c5f785c09364f61642d82416822cb2e1a027fd 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -227,6 +227,8 @@ efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
 	setvar = __efivars->ops->set_variable_nonblocking;
 	if (!setvar || !nonblocking)
 		 setvar = __efivars->ops->set_variable;
+	if (!setvar)
+		return EFI_WRITE_PROTECTED;
 
 	return setvar(name, vendor, attr, data_size, data);
 }

-- 
2.39.5

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).

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?

> Reported-by: Johan Hovold <johan@kernel.org>
> Closes: https://lore.kernel.org/r/aFlps9iUcD42vN4w@hovoldconsulting.com
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>  drivers/firmware/efi/vars.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 3700e98697676d8e6f04f061f447391503f9abba..11c5f785c09364f61642d82416822cb2e1a027fd 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -227,6 +227,8 @@ efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
>  	setvar = __efivars->ops->set_variable_nonblocking;
>  	if (!setvar || !nonblocking)
>  		 setvar = __efivars->ops->set_variable;
> +	if (!setvar)
> +		return EFI_WRITE_PROTECTED;
>  
>  	return setvar(name, vendor, attr, data_size, data);
>  }

Johan

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.

> 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.

> 
> > Reported-by: Johan Hovold <johan@kernel.org>
> > Closes: https://lore.kernel.org/r/aFlps9iUcD42vN4w@hovoldconsulting.com
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > ---
> >  drivers/firmware/efi/vars.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > index 3700e98697676d8e6f04f061f447391503f9abba..11c5f785c09364f61642d82416822cb2e1a027fd 100644
> > --- a/drivers/firmware/efi/vars.c
> > +++ b/drivers/firmware/efi/vars.c
> > @@ -227,6 +227,8 @@ efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
> >  	setvar = __efivars->ops->set_variable_nonblocking;
> >  	if (!setvar || !nonblocking)
> >  		 setvar = __efivars->ops->set_variable;
> > +	if (!setvar)
> > +		return EFI_WRITE_PROTECTED;
> >  
> >  	return setvar(name, vendor, attr, data_size, data);
> >  }
> 
> Johan

-- 
With best wishes
Dmitry

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")).

> > 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.

Johan

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.

>
> > > 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.


-- 
With best wishes
Dmitry

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

On Fri, Jun 27, 2025 at 02:27:46PM +0200, Johan Hovold wrote:
> 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.

I'd prefer to address it in both places.

> > > > > 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.

I'd leave that to a discretion of EFI / EFI vars maintainers. RTC driver
definitely is broken in its current state.

-- 
With best wishes
Dmitry

On Sat, Jun 28, 2025 at 06:05:51PM +0300, Dmitry Baryshkov wrote:
> On Fri, Jun 27, 2025 at 02:27:46PM +0200, Johan Hovold wrote:

> > 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.
> 
> I'd leave that to a discretion of EFI / EFI vars maintainers. RTC driver
> definitely is broken in its current state.

Again, no. We only need this when you start enabling RO efivars on
Qualcomm platforms. So you're not fixing anything that is currently
broken.

Johan