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@xxxxxxxxxxxxxxxxxxxxx>
> ---
> 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?
