On Wed, 2024-12-11 at 07:39 -0500, James Bottomley wrote:
> 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.
Actually on further testing, this did turn out to be a use after free.
Not because of the dput, but because file->release is called for every
closed filehandle, so if you open the file for creation more than once,
both closes will try to delete it and ... oops.
The way I thought of mediating this is to check d_hashed in the file-
>release to see if the file has already been deleted. That also means
we need a d_hashed() check in write because we can't resurrect the now
deleted file. And finally something needs to mediate the check and
remove or check and add, so I used the inode semaphore for that. The
updated patch is below and now passes the concurrency tests.
Regards,
James
------8>8>8><8<8<8-------------
From: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
Subject: [PATCH 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@xxxxxxxxxxxxxxxxxxxxx>
---
fs/efivarfs/file.c | 44 +++++++++++++++++++++++++++++++++++---------
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 23c51d62f902..70a673e7fda3 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,21 @@ 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)
+{
+ 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;
+}
+
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,
};
--
2.35.3
