[PATCH] ima: fix freeing ongoing ahash_request

[PATCH] ima: fix freeing ongoing ahash_request

[Sascha Hauer]

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;
+		}
 
 		if (rbuf[1] && offset) {
 			/* Using two buffers, and it is not the first
-- 
2.20.1

Re: [PATCH] ima: fix freeing ongoing ahash_request

[Mimi Zohar]

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

Re: [PATCH] ima: fix freeing ongoing ahash_request

[Sascha Hauer]

On Sun, Jun 30, 2019 at 07:01:44PM -0400, Mimi Zohar wrote:
> 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. 

When integrity_kernel_read() returns a value smaller than 0 then it's
clearly an error and we want to bail out. The case when
integrity_kernel_read() returns a short read though isn't properly
handled. We have:

		rc = integrity_kernel_read(file, offset, rbuf[active],
					   rbuf_len);
		if (rc != rbuf_len)
			goto out3;

		...

out3:
	ima_free_pages(rbuf[0], rbuf_size[0]);
	ima_free_pages(rbuf[1], rbuf_size[1]);
out2:
	if (!rc) {
		ahash_request_set_crypt(req, NULL, hash->digest, 0);
		rc = ahash_wait(crypto_ahash_final(req), &wait);
	}
out1:
	ahash_request_free(req);
	return rc;


So on a short read we never finish the ahash request and we return a
positive number from this function which it seems isn't expected from
the callers.

I'm not sure if we have to handle a short read, but currently it isn't
handled. It seems we have to sort that out first.

> 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().

The cases are different. Two times we call ahash_wait() and if that
fails we jump to "out3:". In the case I handle here we are already in
the error path and still have to call ahash_wait(). We also can't use
the ahash_wait() after the loop because that would hide the error value
we want to return (after the loop we have rc = ahash_wait(), we would
return successfully if we'd jump there).

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] ima: fix freeing ongoing ahash_request

[Mimi Zohar]

On Mon, 2019-07-01 at 09:27 +0200, Sascha Hauer wrote:
> On Sun, Jun 30, 2019 at 07:01:44PM -0400, Mimi Zohar wrote:
> > 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. 
> 
> When integrity_kernel_read() returns a value smaller than 0 then it's
> clearly an error and we want to bail out. The case when
> integrity_kernel_read() returns a short read though isn't properly
> handled. We have:
> 
> 		rc = integrity_kernel_read(file, offset, rbuf[active],
> 					   rbuf_len);
> 		if (rc != rbuf_len)
> 			goto out3;
> 
> 		...
> 
> out3:
> 	ima_free_pages(rbuf[0], rbuf_size[0]);
> 	ima_free_pages(rbuf[1], rbuf_size[1]);
> out2:
> 	if (!rc) {
> 		ahash_request_set_crypt(req, NULL, hash->digest, 0);
> 		rc = ahash_wait(crypto_ahash_final(req), &wait);
> 	}
> out1:
> 	ahash_request_free(req);
> 	return rc;
> 
> 
> So on a short read we never finish the ahash request and we return a
> positive number from this function which it seems isn't expected from
> the callers.
> 
> I'm not sure if we have to handle a short read, but currently it isn't
> handled. It seems we have to sort that out first.

Agreed.  For this code to work, which it does, it must be returning 0.
 So I would assume your code should differentiate between 0 and < 0.

> 
> > 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().
> 
> The cases are different. Two times we call ahash_wait() and if that
> fails we jump to "out3:". In the case I handle here we are already in
> the error path and still have to call ahash_wait(). We also can't use
> the ahash_wait() after the loop because that would hide the error value
> we want to return (after the loop we have rc = ahash_wait(), we would
> return successfully if we'd jump there).

Thank you for the explanation.  The code should be documented,
otherwise someone is going to "clean" it up.

Mimi