Re: [PATCH] securityfs: Add missing d_delete() call on removal

[Al Viro]

On Tue, May 05, 2020 at 04:40:35PM -0700, Kees Cook wrote:
> After using simple_unlink(), a call to d_delete() is needed in addition
> to dput().
> 
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
> Is this correct? I went looking around and there are a lot of variations
> on the simple_unlink() pattern...
> 
> Many using explicit locking and combinations of d_drop(), __d_drop(), etc.

Quite a few of those should switch to simple_recursive_removal().  As for
securityfs...  d_drop() is _probably_ a saner variant, but looking at the
callers of that thing... at least IMA ones seem to be garbage.

They _might_ work since nobody else is around that early to screw them over,
but... I'm not too optimistic about that.  And the lack of d_delete()/d_drop()
here is the least of the problems, really - look at e.g. the bulk of those
suckers in ima_fs_init().  And check the callchain - it'll lead you to this
        if (error && strcmp(hash_algo_name[ima_hash_algo],
                            CONFIG_IMA_DEFAULT_HASH) != 0) {
                pr_info("Allocating %s failed, going to use default hash algorithm %s\n",
                        hash_algo_name[ima_hash_algo], CONFIG_IMA_DEFAULT_HASH);
                hash_setup_done = 0;
                hash_setup(CONFIG_IMA_DEFAULT_HASH);
                error = ima_init();
        }

        error = register_blocking_lsm_notifier(&ima_lsm_policy_notifier);

And the other IMA caller (in ima_release_policy()) is... misguided, to put it
politely.  This kind of "unlink on final close" makes no sense - if nothing
else, it can be looked up until that very point.  Moreover, this
        inode->i_mode &= ~S_IWUSR;
is obviously racy wrt permission(), and there's no damn reason to do it there.
These checks belong in ->open() and they shouldn't rely upon the damn thing
disappearing from directory or permission() failing, etc..

And looking at their ->open()... ouch.  ->f_flags & O_ACCMODE is almost
never the right thing to check.  It should be looking at ->f_mode &
FMODE_{READ,WRITE}.

I hadn't looked into details for EVM, but at the first glance it's similar
to IMA failure handling.  And probably relies upon nobody being able to
open that stuff while the things are being set up.  There seems to be
something in TPM as well - and by the look of it they are trying to work
around the use of unsuitable primitive, and none too well, at that.
I mean,
        int err;
        struct seq_file *seq;
        struct tpm_chip_seqops *chip_seqops;
        const struct seq_operations *seqops;
        struct tpm_chip *chip;

        inode_lock(inode);
        if (!inode->i_private) {
                inode_unlock(inode);
                return -ENODEV;
        }
        chip_seqops = (struct tpm_chip_seqops *)inode->i_private;
        seqops = chip_seqops->seqops;
        chip = chip_seqops->chip;
        get_device(&chip->dev);
        inode_unlock(inode);

        /* now register seq file */
        err = seq_open(file, seqops);
        if (!err) {
                seq = file->private_data;
                seq->private = chip;
        }

        return err;
doesn't look sane - late error would appear to leak &chip->dev...