* [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-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
* 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
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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).