Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, Feb 02, 2024 at 01:01:32PM +0200, Amir Goldstein wrote:
> The only remaining user of ->d_real() method is d_real_inode(), which
> passed NULL inode argument to get the real data dentry.
> 
> There are no longer any users that call ->d_real() with a non-NULL
> inode argument for getting a detry from a specific underlying layer.
> 
> Remove the inode argument of the method and replace it with an integer
> 'type' argument, to allow callers to request the real metadata dentry
> instead of the real data dentry.
> 
> All the current users of d_real_inode() (e.g. uprobe) continue to get
> the real data inode.  Caller that need to get the real metadata inode
> (e.g. IMA/EVM) can use d_inode(d_real(dentry, D_REAL_METADATA)).

Hmm...  Speaking of the callers, could somebody try explain to IMA
folks that they _still_ have a blatant UAF in ima_collect_measurement()?
I gave up after several attempts years ago...

int ima_collect_measurement(struct integrity_iint_cache *iint,
                            struct file *file, void *buf, loff_t size,
                            enum hash_algo algo, struct modsig *modsig)
{
        const char *audit_cause = "failed";
        struct inode *inode = file_inode(file);
        struct inode *real_inode = d_real_inode(file_dentry(file));
        const char *filename = file->f_path.dentry->d_name.name;

The name is longer than 40 characters, and thus separately allocated.

	...
Somebody renames the file, now the name is short and ->d_name.name points to
embedded array.  The reference to external name is dropped and it's freed
after an RCU delay.
	...
        tmpbuf = krealloc(iint->ima_hash, length, GFP_NOFS);
We block, RCU delay expires and filename points to freed memory object.
	...

                integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
                                    filename, "collect_data", audit_cause,
                                    result, 0);

Which calls integrity_audit_message(), where we hit
                audit_log_untrustedstring(ab, fname);
with fname being our dangling pointer.

Use After Free.  Really.  And "untrusted" in the function name does not
refer to "it might be pointing to unmapped page" - it's just "don't
expect anything from the characters you might find there, including
the presence of NUL".