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 12:34:15PM -0500, Stefan Berger wrote:

> I suppose this would provide a stable name?
> 
> diff --git a/security/integrity/ima/ima_api.c
> b/security/integrity/ima/ima_api.c
> index 597ea0c4d72f..48ae6911139b 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -244,7 +244,6 @@ int ima_collect_measurement(struct integrity_iint_cache
> *iint,
>         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;
>         struct ima_max_digest_data hash;
>         struct kstat stat;
>         int result = 0;
> @@ -313,11 +312,17 @@ int ima_collect_measurement(struct
> integrity_iint_cache *iint,
>                 iint->flags |= IMA_COLLECTED;
>  out:
>         if (result) {
> +               struct qstr *qstr = &file->f_path.dentry->d_name;
> +               char buf[NAME_MAX + 1];
> +
>                 if (file->f_flags & O_DIRECT)
>                         audit_cause = "failed(directio)";
> 
> +               memcpy(buf, qstr->name, qstr->len);
> +               buf[qstr->len] = 0;

	Think what happens if you fetch ->len in state prior to
rename and ->name - after.  memcpy() from one memory object
with length that matches another, UAF right there.

	If you want to take a snapshot of the name, just use
take_dentry_name_snapshot() - that will copy name if it's
short or grab a reference to external name if the length is
>= 40, all of that under ->d_lock to stabilize things.

	Paired with release_dentry_name_snapshot(), to
drop the reference to external name if one had been taken.
No need to copy in long case (external names are never
rewritten) and it's kinder on the stack footprint that
way (56 bytes vs. 256).

	Something like this (completely untested):

fix a UAF in ima_collect_measurement()

->d_name.name can change on rename and the earlier value can get freed;
there are conditions sufficient to stabilize it (->d_lock on denty,
->d_lock on its parent, ->i_rwsem exclusive on the parent's inode,
rename_lock), but none of those are met in ima_collect_measurement().
Take a stable snapshot of name instead.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 597ea0c4d72f..d8be2280d97b 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -244,7 +244,6 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
 	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;
 	struct ima_max_digest_data hash;
 	struct kstat stat;
 	int result = 0;
@@ -313,12 +312,16 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
 		iint->flags |= IMA_COLLECTED;
 out:
 	if (result) {
+		struct name_snapshot filename;
+
+		take_dentry_name_snapshot(&filename, file->f_path.dentry);
 		if (file->f_flags & O_DIRECT)
 			audit_cause = "failed(directio)";
 
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
-				    filename, "collect_data", audit_cause,
-				    result, 0);
+				    filename.name.name, "collect_data",
+				    audit_cause, result, 0);
+		release_dentry_name_snapshot(&filename);
 	}
 	return result;
 }