[PATCH v0] IMA: Check IMA policy flag

[PATCH v0] IMA: Check IMA policy flag

[Lakshmi Ramasubramanian]

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
(eric.snowberg@oracle.com) when testing key measurement:
https://lore.kernel.org/linux-integrity/20191118223818.3353-1-nramas@linux.microsoft.com/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 <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 60027c643ecd..c9374430bb72 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -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))
-- 
2.17.1

Re: [PATCH v0] IMA: Check IMA policy flag

[Lakshmi Ramasubramanian]

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

Hi Mimi,

> 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 <nramas@linux.microsoft.com>
> ---
>   security/integrity/ima/ima_main.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 60027c643ecd..c9374430bb72 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -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

Re: [PATCH v0] IMA: Check IMA policy flag

[Mimi Zohar]

On Mon, 2019-11-25 at 10:23 -0800, Lakshmi Ramasubramanian wrote:
> On 11/21/19 9:14 AM, Lakshmi Ramasubramanian wrote:
> 
> Hi Mimi,
> 
> > 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 <nramas@linux.microsoft.com>
> > ---
> >   security/integrity/ima/ima_main.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 60027c643ecd..c9374430bb72 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -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?

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

Re: [PATCH v0] IMA: Check IMA policy flag

[Lakshmi Ramasubramanian]

On 11/25/19 10:30 AM, Mimi Zohar wrote:

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

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