Re: [PATCH RFC 22/28] efivarfs: stop using SB_RDONLY for read-only efivar ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] efi: NULL pointer derefs in efivar_lock() and efivar_set_variable_locked()
--

commit 8baf8c78bb84688313f856459551571bca3c4964
Author: Christian Brauner <brauner@kernel.org>

efivarfs: stop using SB_RDONLY for read-only efivar ops

This commit introduces a private flag EFIVARFS_OPS_RDONLY to track whether
efivar operations are read-only, replacing the use of SB_RDONLY. It aims to
resolve freeze and thaw deadlocks caused by concurrent modifications to the
superblock flags.

> diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
> index cb1b6d0c34545..a074145fa57e1 100644
> --- a/fs/efivarfs/file.c
> +++ b/fs/efivarfs/file.c
> @@ -23,6 +23,9 @@ static ssize_t efivarfs_file_write(struct file *file,
>  	ssize_t bytes;
>  	bool set = false;
>  
> +	if (efivarfs_ops_rdonly(inode->i_sb))
> +		return -EROFS;
> +

[Severity: High]
Does this lockless validation allow a race condition that leads to a NULL
pointer dereference?

If a user-space process initiates a write, it evaluates this check outside of
efivars_lock. Concurrently, if a module like tee_stmm_efi is removed or
fails probing, efivars_unregister() may set __efivars to NULL.

When the racing write thread subsequently attempts to acquire the lock via
efivar_lock(), it does the following:

drivers/firmware/efi/vars.c:efivar_lock() {
    if (down_interruptible(&efivars_lock))
        return -EINTR;
    if (!__efivars->ops) {
        up(&efivars_lock);
        return -ENODEV;
    }
...

Evaluating !__efivars->ops will unconditionally dereference __efivars,
crashing if it is NULL.

Alternatively, if a generic_ops struct lacking a set_variable callback is
registered immediately after, efivar_set_variable_locked() executes:

drivers/firmware/efi/vars.c:efivar_set_variable_locked() {
...
    setvar = __efivars->ops->set_variable_nonblocking;
    if (!setvar || !nonblocking)
         setvar = __efivars->ops->set_variable;

    return setvar(name, vendor, attr, data_size, data);
}

This attempts to call a NULL setvar pointer without validation, which also
leads to a crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-work-fs-sb_rdonly-v1-0-60743a3dc326@kernel.org?part=22