| Message ID | 20241210170224.19159-7-James.Bottomley@HansenPartnership.com |
|---|---|
| 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
> 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...
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(-)