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

[Mimi Zohar <zohar@linux.ibm.com>]

Hi Simon,

On Wed, 2021-07-28 at 07:00 +0000, THOBY Simon wrote: 
> >> +
> >> +	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 ?

Memory pressure isn't the reason for preventing the xattr write.  It's
the reason for not being able to audit the setxattr failure.

thanks,

Mimi