Received-SPF: pass (google.com: domain of linux-kernel+bounces-121654-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Wed, 27 Mar 2024 18:54:16 +0200
Message-Id: <D04OU424128P.22TP02GW2CJCT@kernel.org>
To: "Mimi Zohar" <zohar@linux.ibm.com>, "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>
Subject: Re: [PATCH] ima: define an init_module critical data record
From: "Jarkko Sakkinen" <jarkko@kernel.org>
References: <20240327150019.81477-1-zohar@linux.ibm.com>
In-Reply-To: <20240327150019.81477-1-zohar@linux.ibm.com>

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=3Dcritical_data" on the boot command line.  Sinc=
e
> builtin policies are not persistent, a custom IMA policy must include
> the rule as well: measure func=3DCRITICAL_DATA label=3Dmodules
>
> 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/i=
ma_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;
>  	}
> =20
> +	/*
> +	 * Measure the init_module syscall buffer containing the ELF image.
> +	 */
> +	if (load_id =3D=3D 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):

	if (load_id =3D=3D LOADING_MODULE)
		ima_measure_critical_data("modules", "init_module", buf, size, true, NULL=
, 0);

< 100 characters

> +
>  	return 0;
>  }
> =20

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko