[6/6] 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.

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 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 variable that doesn't exist in the EFI table.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 fs/efivarfs/file.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

On Tue, Dec 10, 2024 at 12:02:24PM -0500, James Bottomley wrote:
> 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.
> 
> 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 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 variable that doesn't exist in the EFI table.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  fs/efivarfs/file.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
> index 23c51d62f902..edf363f395f5 100644
> --- a/fs/efivarfs/file.c
> +++ b/fs/efivarfs/file.c
> @@ -38,22 +38,24 @@ static ssize_t efivarfs_file_write(struct file *file,
>  
>  	bytes = efivar_entry_set_get_size(var, attributes, &datasize,
>  					  data, &set);
> -	if (!set && bytes) {
> +	if (!set) {
>  		if (bytes == -ENOENT)
>  			bytes = -EIO;
>  		goto out;
>  	}
>  
> +	inode_lock(inode);
>  	if (bytes == -ENOENT) {
> -		drop_nlink(inode);
> -		d_delete(file->f_path.dentry);
> -		dput(file->f_path.dentry);
> +		/*
> +		 * zero size signals to release that the write deleted
> +		 * the variable
> +		 */
> +		i_size_write(inode, 0);
>  	} else {
> -		inode_lock(inode);
>  		i_size_write(inode, datasize + sizeof(attributes));
>  		inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
> -		inode_unlock(inode);
>  	}
> +	inode_unlock(inode);
>  
>  	bytes = count;
>  
> @@ -106,8 +108,19 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
>  	return size;
>  }
>  
> +static int efivarfs_file_release(struct inode *inode, struct file *file)
> +{
> +	if (i_size_read(inode) == 0) {
> +		drop_nlink(inode);
> +		d_delete(file->f_path.dentry);
> +		dput(file->f_path.dentry);
> +	}

Without wider context the dput() looks UAF-y as __fput() will do:

struct dentry *dentry = file->f_path.dentry;
if (file->f_op->release)
        file->f_op->release(inode, file);
dput(dentry);

Is there an extra reference on file->f_path.dentry taken somewhere?

On Wed, 2024-12-11 at 12:16 +0100, Christian Brauner wrote:
> On Tue, Dec 10, 2024 at 12:02:24PM -0500, James Bottomley wrote:
[...]
> > +static int efivarfs_file_release(struct inode *inode, struct file
> > *file)
> > +{
> > +       if (i_size_read(inode) == 0) {
> > +               drop_nlink(inode);
> > +               d_delete(file->f_path.dentry);
> > +               dput(file->f_path.dentry);
> > +       }
> 
> Without wider context the dput() looks UAF-y as __fput() will do:
> 
> struct dentry *dentry = file->f_path.dentry;
> if (file->f_op->release)
>         file->f_op->release(inode, file);
> dput(dentry);
> 
> Is there an extra reference on file->f_path.dentry taken somewhere?

Heh, well, this is why I cc'd fsdevel to make sure I got all the fs
bits I used to be familiar with, but knowledge of which has atrophied,
correct.

I think it's paired with the extra dget() just after d_instantiate() in
fs/efivarfs/inode.c:efivarfs_create().  The reason being this is a
pseudo-filesystem so all the dentries representing objects have to be
born with a positive reference count to prevent them being reclaimed
under memory pressure.

Regards,

James

On Tue, 2024-12-10 at 12:02 -0500, James Bottomley wrote:
> 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 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 variable that doesn't exist in the EFI table.

Systemd doesn't like 0 length files appearing in efivarfs, even if only
momentarily, so I think this needs updating to prevent even momentary
instances of zero length files:

https://github.com/systemd/systemd/issues/34304

These occur for two reasons

   1. The system has hibernated and resumed and the dcache entries are
      now out of sync with the original variables
   2. between the create and a successful write of a variable being
      created in efivarfs

1. can only really be fixed by adding a hibernation hook to the
filesystem code, which would be a separate patch set (which I'll work
on after we get this upstream); but 2. can be fixed by ensuring that
all variables returned from .create aren't visible in the directory
listing until a successful write.

Since we need the file to be visible to lookups but not the directory,
the only two ways of doing this are either to mark the directory in a
way that libfs.c:dcache_readdir() won't see it ... I think this would
have to be marking it as a cursor (we'd remove the cursor mark on
successful write); or to implement our own .iterate_shared function and
hijack the actor to skip newly created files (this is similar to what
overlayfs does to merge directories) which would be identified as
having zero size.

Do the fs people have a preference? The cursor mark is simpler to
implement but depends on internal libfs.c magic. The actor hijack is at
least something that already exists, so would be less prone to breaking
due to internal changes.

Regards,

James

> Do the fs people have a preference? The cursor mark is simpler to
> implement but depends on internal libfs.c magic. The actor hijack is at
> least something that already exists, so would be less prone to breaking
> due to internal changes.

I think making this filesystem specific is better than plumbing this
into dcache_readdir().

On Sun, 2024-12-22 at 11:12 +0100, Christian Brauner wrote:
> > Do the fs people have a preference? The cursor mark is simpler to
> > implement but depends on internal libfs.c magic. The actor hijack
> > is at least something that already exists, so would be less prone
> > to breaking due to internal changes.
> 
> I think making this filesystem specific is better than plumbing this
> into dcache_readdir().

Neither would require changes to libfs.c: the dcache_readdir already
does the ignore cursor behaviour; the code in efivarfs would just set
the cursor flag to take advantage of it.

However, on further consideration I think making the zero size entries
not show up in the directory listing doesn't really help anything: they
still have to be found on lookup (otherwise nothing mediates a same
variable create race) which means an open by name from userspace will
still get hold of one.  The good news is that after this change they
should only show up fleetingly instead of hanging around until the next
reboot, so the chances of anyone hitting a problem is much smaller.

Regards,

James

On Mon, Dec 23, 2024 at 02:52:12PM -0500, James Bottomley wrote:
>  
> +static int efivarfs_file_release(struct inode *inode, struct file *file)
> +{
> +	inode_lock(inode);
> +	if (i_size_read(inode) == 0 && !d_unhashed(file->f_path.dentry)) {
> +		drop_nlink(inode);
> +		d_delete(file->f_path.dentry);
> +		dput(file->f_path.dentry);
> +	}
> +	inode_unlock(inode);
> +	return 0;
> +}

This is wrong; so's existing logics for removal from write().  Think
what happens if you open the sucker, have something bound on top of
it and do that deleting write().

Let me look into that area...

On Mon, Dec 23, 2024 at 05:56:04PM -0500, James Bottomley wrote:
> > Let me look into that area...
> 
> I thought about this some more.  I could see a twisted container use
> case where something like this might happen (expose some but not all
> efi variables to the container).
> 
> So, help me understand the subtleties here.  If it's the target of a
> bind mount, that's all OK, because you are allowed to delete the
> target.  If something is bind mounted on to an efivarfs object, the
> is_local_mountpoint() check in vfs_unlink would usually trip and
> prevent deletion (so the subtree doesn't become unreachable).  If I
> were to duplicate that, I think the best way would be simply to do a
> d_put() in the file->release function and implement drop_nlink() in
> d_prune (since last put will always call __dentry_kill)?

Refcounting is not an issue.  At all.

Inability to find and evict the mount, OTOH, very much is.  And after your
blind d_delete() that's precisely what will happen.

You are steadily moving towards more and more convoluted crap, in places
where it really does not belong.

If anything, simple_recursive_removal() should be used for that, instead
of trying to open-code bizarre subsets of its functionality...

On Mon, 2024-12-23 at 23:12 +0000, Al Viro wrote:
> On Mon, Dec 23, 2024 at 05:56:04PM -0500, James Bottomley wrote:
> > > Let me look into that area...
> > 
> > I thought about this some more.  I could see a twisted container
> > use case where something like this might happen (expose some but
> > not all efi variables to the container).
> > 
> > So, help me understand the subtleties here.  If it's the target of
> > a bind mount, that's all OK, because you are allowed to delete the
> > target.  If something is bind mounted on to an efivarfs object, the
> > is_local_mountpoint() check in vfs_unlink would usually trip and
> > prevent deletion (so the subtree doesn't become unreachable).  If I
> > were to duplicate that, I think the best way would be simply to do
> > a d_put() in the file->release function and implement drop_nlink()
> > in d_prune (since last put will always call __dentry_kill)?
> 
> Refcounting is not an issue.  At all.
> 
> Inability to find and evict the mount, OTOH, very much is.  And after
> your blind d_delete() that's precisely what will happen.
> 
> You are steadily moving towards more and more convoluted crap, in
> places where it really does not belong.
> 
> If anything, simple_recursive_removal() should be used for that,
> instead of trying to open-code bizarre subsets of its
> functionality...

OK, so like the below?

In my defence, simple_recursive_removal() isn't mentioned in
Documentation/filesystems and the function itself also has no
documentation, so even if I had stumbled across it in libfs.c the
recursive in the name would have lead me to believe it wasn't for
single dentry removal.

Regards,

James

---8>8>8><8<8<8---

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Subject: [PATCH] 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.

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 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 variable that doesn't exist in the EFI table.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 fs/efivarfs/file.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 23c51d62f902..0e545c8be173 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -36,28 +36,41 @@ static ssize_t efivarfs_file_write(struct file *file,
 	if (IS_ERR(data))
 		return PTR_ERR(data);
 
+	inode_lock(inode);
+	if (d_unhashed(file->f_path.dentry)) {
+		/*
+		 * file got removed; don't allow a set.  Caused by an
+		 * unsuccessful create or successful delete write
+		 * racing with us.
+		 */
+		bytes = -EIO;
+		goto out;
+	}
+
 	bytes = efivar_entry_set_get_size(var, attributes, &datasize,
 					  data, &set);
-	if (!set && bytes) {
+	if (!set) {
 		if (bytes == -ENOENT)
 			bytes = -EIO;
 		goto out;
 	}
 
 	if (bytes == -ENOENT) {
-		drop_nlink(inode);
-		d_delete(file->f_path.dentry);
-		dput(file->f_path.dentry);
+		/*
+		 * zero size signals to release that the write deleted
+		 * the variable
+		 */
+		i_size_write(inode, 0);
 	} else {
-		inode_lock(inode);
 		i_size_write(inode, datasize + sizeof(attributes));
 		inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
-		inode_unlock(inode);
 	}
 
 	bytes = count;
 
 out:
+	inode_unlock(inode);
+
 	kfree(data);
 
 	return bytes;
@@ -106,8 +119,17 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 	return size;
 }
 
+static int efivarfs_file_release(struct inode *inode, struct file *file)
+{
+	if (i_size_read(inode) == 0)
+		simple_recursive_removal(file->f_path.dentry, NULL);
+
+	return 0;
+}
+
 const struct file_operations efivarfs_file_operations = {
-	.open	= simple_open,
-	.read	= efivarfs_file_read,
-	.write	= efivarfs_file_write,
+	.open		= simple_open,
+	.read		= efivarfs_file_read,
+	.write		= efivarfs_file_write,
+	.release	= efivarfs_file_release,
 };

On Tue, 2024-12-24 at 08:07 -0500, James Bottomley wrote:
[...]

> On the other hand the most intuitive thing would be to remove zero
> length files on last close, not first, so if you have a thought on
> how to do that easily, I'm all ears.

I could do this by adding an open_count to the i_private data struct
efivar_entry and reimplementing simple_open as an efivarfs specific
open that increments this count and decrementing it in ->release(). 
That's still somewhat adding "more convoluted crap", though ...

Regards,

James