* [PATCH] ima: avoid appraise error for hash calc interrupt
@ 2019-11-11 19:23 Patrick Callaghan
2019-11-11 22:29 ` Lakshmi Ramasubramanian
2019-11-13 7:52 ` Sascha Hauer
0 siblings, 2 replies; 10+ messages in thread
From: Patrick Callaghan @ 2019-11-11 19:23 UTC (permalink / raw)
To: linux-integrity; +Cc: linux-kernel, Sascha Hauer, Patrick Callaghan, Mimi Zohar
The integrity_kernel_read() call in ima_calc_file_hash_tfm() can return
a value of 0 before all bytes of the file are read. A value of 0 would
normally indicate an EOF. This has been observed if a user process is
causing a file appraisal and is terminated with a SIGTERM signal. The
most common occurrence of seeing the problem is if a shutdown or systemd
reload is initiated while files are being appraised.
The problem is similar to commit <f5e1040196db> (ima: always return
negative code for error) that fixed the problem in
ima_calc_file_hash_atfm().
Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Patrick Callaghan <patrickc@linux.ibm.com>
---
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 73044fc..7967a69 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -362,8 +362,10 @@ static int ima_calc_file_hash_tfm(struct file *file,
rc = rbuf_len;
break;
}
- if (rbuf_len == 0)
+ if (rbuf_len == 0) { /* unexpected EOF */
+ rc = -EINVAL;
break;
+ }
offset += rbuf_len;
rc = crypto_shash_update(shash, rbuf, rbuf_len);
--
2.8.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] ima: avoid appraise error for hash calc interrupt
2019-11-11 19:23 [PATCH] ima: avoid appraise error for hash calc interrupt Patrick Callaghan
@ 2019-11-11 22:29 ` Lakshmi Ramasubramanian
2019-11-12 17:14 ` Mimi Zohar
2019-11-13 7:52 ` Sascha Hauer
1 sibling, 1 reply; 10+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-11 22:29 UTC (permalink / raw)
To: Patrick Callaghan, linux-integrity; +Cc: linux-kernel, Sascha Hauer, Mimi Zohar
On 11/11/19 11:23 AM, Patrick Callaghan wrote:
> - if (rbuf_len == 0)
> + if (rbuf_len == 0) { /* unexpected EOF */
> + rc = -EINVAL;
> break;
> + }
> offset += rbuf_len;
Should there be an additional check to validate that (offset + rbuf_len)
is less than i_size before calling cypto_shash_update (since rbuf_len is
one of the parameters for this call)?
if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) {
rc = -EINVAL;
break;
}
offset += rbuf_len;
> rc = crypto_shash_update(shash, rbuf, rbuf_len);
-lakshmi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ima: avoid appraise error for hash calc interrupt
2019-11-11 22:29 ` Lakshmi Ramasubramanian
@ 2019-11-12 17:14 ` Mimi Zohar
2019-11-12 17:33 ` Lakshmi Ramasubramanian
0 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2019-11-12 17:14 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, Patrick Callaghan, linux-integrity
Cc: linux-kernel, Sascha Hauer
On Mon, 2019-11-11 at 14:29 -0800, Lakshmi Ramasubramanian wrote:
> On 11/11/19 11:23 AM, Patrick Callaghan wrote:
>
> > - if (rbuf_len == 0)
> > + if (rbuf_len == 0) { /* unexpected EOF */
> > + rc = -EINVAL;
> > break;
> > + }
> > offset += rbuf_len;
>
> Should there be an additional check to validate that (offset + rbuf_len)
> is less than i_size before calling cypto_shash_update (since rbuf_len is
> one of the parameters for this call)?
The "while" statement enforces that.
Mimi
>
> if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) {
> rc = -EINVAL;
> break;
> }
> offset += rbuf_len;
>
> > rc = crypto_shash_update(shash, rbuf, rbuf_len);
>
> -lakshmi
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ima: avoid appraise error for hash calc interrupt
2019-11-12 17:14 ` Mimi Zohar
@ 2019-11-12 17:33 ` Lakshmi Ramasubramanian
2019-11-12 18:12 ` Mimi Zohar
0 siblings, 1 reply; 10+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-12 17:33 UTC (permalink / raw)
To: Mimi Zohar, Patrick Callaghan, linux-integrity; +Cc: linux-kernel, Sascha Hauer
On 11/12/2019 9:14 AM, Mimi Zohar wrote:
> On Mon, 2019-11-11 at 14:29 -0800, Lakshmi Ramasubramanian wrote:
>> On 11/11/19 11:23 AM, Patrick Callaghan wrote:
>>
>>> - if (rbuf_len == 0)
>>> + if (rbuf_len == 0) { /* unexpected EOF */
>>> + rc = -EINVAL;
>>> break;
>>> + }
>>> offset += rbuf_len;
>>
>> Should there be an additional check to validate that (offset + rbuf_len)
>> is less than i_size before calling cypto_shash_update (since rbuf_len is
>> one of the parameters for this call)?
>
> The "while" statement enforces that.
>
> Mimi
Yes - but that check happens after the call to crypto_shash_update().
Perhaps integrity_kernel_read() will never return (rbuf_len) that will
=> violate the check in the "while" statement.
=> number of bytes read that is greater than the memory allocated for
rbuf even in error conditions.
Just making sure.
thanks,
-lakshmi
>
>>
>> if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) {
>> rc = -EINVAL;
>> break;
>> }
>> offset += rbuf_len;
>>
>>> rc = crypto_shash_update(shash, rbuf, rbuf_len);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ima: avoid appraise error for hash calc interrupt
2019-11-12 17:33 ` Lakshmi Ramasubramanian
@ 2019-11-12 18:12 ` Mimi Zohar
2019-11-14 13:55 ` Patrick Callaghan
0 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2019-11-12 18:12 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, Patrick Callaghan, linux-integrity
Cc: linux-kernel, Sascha Hauer
On Tue, 2019-11-12 at 09:33 -0800, Lakshmi Ramasubramanian wrote:
> On 11/12/2019 9:14 AM, Mimi Zohar wrote:
>
> > On Mon, 2019-11-11 at 14:29 -0800, Lakshmi Ramasubramanian wrote:
> >> On 11/11/19 11:23 AM, Patrick Callaghan wrote:
> >>
> >>> - if (rbuf_len == 0)
> >>> + if (rbuf_len == 0) { /* unexpected EOF */
> >>> + rc = -EINVAL;
> >>> break;
> >>> + }
> >>> offset += rbuf_len;
> >>
> >> Should there be an additional check to validate that (offset + rbuf_len)
> >> is less than i_size before calling cypto_shash_update (since rbuf_len is
> >> one of the parameters for this call)?
> >
> > The "while" statement enforces that.
> >
> > Mimi
>
> Yes - but that check happens after the call to crypto_shash_update().
>
> Perhaps integrity_kernel_read() will never return (rbuf_len) that will
> => violate the check in the "while" statement.
> => number of bytes read that is greater than the memory allocated for
> rbuf even in error conditions.
>
> Just making sure.
integrity_kernel_read() returns an error (< 0) or the number of bytes
read. The while statement ensures that there is more data to read, so
returning 0 is always an error.
Mimi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ima: avoid appraise error for hash calc interrupt
2019-11-11 19:23 [PATCH] ima: avoid appraise error for hash calc interrupt Patrick Callaghan
2019-11-11 22:29 ` Lakshmi Ramasubramanian
@ 2019-11-13 7:52 ` Sascha Hauer
1 sibling, 0 replies; 10+ messages in thread
From: Sascha Hauer @ 2019-11-13 7:52 UTC (permalink / raw)
To: Patrick Callaghan; +Cc: linux-integrity, linux-kernel, Mimi Zohar
On Mon, Nov 11, 2019 at 02:23:48PM -0500, Patrick Callaghan wrote:
> The integrity_kernel_read() call in ima_calc_file_hash_tfm() can return
> a value of 0 before all bytes of the file are read. A value of 0 would
> normally indicate an EOF. This has been observed if a user process is
> causing a file appraisal and is terminated with a SIGTERM signal. The
> most common occurrence of seeing the problem is if a shutdown or systemd
> reload is initiated while files are being appraised.
>
> The problem is similar to commit <f5e1040196db> (ima: always return
> negative code for error) that fixed the problem in
> ima_calc_file_hash_atfm().
>
> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Patrick Callaghan <patrickc@linux.ibm.com>
> ---
> 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 73044fc..7967a69 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -362,8 +362,10 @@ static int ima_calc_file_hash_tfm(struct file *file,
> rc = rbuf_len;
> break;
> }
> - if (rbuf_len == 0)
> + if (rbuf_len == 0) { /* unexpected EOF */
> + rc = -EINVAL;
> break;
> + }
There's no point in calling crypto_shash_final() on incomplete data, so
setting rc to an error to avoid that seems the right thing to do to me,
so:
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ima: avoid appraise error for hash calc interrupt
2019-11-12 18:12 ` Mimi Zohar
@ 2019-11-14 13:55 ` Patrick Callaghan
2019-11-14 18:45 ` Lakshmi Ramasubramanian
0 siblings, 1 reply; 10+ messages in thread
From: Patrick Callaghan @ 2019-11-14 13:55 UTC (permalink / raw)
To: Mimi Zohar, Lakshmi Ramasubramanian, Patrick Callaghan, linux-integrity
Cc: linux-kernel, Sascha Hauer
On Tue, 2019-11-12 at 13:12 -0500, Mimi Zohar wrote:
> On Tue, 2019-11-12 at 09:33 -0800, Lakshmi Ramasubramanian wrote:
> > On 11/12/2019 9:14 AM, Mimi Zohar wrote:
> >
> > > On Mon, 2019-11-11 at 14:29 -0800, Lakshmi Ramasubramanian wrote:
> > > > On 11/11/19 11:23 AM, Patrick Callaghan wrote:
> > > >
> > > > > - if (rbuf_len == 0)
> > > > > + if (rbuf_len == 0) { /* unexpected EOF */
> > > > > + rc = -EINVAL;
> > > > > break;
> > > > > + }
> > > > > offset += rbuf_len;
> > > >
> > > > Should there be an additional check to validate that (offset +
> > > > rbuf_len)
> > > > is less than i_size before calling cypto_shash_update (since
> > > > rbuf_len is
> > > > one of the parameters for this call)?
> > >
> > > The "while" statement enforces that.
> > >
> > > Mimi
> >
> > Yes - but that check happens after the call to
> > crypto_shash_update().
> >
> > Perhaps integrity_kernel_read() will never return (rbuf_len) that
> > will
> > => violate the check in the "while" statement.
> > => number of bytes read that is greater than the memory allocated
> > for
> > rbuf even in error conditions.
> >
> > Just making sure.
>
> integrity_kernel_read() returns an error (< 0) or the number of bytes
> read. The while statement ensures that there is more data to read,
> so
> returning 0 is always an error.
>
> Mimi
Hello Laks,
You suggested that the if statement of the patch change to the
following:
if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) {
Unless the file size changed between the time that i_size was set in
ima_calc_file_hash_tfm() and an i_size_read() call was subsequently
issued in a function downstream of the integrity_kernel_read() call,
the rbuf_len returned on the integrity_kernel_read() call will not be
more than i_size - offset. I do not think that it is possible for the
file size to change during this window but nonetheless, if it can, this
would be a different problem and I would not want to include this in my
patch. That said, I do appreciate you taking time to review this patch.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ima: avoid appraise error for hash calc interrupt
2019-11-14 13:55 ` Patrick Callaghan
@ 2019-11-14 18:45 ` Lakshmi Ramasubramanian
2019-11-15 15:25 ` Patrick Callaghan
0 siblings, 1 reply; 10+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-14 18:45 UTC (permalink / raw)
To: Patrick Callaghan, Mimi Zohar, Patrick Callaghan, linux-integrity
Cc: linux-kernel, Sascha Hauer
On 11/14/19 5:55 AM, Patrick Callaghan wrote:
Hi Patrick,
> Hello Laks,
> You suggested that the if statement of the patch change to the
> following:
>
> if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) {
>
> Unless the file size changed between the time that i_size was set in
> ima_calc_file_hash_tfm() and an i_size_read() call was subsequently
> issued in a function downstream of the integrity_kernel_read() call,
> the rbuf_len returned on the integrity_kernel_read() call will not be
> more than i_size - offset. I do not think that it is possible for the
> file size to change during this window but nonetheless, if it can, this
> would be a different problem and I would not want to include this in my
> patch. That said, I do appreciate you taking time to review this patch.
You are right - unless the file size changes between the calls this
problem would not occur. I agree - that issue, even if it can occur,
should be addressed separately.
Another one (again - am not saying this needs to be addressed in this
patch, but just wanted to point out)
rbuf = kzalloc(PAGE_SIZE, GFP_KERNEL);
...
rbuf_len = integrity_kernel_read(file, offset, rbuf, PAGE_SIZE);
...
rc = crypto_shash_update(shash, rbuf, rbuf_len);
rbuf is of size PAGE_SIZE, but rbuf_len, returned by
integrity_kernel_read() is passed as buffer size to
crypto_shash_update() without any validation (rbuf_len <= PAGE_SIZE)
It is assumed here that integrity_kernel_read() would not return a
length greater than rbuf size and hence crypto_shash_update() would
never access beyond the given buffer.
thanks,
-lakshmi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ima: avoid appraise error for hash calc interrupt
2019-11-14 18:45 ` Lakshmi Ramasubramanian
@ 2019-11-15 15:25 ` Patrick Callaghan
2019-11-15 20:34 ` Lakshmi Ramasubramanian
0 siblings, 1 reply; 10+ messages in thread
From: Patrick Callaghan @ 2019-11-15 15:25 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, Mimi Zohar, Patrick Callaghan, linux-integrity
Cc: linux-kernel, Sascha Hauer
On Thu, 2019-11-14 at 10:45 -0800, Lakshmi Ramasubramanian wrote:
> On 11/14/19 5:55 AM, Patrick Callaghan wrote:
>
> Hi Patrick,
>
> > Hello Laks,
> > You suggested that the if statement of the patch change to the
> > following:
> >
> > if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) {
> >
> > Unless the file size changed between the time that i_size was set
> > in
> > ima_calc_file_hash_tfm() and an i_size_read() call was subsequently
> > issued in a function downstream of the integrity_kernel_read()
> > call,
> > the rbuf_len returned on the integrity_kernel_read() call will not
> > be
> > more than i_size - offset. I do not think that it is possible for
> > the
> > file size to change during this window but nonetheless, if it can,
> > this
> > would be a different problem and I would not want to include this
> > in my
> > patch. That said, I do appreciate you taking time to review this
> > patch.
>
> You are right - unless the file size changes between the calls this
> problem would not occur. I agree - that issue, even if it can occur,
> should be addressed separately.
>
> Another one (again - am not saying this needs to be addressed in
> this
> patch, but just wanted to point out)
>
> rbuf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> ...
> rbuf_len = integrity_kernel_read(file, offset, rbuf,
> PAGE_SIZE);
> ...
> rc = crypto_shash_update(shash, rbuf, rbuf_len);
>
> rbuf is of size PAGE_SIZE, but rbuf_len, returned by
> integrity_kernel_read() is passed as buffer size to
> crypto_shash_update() without any validation (rbuf_len <= PAGE_SIZE)
>
> It is assumed here that integrity_kernel_read() would not return a
> length greater than rbuf size and hence crypto_shash_update() would
> never access beyond the given buffer.
>
> thanks,
> -lakshmi
>
>
Hello Laks,
Agreed. The assumption is that integrity_kernel_read() function does
not return a value greater than the fourth parameter passed to it (i.e.
does not read more bytes from the file than the size of the buffer
passed to it). I tried to validate that this assumption was true by
following the code but felt I could not prove it with my current
knowledge of the code. If this assumption is not true then I believe
that any code change for this problem should go into a different
patch.
Thank you.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ima: avoid appraise error for hash calc interrupt
2019-11-15 15:25 ` Patrick Callaghan
@ 2019-11-15 20:34 ` Lakshmi Ramasubramanian
0 siblings, 0 replies; 10+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-15 20:34 UTC (permalink / raw)
To: Patrick Callaghan, Mimi Zohar, Patrick Callaghan, linux-integrity
Cc: linux-kernel, Sascha Hauer
On 11/15/19 7:25 AM, Patrick Callaghan wrote:
> Hello Laks,
> Agreed. The assumption is that integrity_kernel_read() function does
> not return a value greater than the fourth parameter passed to it (i.e.
> does not read more bytes from the file than the size of the buffer
> passed to it). I tried to validate that this assumption was true by
> following the code but felt I could not prove it with my current
> knowledge of the code. If this assumption is not true then I believe
> that any code change for this problem should go into a different
> patch.
I agree Patrick - not a blocker for this patch set.
thanks,
-lakshmi
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-11-15 20:35 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-11 19:23 [PATCH] ima: avoid appraise error for hash calc interrupt Patrick Callaghan
2019-11-11 22:29 ` Lakshmi Ramasubramanian
2019-11-12 17:14 ` Mimi Zohar
2019-11-12 17:33 ` Lakshmi Ramasubramanian
2019-11-12 18:12 ` Mimi Zohar
2019-11-14 13:55 ` Patrick Callaghan
2019-11-14 18:45 ` Lakshmi Ramasubramanian
2019-11-15 15:25 ` Patrick Callaghan
2019-11-15 20:34 ` Lakshmi Ramasubramanian
2019-11-13 7:52 ` Sascha Hauer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.