Message ID | 20200211231414.6640-4-tusharsu@linux.microsoft.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | IMA: improve log messages in IMA | expand |
On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote: > The #define for formatting log messages, pr_fmt, is duplicated in the > files under security/integrity. > > This change moves the definition to security/integrity/integrity.h and > removes the duplicate definitions in the other files under > security/integrity. Also, it adds KBUILD_MODNAME and KBUILD_BASENAME prefix > to the log messages. > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > Suggested-by: Joe Perches <joe@perches.com> > Suggested-by: Shuah Khan <skhan@linuxfoundation.org> <snip> > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > index 73fc286834d7..b1bb4d2263be 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -6,6 +6,12 @@ > * Mimi Zohar <zohar@us.ibm.com> > */ > > +#ifdef pr_fmt > +#undef pr_fmt > +#endif > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " KBUILD_BASENAME ": " fmt > + > #include <linux/types.h> > #include <linux/integrity.h> > #include <crypto/sha.h> Joe, Shuah, including the pr_fmt() in integrity/integrity.h not only affects the integrity directory but everything below it. Adding KBUILD_BASENAME to pr_fmt() modifies all of the existing IMA and EVM kernel messages. Is that ok or should there be a separate pr_fmt() for the subdirectories? thanks, Mimi
On Wed, 2020-02-12 at 09:29 -0500, Mimi Zohar wrote: > On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote: > > The #define for formatting log messages, pr_fmt, is duplicated in > > the > > files under security/integrity. > > > > This change moves the definition to security/integrity/integrity.h > > and > > removes the duplicate definitions in the other files under > > security/integrity. Also, it adds KBUILD_MODNAME and > > KBUILD_BASENAME prefix > > to the log messages. > > > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > > Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > > Suggested-by: Joe Perches <joe@perches.com> > > Suggested-by: Shuah Khan <skhan@linuxfoundation.org> > > <snip> > > > diff --git a/security/integrity/integrity.h > > b/security/integrity/integrity.h > > index 73fc286834d7..b1bb4d2263be 100644 > > --- a/security/integrity/integrity.h > > +++ b/security/integrity/integrity.h > > @@ -6,6 +6,12 @@ > > * Mimi Zohar <zohar@us.ibm.com> > > */ > > > > +#ifdef pr_fmt > > +#undef pr_fmt > > +#endif > > + > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " KBUILD_BASENAME ": " fmt > > + > > #include <linux/types.h> > > #include <linux/integrity.h> > > #include <crypto/sha.h> > > Joe, Shuah, including the pr_fmt() in integrity/integrity.h not only > affects the integrity directory but everything below it. Adding > KBUILD_BASENAME to pr_fmt() modifies all of the existing IMA and EVM > kernel messages. Is that ok or should there be a separate pr_fmt() > for the subdirectories? Log messages are often consumed by log monitors, which mostly use pattern matching to find messages they're interested in, so you have to take some care when changing the messages the kernel spits out and you have to make sure any change gets well notified so the distributions can warn about it. For this one, can we see a "before" and "after" message so we know what's happening? James
On Wed, 2020-02-12 at 10:26 -0500, James Bottomley wrote: > Log messages are often consumed by log monitors, which mostly use > pattern matching to find messages they're interested in, so you have to > take some care when changing the messages the kernel spits out and you > have to make sure any change gets well notified so the distributions > can warn about it. > > For this one, can we see a "before" and "after" message so we know > what's happening? https://patchwork.kernel.org/patch/11357335/#23134147
On 2/12/20 8:26 AM, James Bottomley wrote: > On Wed, 2020-02-12 at 09:29 -0500, Mimi Zohar wrote: >> On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote: >>> The #define for formatting log messages, pr_fmt, is duplicated in >>> the >>> files under security/integrity. >>> >>> This change moves the definition to security/integrity/integrity.h >>> and >>> removes the duplicate definitions in the other files under >>> security/integrity. Also, it adds KBUILD_MODNAME and >>> KBUILD_BASENAME prefix >>> to the log messages. >>> >>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >>> Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >>> Suggested-by: Joe Perches <joe@perches.com> >>> Suggested-by: Shuah Khan <skhan@linuxfoundation.org> >> >> <snip> >> >>> diff --git a/security/integrity/integrity.h >>> b/security/integrity/integrity.h >>> index 73fc286834d7..b1bb4d2263be 100644 >>> --- a/security/integrity/integrity.h >>> +++ b/security/integrity/integrity.h >>> @@ -6,6 +6,12 @@ >>> * Mimi Zohar <zohar@us.ibm.com> >>> */ >>> >>> +#ifdef pr_fmt >>> +#undef pr_fmt >>> +#endif >>> + >>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " KBUILD_BASENAME ": " fmt >>> + >>> #include <linux/types.h> >>> #include <linux/integrity.h> >>> #include <crypto/sha.h> >> >> Joe, Shuah, including the pr_fmt() in integrity/integrity.h not only >> affects the integrity directory but everything below it. Adding >> KBUILD_BASENAME to pr_fmt() modifies all of the existing IMA and EVM >> kernel messages. Is that ok or should there be a separate pr_fmt() >> for the subdirectories? > > Log messages are often consumed by log monitors, which mostly use > pattern matching to find messages they're interested in, so you have to > take some care when changing the messages the kernel spits out and you > have to make sure any change gets well notified so the distributions > can warn about it. > > For this one, can we see a "before" and "after" message so we know > what's happening? > Mimi and James, My suggestion was based on thinking that simplifying this by removing duplicate defines. Some messages are missing modules names, adding module name to them does change the messages. If using one pr_fmt for all modules changes the world and makes it difficult for log monitors, I would say it isn't a good change. I will leave this totally up to Mimi to decide. Feel free to throw out my suggestion if it leads more trouble than help. :) thanks, -- Shuah
On Wed, 2020-02-12 at 15:52 -0700, Shuah Khan wrote: > On 2/12/20 8:26 AM, James Bottomley wrote: > > On Wed, 2020-02-12 at 09:29 -0500, Mimi Zohar wrote: > >> On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote: > >>> The #define for formatting log messages, pr_fmt, is duplicated in > >>> the > >>> files under security/integrity. > >>> > >>> This change moves the definition to security/integrity/integrity.h > >>> and > >>> removes the duplicate definitions in the other files under > >>> security/integrity. Also, it adds KBUILD_MODNAME and > >>> KBUILD_BASENAME prefix > >>> to the log messages. > >>> > >>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > >>> Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > >>> Suggested-by: Joe Perches <joe@perches.com> > >>> Suggested-by: Shuah Khan <skhan@linuxfoundation.org> > >> > >> <snip> > >> > >>> diff --git a/security/integrity/integrity.h > >>> b/security/integrity/integrity.h > >>> index 73fc286834d7..b1bb4d2263be 100644 > >>> --- a/security/integrity/integrity.h > >>> +++ b/security/integrity/integrity.h > >>> @@ -6,6 +6,12 @@ > >>> * Mimi Zohar <zohar@us.ibm.com> > >>> */ > >>> > >>> +#ifdef pr_fmt > >>> +#undef pr_fmt > >>> +#endif > >>> + > >>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " KBUILD_BASENAME ": " fmt > >>> + > >>> #include <linux/types.h> > >>> #include <linux/integrity.h> > >>> #include <crypto/sha.h> > >> > >> Joe, Shuah, including the pr_fmt() in integrity/integrity.h not only > >> affects the integrity directory but everything below it. Adding > >> KBUILD_BASENAME to pr_fmt() modifies all of the existing IMA and EVM > >> kernel messages. Is that ok or should there be a separate pr_fmt() > >> for the subdirectories? > > > > > Log messages are often consumed by log monitors, which mostly use > > pattern matching to find messages they're interested in, so you have to > > take some care when changing the messages the kernel spits out and you > > have to make sure any change gets well notified so the distributions > > can warn about it. > > > > For this one, can we see a "before" and "after" message so we know > > what's happening? > > > > Mimi and James, > > My suggestion was based on thinking that simplifying this by removing > duplicate defines. Some messages are missing modules names, adding > module name to them does change the messages. > > If using one pr_fmt for all modules changes the world and makes it > difficult for log monitors, I would say it isn't a good change. > > I will leave this totally up to Mimi to decide. Feel free to throw > out my suggestion if it leads more trouble than help. :) Thanks, Shuah. Tushar, I don't see any need for changing the existing IMA/EVM messages. Either remove the KBUILD_BASENAME from the format or limit the new format to the integrity directory. thanks, Mimi
On 2020-02-12 4:38 p.m., Mimi Zohar wrote: > On Wed, 2020-02-12 at 15:52 -0700, Shuah Khan wrote: >> On 2/12/20 8:26 AM, James Bottomley wrote: >>> On Wed, 2020-02-12 at 09:29 -0500, Mimi Zohar wrote: >>>> On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote: >>>>> The #define for formatting log messages, pr_fmt, is duplicated in >>>>> the >>>>> files under security/integrity. >>>>> >>>>> This change moves the definition to security/integrity/integrity.h >>>>> and >>>>> removes the duplicate definitions in the other files under >>>>> security/integrity. Also, it adds KBUILD_MODNAME and >>>>> KBUILD_BASENAME prefix >>>>> to the log messages. >>>>> >>>>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >>>>> Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >>>>> Suggested-by: Joe Perches <joe@perches.com> >>>>> Suggested-by: Shuah Khan <skhan@linuxfoundation.org> >>>> >>>> <snip> >>>> >>>>> diff --git a/security/integrity/integrity.h >>>>> b/security/integrity/integrity.h >>>>> index 73fc286834d7..b1bb4d2263be 100644 >>>>> --- a/security/integrity/integrity.h >>>>> +++ b/security/integrity/integrity.h >>>>> @@ -6,6 +6,12 @@ >>>>> * Mimi Zohar <zohar@us.ibm.com> >>>>> */ >>>>> >>>>> +#ifdef pr_fmt >>>>> +#undef pr_fmt >>>>> +#endif >>>>> + >>>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " KBUILD_BASENAME ": " fmt >>>>> + >>>>> #include <linux/types.h> >>>>> #include <linux/integrity.h> >>>>> #include <crypto/sha.h> >>>> >>>> Joe, Shuah, including the pr_fmt() in integrity/integrity.h not only >>>> affects the integrity directory but everything below it. Adding >>>> KBUILD_BASENAME to pr_fmt() modifies all of the existing IMA and EVM >>>> kernel messages. Is that ok or should there be a separate pr_fmt() >>>> for the subdirectories? >>> >> >>> Log messages are often consumed by log monitors, which mostly use >>> pattern matching to find messages they're interested in, so you have to >>> take some care when changing the messages the kernel spits out and you >>> have to make sure any change gets well notified so the distributions >>> can warn about it. >>> >>> For this one, can we see a "before" and "after" message so we know >>> what's happening? >>> >> >> Mimi and James, >> >> My suggestion was based on thinking that simplifying this by removing >> duplicate defines. Some messages are missing modules names, adding >> module name to them does change the messages. >> >> If using one pr_fmt for all modules changes the world and makes it >> difficult for log monitors, I would say it isn't a good change. >> >> I will leave this totally up to Mimi to decide. Feel free to throw >> out my suggestion if it leads more trouble than help. :) > > Thanks, Shuah. Tushar, I don't see any need for changing the existing > IMA/EVM messages. Either remove the KBUILD_BASENAME from the format > or limit the new format to the integrity directory. Ok. I will remove the KBUILD_BASENAME from the format. And I will keep the definition in security/integrity/integrity.h, and will keep the duplicates removed - as originally proposed in this patch v3 3/3. > > thanks, > > Mimi >
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c index ea1aae3d07b3..e9cbadade74b 100644 --- a/security/integrity/digsig.c +++ b/security/integrity/digsig.c @@ -6,8 +6,6 @@ * Dmitry Kasatkin <dmitry.kasatkin@intel.com> */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <linux/err.h> #include <linux/sched.h> #include <linux/slab.h> diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c index 55aec161d0e1..4e0d6778277e 100644 --- a/security/integrity/digsig_asymmetric.c +++ b/security/integrity/digsig_asymmetric.c @@ -6,8 +6,6 @@ * Dmitry Kasatkin <dmitry.kasatkin@intel.com> */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <linux/err.h> #include <linux/ratelimit.h> #include <linux/key-type.h> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index d485f6fc908e..35682852ddea 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -10,8 +10,6 @@ * Using root's kernel master key (kmk), calculate the HMAC */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <linux/export.h> #include <linux/crypto.h> #include <linux/xattr.h> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index f9a81b187fae..d361d7fdafc4 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -11,8 +11,6 @@ * evm_inode_removexattr, and evm_verifyxattr */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <linux/init.h> #include <linux/crypto.h> #include <linux/audit.h> diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c index c11c1f7b3ddd..39ad1038d45d 100644 --- a/security/integrity/evm/evm_secfs.c +++ b/security/integrity/evm/evm_secfs.c @@ -10,8 +10,6 @@ * - Get the key and enable EVM */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <linux/audit.h> #include <linux/uaccess.h> #include <linux/init.h> diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c index 7678f0e3e84d..aaae80c4e376 100644 --- a/security/integrity/ima/ima_asymmetric_keys.c +++ b/security/integrity/ima/ima_asymmetric_keys.c @@ -9,8 +9,6 @@ * create or update. */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <keys/asymmetric-type.h> #include "ima.h" diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index 7967a6904851..423c84f95a14 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -10,8 +10,6 @@ * Calculates md5/sha1 file hash, template hash, boot-aggreate hash */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <linux/kernel.h> #include <linux/moduleparam.h> #include <linux/ratelimit.h> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index 2000e8df0301..a71e822a6e92 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -12,8 +12,6 @@ * current measurement list and IMA statistics */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <linux/fcntl.h> #include <linux/slab.h> #include <linux/init.h> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 195cb4079b2b..567468188a61 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -11,8 +11,6 @@ * initialization and cleanup functions */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <linux/init.h> #include <linux/scatterlist.h> #include <linux/slab.h> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index 9e94eca48b89..121de3e04af2 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -6,7 +6,6 @@ * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> * Mimi Zohar <zohar@linux.vnet.ibm.com> */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include <linux/seq_file.h> #include <linux/vmalloc.h> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 6e1576d9eb48..125a88d6eeca 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -15,8 +15,6 @@ * and ima_file_check. */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <linux/module.h> #include <linux/file.h> #include <linux/binfmts.h> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 453427048999..c334e0dc6083 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -7,8 +7,6 @@ * - initialize default measure policy rules */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <linux/init.h> #include <linux/list.h> #include <linux/fs.h> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 1ce8b1701566..8753212ddb18 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -15,8 +15,6 @@ * ever removed or changed during the boot-cycle. */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <linux/rculist.h> #include <linux/slab.h> #include "ima.h" diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c index c87c72299191..cb3e3f501593 100644 --- a/security/integrity/ima/ima_queue_keys.c +++ b/security/integrity/ima/ima_queue_keys.c @@ -8,8 +8,6 @@ * Enables deferred processing of keys */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <linux/workqueue.h> #include <keys/asymmetric-type.h> #include "ima.h" diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index 6aa6408603e3..062d9ad49afb 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -9,8 +9,6 @@ * Helpers to manage template descriptors. */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include <linux/rculist.h> #include "ima.h" #include "ima_template_lib.h" diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 32ae05d88257..9cd1e50f3ccc 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -9,8 +9,6 @@ * Library of supported template fields. */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - #include "ima_template_lib.h" static bool ima_template_hash_algo_allowed(u8 algo) diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 73fc286834d7..b1bb4d2263be 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -6,6 +6,12 @@ * Mimi Zohar <zohar@us.ibm.com> */ +#ifdef pr_fmt +#undef pr_fmt +#endif + +#define pr_fmt(fmt) KBUILD_MODNAME ": " KBUILD_BASENAME ": " fmt + #include <linux/types.h> #include <linux/integrity.h> #include <crypto/sha.h>