IMA: Check IMA policy flag

PATCH In review Lakshmi Ramasubramanian 2 participants 4 messages Nov 21, 2019 – Nov 27, 2019

Subsystems (4)

Security (LSM) Keys-trusted Integrity measurement architecture (IMA) Extended Verification Module (EVM)

lore

Share & tools

lore

Lakshmi Ramasubramanian

author

2019-11-21 17:14 UTC #0 lore raw

PATCH 1/1 IMA: Check IMA policy flag

process_buffer_measurement() needs to check if ima_policy_flag is set to measure and\or appraise. Not doing this check can result in kernel panic (such as when process_buffer_measurement() is called before IMA is initialized).

Please see one instance reported by Eric Snowberg ([email protected]) when testing key measurement:
https://lore.kernel.org/linux-integrity/[email protected]/T/#mc243b065659d43a7f5070a2383979542f10d58d1

This change adds the check in process_buffer_measurement() to return immediately if ima_policy_flag is set to 0.

[    1.185105] Loading compiled-in X.509 certificates
[    1.190240] BUG: kernel NULL pointer dereference, address: 0000000000000000
[    1.191835] #PF: supervisor read access in kernel mode
[    1.193041] #PF: error_code(0x0000) - not-present page
[    1.222749] Call Trace:
[    1.223344]  ? crypto_destroy_tfm+0x5f/0xb0
[    1.224315]  ima_get_action+0x2c/0x30
[    1.225148]  process_buffer_measurement+0x1da/0x230
[    1.226306]  ima_post_key_create_or_update+0x3b/0x40
[    1.227459]  key_create_or_update+0x371/0x5c0
[    1.228494]  load_system_certificate_list+0x99/0xfa
[    1.229588]  ? system_trusted_keyring_init+0xfb/0xfb
[    1.230705]  ? do_early_param+0x95/0x95
[    1.231574]  do_one_initcall+0x4a/0x1fa
[    1.232444]  ? do_early_param+0x95/0x95
[    1.233313]  kernel_init_freeable+0x1c2/0x267
[    1.234300]  ? rest_init+0xb0/0xb0
[    1.235075]  kernel_init+0xe/0x110
[    1.235842]  ret_from_fork+0x24/0x50

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

---
 security/integrity/ima/ima_main.c | 3 +++
 1 file changed, 3 insertions(+)

security/integrity/ima/ima_main.c +3 -0

index 60027c643ecd..c9374430bb72 100644
@@ -651,6 +651,9 @@ static void process_buffer_measurement(const void *buf, int size,
 	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
 	int action = 0;
 
+	if (!ima_policy_flag)
+		return;
+
 	action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
 				&template_desc);
 	if (!(action & IMA_MEASURE))

Signature · 1 line

2.17.1

Lakshmi Ramasubramanian

author

2019-11-25 18:23 UTC #1 lore raw

On 11/21/19 9:14 AM, Lakshmi Ramasubramanian wrote:

Hi Mimi,

Quoted 18 lines

process_buffer_measurement() needs to check if ima_policy_flag is set to measure and\or appraise. Not doing this check can result in kernel panic (such as when process_buffer_measurement() is called before IMA is initialized).
This change adds the check in process_buffer_measurement() to return immediately if ima_policy_flag is set to 0.
Signed-off-by Lakshmi Ramasubramanian <[email protected]>
---
  security/integrity/ima/ima_main.c | 3 +++
  1 file changed, 3 insertions(+)
security/integrity/ima/ima_main.c +0 -0
index 60027c643ecd..c9374430bb72 100644

security/integrity/ima/ima_main.c static void process_buffer_measurement(const void *buf, int size, · line 656

Quoted 6 lines

security/integrity/ima/ima_main.c +3 -0

@@ -651,6 +651,9 @@ static void process_buffer_measurement(const void *buf, int size,
  	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
  	int action = 0;
 
+	if (!ima_policy_flag)
+		return;
+

Please let me know if the above change would be accepted as a standalone patch (like the one in this patch), or, I should include this change as one of the patches in the "Key Measurement" patch set?

thanks,
-lakshmi

Mimi Zohar

maintainer

2019-11-25 18:30 UTC #2 lore raw

Quoted 25 lines · Lakshmi Ramasubramanian

On 11/21/19 9:14 AM, Lakshmi Ramasubramanian wrote:
Hi Mimi,
Quoted 21 lines
process_buffer_measurement() needs to check if ima_policy_flag is set to measure and\or appraise. Not doing this check can result in kernel panic (such as when process_buffer_measurement() is called before IMA is initialized).
This change adds the check in process_buffer_measurement() to return immediately if ima_policy_flag is set to 0.
Signed-off-by Lakshmi Ramasubramanian <[email protected]>
---
  security/integrity/ima/ima_main.c | 3 +++
  1 file changed, 3 insertions(+)
security/integrity/ima/ima_main.c +0 -0
index 60027c643ecd..c9374430bb72 100644
@@ -651,6 +651,9 @@ static void process_buffer_measurement(const void *buf, int size,
  	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
  	int action = 0;
 

Quoted 8 lines

security/integrity/ima/ima_main.c static void process_buffer_measurement(const void *buf, int size, · line 656
Quoted 3 lines
security/integrity/ima/ima_main.c +3 -0
+	if (!ima_policy_flag)
+		return;
+
Please let me know if the above change would be accepted as a standalone patch (like the one in this patch),
or, I should include this change as one of the patches in the "Key Measurement" patch set?

As I'm not planning on sending a pull request this open window, so that it doesn't get lost/forgotten, please include it as the first patch in this patch set.

Mimi

Lakshmi Ramasubramanian

author

2019-11-27 02:09 UTC #3 lore raw

Quoted 6 lines · Mimi Zohar

Quoted 5 lines
Please let me know if the above change would be accepted as a standalone patch (like the one in this patch),
or, I should include this change as one of the patches in the "Key Measurement" patch set?

Quoted 6 lines

As I'm not planning on sending a pull request this open window, so that it doesn't get lost/forgotten, please include it as the first patch in this patch set.
Mimi

I have included the change to check ima_policy_flag in process_buffer_measurement() as the 1st patch (PATCH v9 1/6) in the updated patch set I posted today.

thanks,
-lakshmi