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

[Roberto Sassu <roberto.sassu@polito.it>]

[-- 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