Re: efivarfs: fix error on write to new variable leaving remnants

Re: efivarfs: fix error on write to new variable leaving remnants

[James Bottomley]

[added correct mailing list for bug report]
On Tue, 2025-02-25 at 12:10 +0000, Richard Hughes wrote:
> Hi,
> 
> I'm not sure what I'm supposed to do about:
> 
> commit 908af31f4896f2c0645031f8b74a89d3a8beb5b9
> Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date:   Sun Jan 19 10:12:12 2025 -0500
> 
>     efivarfs: fix error on write to new variable leaving remnants
>     
>     Make variable cleanup go through the fops release mechanism and
> use
>     zero inode size as the indicator to delete the file.  Since all
> EFI
>     variables must have an initial u32 attribute, zero size occurs
> either
>     because the update deleted the variable or because an
> unsuccessful
>     write after create caused the size never to be set in the first
> place.
>     In the case of multiple racing opens and closes, the open is
> counted
>     to ensure that the zero size check is done on the last close.
>     
>     Even though this fixes the bug that a create either not followed
> by a
>     write or followed by a write that errored would leave a remnant
> file
>     for the variable, the file will appear momentarily globally
> visible
>     until the last close of the fd deletes it.  This is safe because
> the
>     normal filesystem operations will mediate any races; however, it
> is
>     still possible for a directory listing at that instant between
> create
>     and close contain a zero size variable that doesn't exist in the
> EFI
>     table.
>     
>     Signed-off-by: James Bottomley
> <James.Bottomley@HansenPartnership.com>
>     Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> 
> It causes a regression in fwupd -- seen in
> https://github.com/fwupd/fwupd/issues/8495 and
> https://bugzilla.redhat.com/show_bug.cgi?id=2346831 so far -- and it
> seems broken for anyone (including me) updating to 6.14.

OK, so the problem with this as a bug report is that it doesn't explain
what you're doing.  However

> I can work around the behavior in
> https://github.com/fwupd/fwupd/pull/8500 (which is also the arguably
> correct thing to do) but it's going to cause a panic as I have to get
> an updated fwupd out on all distros so we'll need releases for
> multiple branches.

Reading the code in the fix, it looks like you were creating a file in
EFI (which is naturally zero length), then closing it (because glib gio
specifically has an API for this), then clearing the immutable bit and
then writing to it to actually create a variable?  

However, none of that dance is at all required.  A newly created file
naturally allows writing on the file descriptor you used to create it.
It's only if you open it again that the entry becomes immutable.  So
your update has the correct logic: if file exists clear immutable and
write otherwise add O_CREAT.

> I don't mind fixing fwupd (as it's doing a dumb thing) but could we
> revert the kernel change for 6.14 and give everyone a few weeks to
> update userspace? Thanks.

The change is rather embedded in a set of other fixes now.  If we
wanted a temporary and quickly removable work around for the current
kernel, I think using i_size to signal whether the file is newly
created and not written (0) or failed a write (1) and only removing the
file if it failed a write might be a simple two line fix.  That way we
still keep the benefit of cleanup on a failed write while not impacting
your pattern.

Can you confirm this has that effect?

Regards,

James

---

diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index cb1b6d0c3454..c294a8fc566d 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -57,10 +57,11 @@ static ssize_t efivarfs_file_write(struct file *file,
 
 	if (bytes == -ENOENT) {
 		/*
-		 * zero size signals to release that the write deleted
-		 * the variable
+		 * FIXME: temporary workaround for fwupdate, signal
+		 * failed write with a 1 to keep created but not
+		 * written files
 		 */
-		i_size_write(inode, 0);
+		i_size_write(inode, 1);
 	} else {
 		i_size_write(inode, datasize + sizeof(attributes));
 		inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
@@ -124,7 +125,8 @@ static int efivarfs_file_release(struct inode *inode, struct file *file)
 	struct efivar_entry *var = inode->i_private;
 
 	inode_lock(inode);
-	var->removed = (--var->open_count == 0 && i_size_read(inode) == 0);
+	/* FIXME: temporary work around for fwupdate */
+	var->removed = (--var->open_count == 0 && i_size_read(inode) == 1);
 	inode_unlock(inode);
 
 	if (var->removed)

Re: efivarfs: fix error on write to new variable leaving remnants

[Ard Biesheuvel]

On Tue, 25 Feb 2025 at 13:59, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> [added correct mailing list for bug report]
> On Tue, 2025-02-25 at 12:10 +0000, Richard Hughes wrote:
> > Hi,
> >
> > I'm not sure what I'm supposed to do about:
> >
> > commit 908af31f4896f2c0645031f8b74a89d3a8beb5b9
> > Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Date:   Sun Jan 19 10:12:12 2025 -0500
> >
> >     efivarfs: fix error on write to new variable leaving remnants
> >
...
> >
> > It causes a regression in fwupd -- seen in
> > https://github.com/fwupd/fwupd/issues/8495 and
> > https://bugzilla.redhat.com/show_bug.cgi?id=2346831 so far -- and it
> > seems broken for anyone (including me) updating to 6.14.
>
> OK, so the problem with this as a bug report is that it doesn't explain
> what you're doing.  However
>
> > I can work around the behavior in
> > https://github.com/fwupd/fwupd/pull/8500 (which is also the arguably
> > correct thing to do) but it's going to cause a panic as I have to get
> > an updated fwupd out on all distros so we'll need releases for
> > multiple branches.

This code was introduced in the merge window for v6.14, which is due
to be released end of March. How much time do you need?

Derailing LVFS is the last thing I want to do, but we all know how
this works: once a workaround is put in, it is never going to be
removed.

>
> Reading the code in the fix, it looks like you were creating a file in
> EFI (which is naturally zero length), then closing it (because glib gio
> specifically has an API for this), then clearing the immutable bit and
> then writing to it to actually create a variable?
>
> However, none of that dance is at all required.  A newly created file
> naturally allows writing on the file descriptor you used to create it.
> It's only if you open it again that the entry becomes immutable.

Should we be doing this in the first place? It makes sense for
efivarfs to mark variables that it doesn't know about as immutable if
they are present at boot - the original rationale here was systems
with buggy firmware, where an inadvertent 'rm -r
/sys/firmware/efivars/*' would brick the device.

But the fact that a newly created file becomes immutable once you
close and reopen it seems unnecessarily strict to me.

(I am aware that this is orthogonal to the issue at hand, but it is
something we might fix nonetheless)

Re: efivarfs: fix error on write to new variable leaving remnants

[James Bottomley]

On Wed, 2025-02-26 at 12:30 +0100, Ard Biesheuvel wrote:
> On Tue, 25 Feb 2025 at 13:59, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > 
> > [added correct mailing list for bug report]
> > On Tue, 2025-02-25 at 12:10 +0000, Richard Hughes wrote:
> > > Hi,
> > > 
> > > I'm not sure what I'm supposed to do about:
> > > 
> > > commit 908af31f4896f2c0645031f8b74a89d3a8beb5b9
> > > Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > Date:   Sun Jan 19 10:12:12 2025 -0500
> > > 
> > >     efivarfs: fix error on write to new variable leaving remnants
> > > 
> ...
> > > 
> > > It causes a regression in fwupd -- seen in
> > > https://github.com/fwupd/fwupd/issues/8495 and
> > > https://bugzilla.redhat.com/show_bug.cgi?id=2346831 so far -- and
> > > it seems broken for anyone (including me) updating to 6.14.
> > 
> > OK, so the problem with this as a bug report is that it doesn't
> > explain what you're doing.  However
> > 
> > > I can work around the behavior in
> > > https://github.com/fwupd/fwupd/pull/8500 (which is also the
> > > arguably correct thing to do) but it's going to cause a panic as
> > > I have to get an updated fwupd out on all distros so we'll need
> > > releases for multiple branches.
> 
> This code was introduced in the merge window for v6.14, which is due
> to be released end of March. How much time do you need?

The request in the original was for a few more months.

> Derailing LVFS is the last thing I want to do, but we all know how
> this works: once a workaround is put in, it is never going to be
> removed.

I think simply queueing the workaround now (If it works; I'd still like
to see a tested by since it's nowhere near a pure revert) in the fixes
branch for the v14-rc and immediately queueing its revert in the
updates for v15 would give an additional three months automatically and
no-one would have to do anything more (unless more time were needed).

Regards,

James

Re: efivarfs: fix error on write to new variable leaving remnants

[James Bottomley]

On Wed, 2025-02-26 at 12:30 +0100, Ard Biesheuvel wrote:
> On Tue, 25 Feb 2025 at 13:59, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
[...]
> > Reading the code in the fix, it looks like you were creating a file
> > in EFI (which is naturally zero length), then closing it (because
> > glib gio specifically has an API for this), then clearing the
> > immutable bit and then writing to it to actually create a variable?
> > 
> > However, none of that dance is at all required.  A newly created
> > file naturally allows writing on the file descriptor you used to
> > create it. It's only if you open it again that the entry becomes
> > immutable.
> 
> Should we be doing this in the first place? It makes sense for
> efivarfs to mark variables that it doesn't know about as immutable if
> they are present at boot - the original rationale here was systems
> with buggy firmware, where an inadvertent 'rm -r
> /sys/firmware/efivars/*' would brick the device.
> 
> But the fact that a newly created file becomes immutable once you
> close and reopen it seems unnecessarily strict to me.
> 
> (I am aware that this is orthogonal to the issue at hand, but it is
> something we might fix nonetheless)

Well, I've always thought it was silly ever since it was introduced:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ed8b0de5a33d2a2557dce7f9429dca8cb5bc5879

The rationale it was because root could brick their system by doing rm
-fr never made any sense, since if root is going to do rm -fr they're
asking to become unbootable anyway.

All of us who have code that manipulates the variables simply added
remove immutable flag code like the above.  If it does get removed,
efivarfs will still need to respond to get/setfsxattr though to make
sure that code keeps working, so it can't be a clean revert.

Regards,

James

Re: efivarfs: fix error on write to new variable leaving remnants

[Richard Hughes]

On Tuesday, 25 February 2025 at 12:59, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> Reading the code in the fix, it looks like you were creating a file in
> EFI (which is naturally zero length), then closing it (because glib gio
> specifically has an API for this), then clearing the immutable bit and
> then writing to it to actually create a variable?

Correct.

> However, none of that dance is at all required. A newly created file
> naturally allows writing on the file descriptor you used to create it.

Yes, the original code was ported from libefivar iirc, hence why it's not exactly idiomatic.

> It's only if you open it again that the entry becomes immutable. So
> your update has the correct logic: if file exists clear immutable and
> write otherwise add O_CREAT.

Agree.

> The change is rather embedded in a set of other fixes now. If we
> wanted a temporary and quickly removable work around for the current
> kernel, I think using i_size to signal whether the file is newly
> created and not written (0) or failed a write (1) and only removing the
> file if it failed a write might be a simple two line fix. That way we
> still keep the benefit of cleanup on a failed write while not impacting
> your pattern.
> Can you confirm this has that effect?

The fixup does seem to work so please feel free to add "Tested-by: Richard Hughes <richard@hughsie.com>" -- you can revert it after a few months if you like. It'll certainly take the pressure off, and we have a "known issue" we can use on the LVFS for people reporting problems: https://github.com/fwupd/fwupd/wiki/LVFS-Triaged-Issue:-Linux-6.14-efivarfs-regression

Thanks!

Richard.