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

[Stefan Berger <stefanb@linux.ibm.com>]

On 2/2/24 13:27, Al Viro wrote:
> 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.

I had exactly what you show below (inspired by overlayfs) but then 
looked around whether there's another way of doing it and I saw copies 
being made.

Thanks.


> 
> 	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;
>   }

I can take it from here unless you want to formally post it.

    Stefan