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

Stefan Berger
2024-02-02T18:32:47Z

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