[PATCH] IMA: Updated KBUILD_MODNAME for IMA files to ima

[PATCH] IMA: Updated KBUILD_MODNAME for IMA files to ima

[Tushar Sugandhi]

Log statements from ima_mok.c, ima_asymmetric_keys.c, and
ima_queue_keys.c are prefixed with the respective file names
and not with the string "ima". 

This change fixes the log statement prefix to be consistent with the rest
of the IMA files.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 064a256f8725..67dabca670e2 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -11,6 +11,6 @@ ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
 ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
 ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
 ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
-obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
-obj-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
-obj-$(CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS) += ima_queue_keys.o
+ima-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
+ima-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
+ima-$(CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS) += ima_queue_keys.o
-- 
2.17.1

[PATCH] IMA: Add log statements for failure conditions.

[Tushar Sugandhi]

process_buffer_measurement() and ima_alloc_key_entry()
functions do not have log messages for failure conditions.

This change adds log statements in the above functions. 

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima_main.c       | 4 ++++
 security/integrity/ima/ima_queue_keys.c | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 9fe949c6a530..afab796fb765 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -757,6 +757,10 @@ void process_buffer_measurement(const void *buf, int size,
 		ima_free_template_entry(entry);
 
 out:
+	if (ret < 0)
+		pr_err("Process buffer measurement failed, result: %d\n",
+			ret);
+
 	return;
 }
 
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index c87c72299191..2cc52f17ea81 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -90,6 +90,8 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
 
 out:
 	if (rc) {
+		pr_err("Key entry allocation failed, result: %d\n",
+			rc);
 		ima_free_key_entry(entry);
 		entry = NULL;
 	}
-- 
2.17.1

Re: [PATCH] IMA: Add log statements for failure conditions.

[Mimi Zohar]

Hi Tushar,

On Fri, 2020-02-07 at 11:53 -0800, Tushar Sugandhi wrote:
> process_buffer_measurement() and ima_alloc_key_entry()
> functions do not have log messages for failure conditions.
> 
> This change adds log statements in the above functions. 
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>

The two patches you posted are related.  Please group them as a patch
set, making this patch 2/2.

In addition, as Shuah Khan suggested for the security/integrity/
directory, "there is an opportunity here to add #define pr_fmt(fmt)
KBUILD_MODNAME ": " fmt to integrity.h and get rid of duplicate
defines."  With Joe Perches patch (waiting for it to be re-posted),
are all the pr_fmt definitions needed in each file in the
integrity/ima directory?

> ---
>  security/integrity/ima/ima_main.c       | 4 ++++
>  security/integrity/ima/ima_queue_keys.c | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 9fe949c6a530..afab796fb765 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -757,6 +757,10 @@ void process_buffer_measurement(const void *buf, int size,
>  		ima_free_template_entry(entry);
>  
>  out:
> +	if (ret < 0)
> +		pr_err("Process buffer measurement failed, result: %d\n",
> +			ret);

There's no reason to split the statement like this.  The joined line
is less than 80 characters.

> +
>  	return;
>  }
>  
> diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
> index c87c72299191..2cc52f17ea81 100644
> --- a/security/integrity/ima/ima_queue_keys.c
> +++ b/security/integrity/ima/ima_queue_keys.c
> @@ -90,6 +90,8 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
>  
>  out:
>  	if (rc) {
> +		pr_err("Key entry allocation failed, result: %d\n",
> +			rc);

ditto

>  		ima_free_key_entry(entry);
>  		entry = NULL;
>  	}

thanks,

Mimi

Re: [PATCH] IMA: Add log statements for failure conditions.

[Joe Perches]

On Sun, 2020-02-09 at 07:57 -0500, Mimi Zohar wrote:
> Hi Tushar,
> 
> On Fri, 2020-02-07 at 11:53 -0800, Tushar Sugandhi wrote:
> > process_buffer_measurement() and ima_alloc_key_entry()
> > functions do not have log messages for failure conditions.
> > 
> > This change adds log statements in the above functions. 
> > 
> > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> > Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> 
> The two patches you posted are related.  Please group them as a patch
> set, making this patch 2/2.
> 
> In addition, as Shuah Khan suggested for the security/integrity/
> directory, "there is an opportunity here to add #define pr_fmt(fmt)
> KBUILD_MODNAME ": " fmt to integrity.h and get rid of duplicate
> defines."  With Joe Perches patch (waiting for it to be re-posted),
> are all the pr_fmt definitions needed in each file in the
> integrity/ima directory?

btw Tushar and Lakshmi:

I am not formally submitting a patch here.

I was just making suggestions and please do
with it as you think appropriate.

and welcome, cheers, Joe

> > ---
> >  security/integrity/ima/ima_main.c       | 4 ++++
> >  security/integrity/ima/ima_queue_keys.c | 2 ++
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 9fe949c6a530..afab796fb765 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -757,6 +757,10 @@ void process_buffer_measurement(const void *buf, int size,
> >  		ima_free_template_entry(entry);
> >  
> >  out:
> > +	if (ret < 0)
> > +		pr_err("Process buffer measurement failed, result: %d\n",
> > +			ret);
> 
> There's no reason to split the statement like this.  The joined line
> is less than 80 characters.
> 
> > +
> >  	return;
> >  }
> >  
> > diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
> > index c87c72299191..2cc52f17ea81 100644
> > --- a/security/integrity/ima/ima_queue_keys.c
> > +++ b/security/integrity/ima/ima_queue_keys.c
> > @@ -90,6 +90,8 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
> >  
> >  out:
> >  	if (rc) {
> > +		pr_err("Key entry allocation failed, result: %d\n",
> > +			rc);
> 
> ditto
> 
> >  		ima_free_key_entry(entry);
> >  		entry = NULL;
> >  	}
> 
> thanks,
> 
> Mimi
> 

Re: [PATCH] IMA: Add log statements for failure conditions.

[Lakshmi Ramasubramanian]

On 2/9/20 6:46 PM, Joe Perches wrote:

>>
>> In addition, as Shuah Khan suggested for the security/integrity/
>> directory, "there is an opportunity here to add #define pr_fmt(fmt)
>> KBUILD_MODNAME ": " fmt to integrity.h and get rid of duplicate
>> defines."  

Good point - we'll make that change.

With Joe Perches patch (waiting for it to be re-posted),
>> are all the pr_fmt definitions needed in each file in the
>> integrity/ima directory?
> 
> btw Tushar and Lakshmi:
> 
> I am not formally submitting a patch here.
> 
> I was just making suggestions and please do
> with it as you think appropriate.

Joe - it's not clear to me what you are suggesting.
We'll move the #define for pr_fmt to integrity.h.

What's other changes are you proposing?

>>>   
>>>   out:
>>> +	if (ret < 0)
>>> +		pr_err("Process buffer measurement failed, result: %d\n",
>>> +			ret);
>>
>> There's no reason to split the statement like this.  The joined line
>> is less than 80 characters.

Agree.

thanks,
  -lakshmi

Re: [PATCH] IMA: Add log statements for failure conditions.

[Joe Perches]

On Mon, 2020-02-10 at 08:40 -0800, Lakshmi Ramasubramanian wrote:
> On 2/9/20 6:46 PM, Joe Perches wrote:
> 
> > > In addition, as Shuah Khan suggested for the security/integrity/
> > > directory, "there is an opportunity here to add #define pr_fmt(fmt)
> > > KBUILD_MODNAME ": " fmt to integrity.h and get rid of duplicate
> > > defines."  
> 
> Good point - we'll make that change.
> 
> With Joe Perches patch (waiting for it to be re-posted),
> > > are all the pr_fmt definitions needed in each file in the
> > > integrity/ima directory?
> > 
> > btw Tushar and Lakshmi:
> > 
> > I am not formally submitting a patch here.
> > 
> > I was just making suggestions and please do
> > with it as you think appropriate.
> 
> Joe - it's not clear to me what you are suggesting.
> We'll move the #define for pr_fmt to integrity.h.
> 
> What's other changes are you proposing?

https://lore.kernel.org/lkml/4b4ee302f2f97e3907ab03e55a92ccd46b6cf171.camel@perches.com/

Re: [PATCH] IMA: Add log statements for failure conditions.

[Tushar Sugandhi]

On 2020-02-10 8:50 a.m., Joe Perches wrote:
> On Mon, 2020-02-10 at 08:40 -0800, Lakshmi Ramasubramanian wrote:
>> On 2/9/20 6:46 PM, Joe Perches wrote:
>>
>>>> In addition, as Shuah Khan suggested for the security/integrity/
>>>> directory, "there is an opportunity here to add #define pr_fmt(fmt)
>>>> KBUILD_MODNAME ": " fmt to integrity.h and get rid of duplicate
>>>> defines."
>>
>> Good point - we'll make that change.
>>
>> With Joe Perches patch (waiting for it to be re-posted),
>>>> are all the pr_fmt definitions needed in each file in the
>>>> integrity/ima directory?
>>>
>>> btw Tushar and Lakshmi:
>>>
>>> I am not formally submitting a patch here.
>>>
>>> I was just making suggestions and please do
>>> with it as you think appropriate.
>>
>> Joe - it's not clear to me what you are suggesting.
>> We'll move the #define for pr_fmt to integrity.h.
>>
>> What's other changes are you proposing?
> 
> https://lore.kernel.org/lkml/4b4ee302f2f97e3907ab03e55a92ccd46b6cf171.camel@perches.com/
> 
Thanks Joe.

Joe, Shuah:

Could one of you please clarify if the changes proposed in the above URL 
will be part of Shuah's future patchset?

Or should I include those in my patchset? I am referring to the 
following snippet in security/integrity/integrity.h.


+#ifdef pr_fmt
+#undef pr_fmt
+#endif
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " KBUILD_BASENAME ": " fmt
+

If I add the above in my patchset, I believe I should remove #defines 
for pr_fmt in the .c files under /security/integrity? (except the below one)
latform_certs/efi_parser.c:#define pr_fmt(fmt) "EFI: "fmt

Please let me know.

Thanks,
Tushar

Re: [PATCH] IMA: Add log statements for failure conditions.

[Mimi Zohar]

Hi Tushar,

On Mon, 2020-02-10 at 13:33 -0800, Tushar Sugandhi wrote:
> On 2020-02-10 8:50 a.m., Joe Perches wrote:
> > On Mon, 2020-02-10 at 08:40 -0800, Lakshmi Ramasubramanian wrote:
> >> On 2/9/20 6:46 PM, Joe Perches wrote:
> >>
> >>>> In addition, as Shuah Khan suggested for the security/integrity/
> >>>> directory, "there is an opportunity here to add #define pr_fmt(fmt)
> >>>> KBUILD_MODNAME ": " fmt to integrity.h and get rid of duplicate
> >>>> defines."
> >> Good point - we'll make that change.
> >>
> >> With Joe Perches patch (waiting for it to be re-posted),
> >>>> are all the pr_fmt definitions needed in each file in the
> >>>> integrity/ima directory?
> >>> btw Tushar and Lakshmi:
> >>>
> >>> I am not formally submitting a patch here.
> >>>
> >>> I was just making suggestions and please do
> >>> with it as you think appropriate.
> >> Joe - it's not clear to me what you are suggesting.
> >> We'll move the #define for pr_fmt to integrity.h.
> >>
> >> What's other changes are you proposing?
> > https://lore.kernel.org/lkml/4b4ee302f2f97e3907ab03e55a92ccd46b6cf171.camel@perches.com/
> >
> Thanks Joe.
> 
> Joe, Shuah:
> 
> Could one of you please clarify if the changes proposed in the above URL 
> will be part of Shuah's future patchset?
> 
> Or should I include those in my patchset? I am referring to the 
> following snippet in security/integrity/integrity.h.

Joe is saying that he made some suggestions, which Shuah commented on,
but has no intention of posting a formal patch.  The end result of
that discussion is to define pr_fmt once in integrity/integrity.h and
remove any duplication in the integrity/ files.

I'd appreciate your including that change in this patch set, and if
needed a similar one in ima/ima.h.

thanks,

Mimi

Re: [PATCH] IMA: Add log statements for failure conditions.

[Tushar Sugandhi]

Thanks Mimi.

On 2020-02-10 1:46 p.m., Mimi Zohar wrote:
> Hi Tushar,
> 
> On Mon, 2020-02-10 at 13:33 -0800, Tushar Sugandhi wrote:
>> On 2020-02-10 8:50 a.m., Joe Perches wrote:
>>> On Mon, 2020-02-10 at 08:40 -0800, Lakshmi Ramasubramanian wrote:
>>>> On 2/9/20 6:46 PM, Joe Perches wrote:
>>>>
>>>>>> In addition, as Shuah Khan suggested for the security/integrity/
>>>>>> directory, "there is an opportunity here to add #define pr_fmt(fmt)
>>>>>> KBUILD_MODNAME ": " fmt to integrity.h and get rid of duplicate
>>>>>> defines."
>>>> Good point - we'll make that change.
>>>>
>>>> With Joe Perches patch (waiting for it to be re-posted),
>>>>>> are all the pr_fmt definitions needed in each file in the
>>>>>> integrity/ima directory?
>>>>> btw Tushar and Lakshmi:
>>>>>
>>>>> I am not formally submitting a patch here.
>>>>>
>>>>> I was just making suggestions and please do
>>>>> with it as you think appropriate.
>>>> Joe - it's not clear to me what you are suggesting.
>>>> We'll move the #define for pr_fmt to integrity.h.
>>>>
>>>> What's other changes are you proposing?
>>> https://lore.kernel.org/lkml/4b4ee302f2f97e3907ab03e55a92ccd46b6cf171.camel@perches.com/
>>>
>> Thanks Joe.
>>
>> Joe, Shuah:
>>
>> Could one of you please clarify if the changes proposed in the above URL
>> will be part of Shuah's future patchset?
>>
>> Or should I include those in my patchset? I am referring to the
>> following snippet in security/integrity/integrity.h.
> 
> Joe is saying that he made some suggestions, which Shuah commented on,
> but has no intention of posting a formal patch.  The end result of
> that discussion is to define pr_fmt once in integrity/integrity.h and
> remove any duplication in the integrity/ files.
> 
> I'd appreciate your including that change in this patch set, and if
> needed a similar one in ima/ima.h.
I will add the proposed change to pr_fmt to this patchset.
I will also check if a similar change is needed in ima/ima.h.
> 
> thanks,
> 
> Mimi
>