| Message ID | 20241210170224.19159-7-James.Bottomley@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | convert efivarfs to manage object data correctly | expand |
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
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); + } + 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, };
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(-)