Re: [PATCH] ima: fix freeing ongoing ahash_request

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

Hi Sasha,

On Fri, 2019-06-28 at 10:14 +0200, Sascha Hauer wrote:
> integrity_kernel_read() can fail in which case we forward to call
> ahash_request_free() on a currently running request. We have to wait
> for its completion before we can free the request.
> 
> This was observed by interrupting a "find / -type f -xdev -print0 | xargs -0
> cat 1>/dev/null" with ctrl-c on an IMA enabled filesystem.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  security/integrity/ima/ima_crypto.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 16a4f45863b1..6a60bdb322b1 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -271,8 +271,10 @@ static int ima_calc_file_hash_atfm(struct file *file,
>  		rbuf_len = min_t(loff_t, i_size - offset, rbuf_size[active]);
>  		rc = integrity_kernel_read(file, offset, rbuf[active],
>  					   rbuf_len);
> -		if (rc != rbuf_len)
> +		if (rc != rbuf_len) {
> +			ahash_wait(ahash_rc, &wait);
>  			goto out3;
> +		}

The normal case when "rc != rbuf_len" is when the last block of the
file data is read.  In that case the "ahash_wait" isn't needed.  Is
there a performance penalty for adding this wait?  Could you
differentiate between the last buffer and failure?

Immediately before "out3:" there's a call to ahash_wait().  There are
three "goto out3".  This is the only place that skips the call to
ahash_wait().  If we do need to add it, it would be better to move the
"out3:" definition and remove the other calls to ahash_wait().

Mimi

>  
>  		if (rbuf[1] && offset) {
>  			/* Using two buffers, and it is not the first