[BUG] 3ce1217d6cd5 ima patch causes s390 to crash on boot

[BUG] 3ce1217d6cd5 ima patch causes s390 to crash on boot

[Heiko Carstens]

Hi Roberto,

your patch 3ce1217d6cd5 "ima: define template fields library and new helpers"
causes s390 to crash on boot:

 IMA: No TPM chip found, activating TPM-bypass!
 Unable to handle kernel pointer dereference at virtual kernel address 0000000000842000
 Oops: 0004 [#1] SMP
 Modules linked in:
 CPU: 3 PID: 1 Comm: swapper/0 Not tainted 3.12.0-rc2-00098-g3ce1217d6cd5 #17
 task: 000000003ffa0000 ti: 000000003ff84000 task.ti: 000000003ff84000
 Krnl PSW : 0704e00180000000 000000000044bf88 (strsep+0x7c/0xa0)
            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 EA:3
 Krnl GPRS: 000000000000007c 000000000000007c 000000003ff87d90 0000000000821fd8
            0000000000000000 000000000000007c 0000000000aa37e0 0000000000aa9008
            0000000000000051 0000000000a114d8 0000000100000002 0000000000842bde
            0000000000842bdf 00000000006f97f0 000000000040062c 000000003ff87cf0
 Krnl Code: 000000000044bf7c: a7f4000a           brc     15,44bf90
            000000000044bf80: b90200cc           ltgr    %r12,%r12
           #000000000044bf84: a7840006           brc     8,44bf90
           >000000000044bf88: 9200c000           mvi     0(%r12),0
            000000000044bf8c: 41c0c001           la      %r12,1(%r12)
            000000000044bf90: e3c020000024       stg     %r12,0(%r2)
            000000000044bf96: b904002b           lgr     %r2,%r11
            000000000044bf9a: ebbcf0700004       lmg     %r11,%r12,112(%r15)
 Call Trace:
 ([<00000000004005fe>] ima_init_template+0xa2/0x1bc)
  [<0000000000a7c896>] ima_init+0x7a/0xa8
  [<0000000000a7c938>] init_ima+0x24/0x40
  [<00000000001000e8>] do_one_initcall+0x68/0x128
  [<0000000000a4eb56>] kernel_init_freeable+0x20a/0x2b4
  [<00000000006a1ff4>] kernel_init+0x30/0x178
  [<00000000006b69fe>] kernel_thread_starter+0x6/0xc
  [<00000000006b69f8>] kernel_thread_starter+0x0/0xc
 Last Breaking-Event-Address:
  [<000000000044bf42>] strsep+0x36/0xa0

Re: [BUG] 3ce1217d6cd5 ima patch causes s390 to crash on boot

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 2287 bytes --]

On 11/22/2013 12:48 PM, Heiko Carstens wrote:
> Hi Roberto,
>
> your patch 3ce1217d6cd5 "ima: define template fields library and new helpers"
> causes s390 to crash on boot:
>

Hi Heiko

thanks for the information. I think this issue is related to the error
detected by the kbuild test robot. Please, try to apply the attached
patch to see if it solves the problem.

Thanks

Roberto


>   IMA: No TPM chip found, activating TPM-bypass!
>   Unable to handle kernel pointer dereference at virtual kernel address 0000000000842000
>   Oops: 0004 [#1] SMP
>   Modules linked in:
>   CPU: 3 PID: 1 Comm: swapper/0 Not tainted 3.12.0-rc2-00098-g3ce1217d6cd5 #17
>   task: 000000003ffa0000 ti: 000000003ff84000 task.ti: 000000003ff84000
>   Krnl PSW : 0704e00180000000 000000000044bf88 (strsep+0x7c/0xa0)
>              R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 EA:3
>   Krnl GPRS: 000000000000007c 000000000000007c 000000003ff87d90 0000000000821fd8
>              0000000000000000 000000000000007c 0000000000aa37e0 0000000000aa9008
>              0000000000000051 0000000000a114d8 0000000100000002 0000000000842bde
>              0000000000842bdf 00000000006f97f0 000000000040062c 000000003ff87cf0
>   Krnl Code: 000000000044bf7c: a7f4000a           brc     15,44bf90
>              000000000044bf80: b90200cc           ltgr    %r12,%r12
>             #000000000044bf84: a7840006           brc     8,44bf90
>             >000000000044bf88: 9200c000           mvi     0(%r12),0
>              000000000044bf8c: 41c0c001           la      %r12,1(%r12)
>              000000000044bf90: e3c020000024       stg     %r12,0(%r2)
>              000000000044bf96: b904002b           lgr     %r2,%r11
>              000000000044bf9a: ebbcf0700004       lmg     %r11,%r12,112(%r15)
>   Call Trace:
>   ([<00000000004005fe>] ima_init_template+0xa2/0x1bc)
>    [<0000000000a7c896>] ima_init+0x7a/0xa8
>    [<0000000000a7c938>] init_ima+0x24/0x40
>    [<00000000001000e8>] do_one_initcall+0x68/0x128
>    [<0000000000a4eb56>] kernel_init_freeable+0x20a/0x2b4
>    [<00000000006a1ff4>] kernel_init+0x30/0x178
>    [<00000000006b69fe>] kernel_thread_starter+0x6/0xc
>    [<00000000006b69f8>] kernel_thread_starter+0x0/0xc
>   Last Breaking-Event-Address:
>    [<000000000044bf42>] strsep+0x36/0xa0
>


[-- Attachment #2: noderef-ima_template.patch --]
[-- Type: text/x-diff, Size: 1101 bytes --]

From: Fengguang Wu <fengguang.wu@intel.com>
Subject: [PATCH] ima: fix coccinelle warnings
TO: Mimi Zohar <zohar@linux.vnet.ibm.com>
CC: Roberto Sassu <roberto.sassu@polito.it>
CC: linux-kernel@vger.kernel.org 

security/integrity/ima/ima_template.c:62:41-47: ERROR: application of sizeof to pointer

 sizeof when applied to a pointer typed expression gives the size of
 the pointer

Generated by: coccinelle/misc/noderef.cocci

CC: Roberto Sassu <roberto.sassu@polito.it>
CC: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 cocci-output-13142-271b5e-ima_template.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -59,7 +59,7 @@ static int template_desc_init_fields(cha
 	if (template_num_fields > IMA_TEMPLATE_NUM_FIELDS_MAX)
 		return -EINVAL;
 
-	*fields = kzalloc(template_num_fields * sizeof(*fields), GFP_KERNEL);
+	*fields = kzalloc(template_num_fields * sizeof(**fields), GFP_KERNEL);
 	if (*fields == NULL) {
 		result = -ENOMEM;
 		goto out;

Re: [BUG] 3ce1217d6cd5 ima patch causes s390 to crash on boot

[Heiko Carstens]

On Fri, Nov 22, 2013 at 02:28:33PM +0100, Roberto Sassu wrote:
> On 11/22/2013 12:48 PM, Heiko Carstens wrote:
> >Hi Roberto,
> >
> >your patch 3ce1217d6cd5 "ima: define template fields library and new helpers"
> >causes s390 to crash on boot:
> >
> 
> Hi Heiko
> 
> thanks for the information. I think this issue is related to the error
> detected by the kbuild test robot. Please, try to apply the attached
> patch to see if it solves the problem.

No, the patch doesn't fix the problem.

> From: Fengguang Wu <fengguang.wu@intel.com>
> Subject: [PATCH] ima: fix coccinelle warnings
> TO: Mimi Zohar <zohar@linux.vnet.ibm.com>
> CC: Roberto Sassu <roberto.sassu@polito.it>
> CC: linux-kernel@vger.kernel.org 
> 
> security/integrity/ima/ima_template.c:62:41-47: ERROR: application of sizeof to pointer
> 
>  sizeof when applied to a pointer typed expression gives the size of
>  the pointer
> 
> Generated by: coccinelle/misc/noderef.cocci
> 
> CC: Roberto Sassu <roberto.sassu@polito.it>
> CC: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> ---
> 
>  cocci-output-13142-271b5e-ima_template.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/security/integrity/ima/ima_template.c
> +++ b/security/integrity/ima/ima_template.c
> @@ -59,7 +59,7 @@ static int template_desc_init_fields(cha
>  	if (template_num_fields > IMA_TEMPLATE_NUM_FIELDS_MAX)
>  		return -EINVAL;
> 
> -	*fields = kzalloc(template_num_fields * sizeof(*fields), GFP_KERNEL);
> +	*fields = kzalloc(template_num_fields * sizeof(**fields), GFP_KERNEL);
>  	if (*fields == NULL) {
>  		result = -ENOMEM;
>  		goto out;

Re: [BUG] 3ce1217d6cd5 ima patch causes s390 to crash on boot

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 2413 bytes --]

On 11/22/2013 03:13 PM, Heiko Carstens wrote:
> On Fri, Nov 22, 2013 at 02:28:33PM +0100, Roberto Sassu wrote:
>> On 11/22/2013 12:48 PM, Heiko Carstens wrote:
>>> Hi Roberto,
>>>
>>> your patch 3ce1217d6cd5 "ima: define template fields library and new helpers"
>>> causes s390 to crash on boot:
>>>
>>
>> Hi Heiko
>>
>> thanks for the information. I think this issue is related to the error
>> detected by the kbuild test robot. Please, try to apply the attached
>> patch to see if it solves the problem.
>
> No, the patch doesn't fix the problem.

Ok, sorry for the delay. I was involved in another task.

The previous patch is not correct, as I allocate an
array of pointers, not structures. You can discard it.

Another problem that I found is that strsep()
modifies the source buffer by replacing the separator
character with '\0'. In particular, this function
modifies static data initialized at the beginning
of the ima_template.c file. Maybe, this is causing
the kernel panic. I already sent a patch to fix this
problem (attached to the email) even if it is not
supposed to land on the 3.13 kernel. Let me know
if this fixes the issue. Otherwise, I will check
the code more in depth.

Thanks

Roberto


>
>> From: Fengguang Wu <fengguang.wu@intel.com>
>> Subject: [PATCH] ima: fix coccinelle warnings
>> TO: Mimi Zohar <zohar@linux.vnet.ibm.com>
>> CC: Roberto Sassu <roberto.sassu@polito.it>
>> CC: linux-kernel@vger.kernel.org
>>
>> security/integrity/ima/ima_template.c:62:41-47: ERROR: application of sizeof to pointer
>>
>>   sizeof when applied to a pointer typed expression gives the size of
>>   the pointer
>>
>> Generated by: coccinelle/misc/noderef.cocci
>>
>> CC: Roberto Sassu <roberto.sassu@polito.it>
>> CC: Mimi Zohar <zohar@linux.vnet.ibm.com>
>> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
>> ---
>>
>>   cocci-output-13142-271b5e-ima_template.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> --- a/security/integrity/ima/ima_template.c
>> +++ b/security/integrity/ima/ima_template.c
>> @@ -59,7 +59,7 @@ static int template_desc_init_fields(cha
>>   	if (template_num_fields > IMA_TEMPLATE_NUM_FIELDS_MAX)
>>   		return -EINVAL;
>>
>> -	*fields = kzalloc(template_num_fields * sizeof(*fields), GFP_KERNEL);
>> +	*fields = kzalloc(template_num_fields * sizeof(**fields), GFP_KERNEL);
>>   	if (*fields == NULL) {
>>   		result = -ENOMEM;
>>   		goto out;
>


[-- Attachment #2: 0002-ima-make-a-copy-of-template_fmt-in-template_desc_ini.patch --]
[-- Type: text/x-diff, Size: 1768 bytes --]

>From 2d3aa1c0328c44ecc3af7de162791c8cddfb6dfd Mon Sep 17 00:00:00 2001
From: Roberto Sassu <roberto.sassu@polito.it>
Date: Wed, 6 Nov 2013 13:51:35 +0100
Subject: [RFC][PATCH 2/4] ima: make a copy of template_fmt in
 template_desc_init_fields()

This patch makes a copy of the 'template_fmt' function argument so that
the latter will not be modified by strsep(), which does the splitting by
replacing the given separator with '\0'.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
---
 security/integrity/ima/ima_template.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 7bcff5c..bb33576 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -113,13 +113,19 @@ static int template_desc_init_fields(char *template_fmt,
 				     struct ima_template_field ***fields,
 				     int *num_fields)
 {
-	char *c, *template_fmt_ptr = template_fmt;
+	char *c, *template_fmt_ptr, *template_fmt_copy = NULL;
 	int template_num_fields = template_fmt_size(template_fmt);
 	int i, result = 0;
 
 	if (template_num_fields > IMA_TEMPLATE_NUM_FIELDS_MAX)
 		return -EINVAL;
 
+	/* copying is needed as strsep() modifies the original buffer */
+	template_fmt_copy = kstrdup(template_fmt, GFP_KERNEL);
+	if (template_fmt_copy == NULL)
+		return -ENOMEM;
+
+	template_fmt_ptr = template_fmt_copy;
 	*fields = kzalloc(template_num_fields * sizeof(*fields), GFP_KERNEL);
 	if (*fields == NULL) {
 		result = -ENOMEM;
@@ -139,6 +145,7 @@ static int template_desc_init_fields(char *template_fmt,
 	*num_fields = i;
 	return 0;
 out:
+	kfree(template_fmt_copy);
 	kfree(*fields);
 	*fields = NULL;
 	return result;
-- 
1.8.1.4

Re: [BUG] 3ce1217d6cd5 ima patch causes s390 to crash on boot

[Heiko Carstens]

On Fri, Nov 22, 2013 at 05:23:49PM +0100, Roberto Sassu wrote:
> Another problem that I found is that strsep()
> modifies the source buffer by replacing the separator
> character with '\0'. In particular, this function
> modifies static data initialized at the beginning
> of the ima_template.c file. Maybe, this is causing
> the kernel panic. I already sent a patch to fix this
> problem (attached to the email) even if it is not
> supposed to land on the 3.13 kernel. Let me know
> if this fixes the issue. Otherwise, I will check
> the code more in depth.

With your patch below applied the kernel boots again.
So it should go into 3.13 (or a different fix).

Thanks!

> From 2d3aa1c0328c44ecc3af7de162791c8cddfb6dfd Mon Sep 17 00:00:00 2001
> From: Roberto Sassu <roberto.sassu@polito.it>
> Date: Wed, 6 Nov 2013 13:51:35 +0100
> Subject: [RFC][PATCH 2/4] ima: make a copy of template_fmt in
>  template_desc_init_fields()
> 
> This patch makes a copy of the 'template_fmt' function argument so that
> the latter will not be modified by strsep(), which does the splitting by
> replacing the given separator with '\0'.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
> ---
>  security/integrity/ima/ima_template.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
> index 7bcff5c..bb33576 100644
> --- a/security/integrity/ima/ima_template.c
> +++ b/security/integrity/ima/ima_template.c
> @@ -113,13 +113,19 @@ static int template_desc_init_fields(char *template_fmt,
>  				     struct ima_template_field ***fields,
>  				     int *num_fields)
>  {
> -	char *c, *template_fmt_ptr = template_fmt;
> +	char *c, *template_fmt_ptr, *template_fmt_copy = NULL;
>  	int template_num_fields = template_fmt_size(template_fmt);
>  	int i, result = 0;
> 
>  	if (template_num_fields > IMA_TEMPLATE_NUM_FIELDS_MAX)
>  		return -EINVAL;
> 
> +	/* copying is needed as strsep() modifies the original buffer */
> +	template_fmt_copy = kstrdup(template_fmt, GFP_KERNEL);
> +	if (template_fmt_copy == NULL)
> +		return -ENOMEM;
> +
> +	template_fmt_ptr = template_fmt_copy;
>  	*fields = kzalloc(template_num_fields * sizeof(*fields), GFP_KERNEL);
>  	if (*fields == NULL) {
>  		result = -ENOMEM;
> @@ -139,6 +145,7 @@ static int template_desc_init_fields(char *template_fmt,
>  	*num_fields = i;
>  	return 0;
>  out:
> +	kfree(template_fmt_copy);
>  	kfree(*fields);
>  	*fields = NULL;
>  	return result;
> -- 
> 1.8.1.4
> 

Re: [BUG] 3ce1217d6cd5 ima patch causes s390 to crash on boot

[Roberto Sassu]

On 11/22/2013 05:35 PM, Heiko Carstens wrote:
> On Fri, Nov 22, 2013 at 05:23:49PM +0100, Roberto Sassu wrote:
>> Another problem that I found is that strsep()
>> modifies the source buffer by replacing the separator
>> character with '\0'. In particular, this function
>> modifies static data initialized at the beginning
>> of the ima_template.c file. Maybe, this is causing
>> the kernel panic. I already sent a patch to fix this
>> problem (attached to the email) even if it is not
>> supposed to land on the 3.13 kernel. Let me know
>> if this fixes the issue. Otherwise, I will check
>> the code more in depth.
>
> With your patch below applied the kernel boots again.
> So it should go into 3.13 (or a different fix).
>

Ok, good. Yes, we will apply this patch to the 3.13 kernel.


> Thanks!

No problem.

Roberto


>
>>  From 2d3aa1c0328c44ecc3af7de162791c8cddfb6dfd Mon Sep 17 00:00:00 2001
>> From: Roberto Sassu <roberto.sassu@polito.it>
>> Date: Wed, 6 Nov 2013 13:51:35 +0100
>> Subject: [RFC][PATCH 2/4] ima: make a copy of template_fmt in
>>   template_desc_init_fields()
>>
>> This patch makes a copy of the 'template_fmt' function argument so that
>> the latter will not be modified by strsep(), which does the splitting by
>> replacing the given separator with '\0'.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
>> ---
>>   security/integrity/ima/ima_template.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
>> index 7bcff5c..bb33576 100644
>> --- a/security/integrity/ima/ima_template.c
>> +++ b/security/integrity/ima/ima_template.c
>> @@ -113,13 +113,19 @@ static int template_desc_init_fields(char *template_fmt,
>>   				     struct ima_template_field ***fields,
>>   				     int *num_fields)
>>   {
>> -	char *c, *template_fmt_ptr = template_fmt;
>> +	char *c, *template_fmt_ptr, *template_fmt_copy = NULL;
>>   	int template_num_fields = template_fmt_size(template_fmt);
>>   	int i, result = 0;
>>
>>   	if (template_num_fields > IMA_TEMPLATE_NUM_FIELDS_MAX)
>>   		return -EINVAL;
>>
>> +	/* copying is needed as strsep() modifies the original buffer */
>> +	template_fmt_copy = kstrdup(template_fmt, GFP_KERNEL);
>> +	if (template_fmt_copy == NULL)
>> +		return -ENOMEM;
>> +
>> +	template_fmt_ptr = template_fmt_copy;
>>   	*fields = kzalloc(template_num_fields * sizeof(*fields), GFP_KERNEL);
>>   	if (*fields == NULL) {
>>   		result = -ENOMEM;
>> @@ -139,6 +145,7 @@ static int template_desc_init_fields(char *template_fmt,
>>   	*num_fields = i;
>>   	return 0;
>>   out:
>> +	kfree(template_fmt_copy);
>>   	kfree(*fields);
>>   	*fields = NULL;
>>   	return result;
>> --
>> 1.8.1.4
>>
>

Re: [BUG] 3ce1217d6cd5 ima patch causes s390 to crash on boot

[Mimi Zohar]

On Fri, 2013-11-22 at 17:35 +0100, Heiko Carstens wrote:
> On Fri, Nov 22, 2013 at 05:23:49PM +0100, Roberto Sassu wrote:
> > Another problem that I found is that strsep()
> > modifies the source buffer by replacing the separator
> > character with '\0'. In particular, this function
> > modifies static data initialized at the beginning
> > of the ima_template.c file. Maybe, this is causing
> > the kernel panic. I already sent a patch to fix this
> > problem (attached to the email) even if it is not
> > supposed to land on the 3.13 kernel. Let me know
> > if this fixes the issue. Otherwise, I will check
> > the code more in depth.
> 
> With your patch below applied the kernel boots again.
> So it should go into 3.13 (or a different fix).
> 
> Thanks!

Thanks for testing!  There are a number of bug fixes for this release in
linux-integrity/ima-fixes-new.  Now that the security patches made it
into Linus' tree, I'll send the pull request to James.

thanks,

Mimi