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

Al Viro
2024-02-02T16:05:14Z

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".