Received-SPF: pass (google.com: domain of linux-kernel+bounces-122039-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <80b2e479bc0c520423885a16dd46e0201a1c9418.camel@linux.ibm.com>
Subject: Re: [PATCH] ima: define an init_module critical data record
From: Mimi Zohar <zohar@linux.ibm.com>
To: Jarkko Sakkinen <jarkko@kernel.org>, Luis Chamberlain
 <mcgrof@kernel.org>
Cc: linux-modules@vger.kernel.org, linux-integrity@vger.kernel.org,
        Roberto
	Sassu <roberto.sassu@huawei.com>,
        linux-kernel@vger.kernel.org, Ken Goldman
	 <kgold@linux.ibm.com>
In-Reply-To: <D04OU424128P.22TP02GW2CJCT@kernel.org>
References: <20240327150019.81477-1-zohar@linux.ibm.com>
	 <D04OU424128P.22TP02GW2CJCT@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Date: Wed, 27 Mar 2024 17:37:57 -0400
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0

On Wed, 2024-03-27 at 18:54 +0200, Jarkko Sakkinen wrote:
> On Wed Mar 27, 2024 at 5:00 PM EET, Mimi Zohar wrote:
> > The init_module syscall loads an ELF image into kernel space without
> > measuring the buffer containing the ELF image.  To close this kernel
> > module integrity gap, define a new critical-data record which includes
> > the hash of the ELF image.
> > 
> > Instead of including the buffer data in the IMA measurement list,
> > include the hash of the buffer data to avoid large IMA measurement
> > list records.  The buffer data hash would be the same value as the
> > finit_module syscall file hash.
> > 
> > To enable measuring the init_module buffer and other critical data from
> > boot, define "ima_policy=critical_data" on the boot command line.  Since
> > builtin policies are not persistent, a custom IMA policy must include
> > the rule as well: measure func=CRITICAL_DATA label=modules
> > 
> > To verify the template data hash value, first convert the buffer data
> > hash to binary:
> > grep "init_module" \
> > 	/sys/kernel/security/integrity/ima/ascii_runtime_measurements | \
> > 	tail -1 | cut -d' ' -f 6 | xxd -r -p | sha256sum
> > 
> > Reported-by: Ken Goldman <kgold@linux.ibm.com>
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >  security/integrity/ima/ima_main.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/security/integrity/ima/ima_main.c
> > b/security/integrity/ima/ima_main.c
> > index c84e8c55333d..4b4348d681a6 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -902,6 +902,13 @@ static int ima_post_load_data(char *buf, loff_t size,
> >  		return 0;
> >  	}
> >  
> > +	/*
> > +	 * Measure the init_module syscall buffer containing the ELF image.
> > +	 */
> > +	if (load_id == LOADING_MODULE)
> > +		ima_measure_critical_data("modules", "init_module",
> > +					  buf, size, true, NULL, 0);
> 
> No reason not to ack but could be just as well (passing checkpatch):

Please review the tag usage as defined in 
https://docs.kernel.org/process/submitting-patches.html.

> 
> 	if (load_id == LOADING_MODULE)
> 		ima_measure_critical_data("modules", "init_module", buf, size,
> true, NULL, 0);
> 
> < 100 characters

From what I understand, it's still preferable to stay under the 80 character
limit, but checkpatch.pl will not complain.  From 
https://www.kernel.org/doc/Documentation/process/maintainer-tip.rst:

"The 80 character rule is not a strict rule, so please use common sense when
breaking lines. Especially format strings should never be broken up."

> 
> > +
> >  	return 0;
> >  }
> >  
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Thanks!

Mimi