Re: [PATCH v4 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms

[THOBY Simon <Simon.THOBY@viveris.fr>]

Hi Mimi,

On 7/27/21 10:32 PM, Mimi Zohar wrote:
> [Cc'ing Paul Moore]
> 
> Hi Simon,
> 

[snip]

> 
>> +
>> +	if (likely(dentry_hash == ima_hash_algo
>> +	    || crypto_has_alg(hash_algo_name[dentry_hash], 0, 0)))
>> +		return 0;
>> +
>> +	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
>> +	/* no memory available ? no file path for you */
> 
> The comment here is unnecessary.  Avoid or limit comments inside a
> function.  Refer to the section "8) Commenting" in
> Documentation/process/coding-style.rst
> 
>> +	if (pathbuf)
>> +		path = dentry_path(dentry, pathbuf, PATH_MAX);
>> +
>> +	/* disallow xattr writes with algorithms not built in the kernel */
>> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
>> +		path, "collect_data", "unavailable-hash-algorithm", res, 0);
> 
> This will emit an audit message without the filename when !path.  Is
> this what you intended?
> 

This is what I was clumsily trying to explain in the previous comment: if we cannot
allocate memory for a file path, I thought it best to log the audit message without
the path than fail with a -ENOMEM (auditing will also try to allocate a memory buffer
too, but a bit smaller, and memory could have been reclaimed between the two calls,
so the auditing operation may succeed).

Of course I could also return -ENOMEM, and it would happily propagate back to the user.

What do you think ?

Thanks,
Simon