On Mon, Aug 24, 2020 at 04:11:22PM -0400, Mimi Zohar wrote:
> On Mon, 2020-08-17 at 18:52 -0300, Bruno Meneguele wrote:
> > Instead of print to kmsg any ima_appraise= option passed by the user in case
> > of secure boot being enabled, first check if the state was really changed
> > from its original "enforce" state, otherwise don't print anything.
>
> Please reword this patch description, removing "Instead of print to
> kmsg".
>
ack
> >
> > Signed-off-by: Bruno Meneguele <bmeneg@xxxxxxxxxx>
> > ---
> > security/integrity/ima/ima_appraise.c | 16 +++++++++++-----
> > 1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > index 2193b51c2743..000df14f198a 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -19,11 +19,7 @@
> > static int __init default_appraise_setup(char *str)
> > {
> > #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> > - if (arch_ima_get_secureboot()) {
> > - pr_info("Secure boot enabled: ignoring ima_appraise=%s boot parameter option",
> > - str);
> > - return 1;
> > - }
> > + bool sb_state = arch_ima_get_secureboot();
> >
> > if (strncmp(str, "off", 3) == 0)
> > ima_appraise = 0;
> > @@ -35,6 +31,16 @@ static int __init default_appraise_setup(char *str)
> > ima_appraise = IMA_APPRAISE_ENFORCE;
> > else
> > pr_err("invalid \"%s\" appraise option", str);
> > +
> > + /* If appraisal state was changed, but secure boot is enabled,
> > + * reset it to enforced */
> > + if (sb_state) {
> > + if (!is_ima_appraise_enabled()) {
> > + pr_info("Secure boot enabled: ignoring ima_appraise=%s option",
> > + str);
> > + ima_appraise = IMA_APPRAISE_ENFORCE;
> > + }
> > + }
>
> Instead of changing ima_appraise and then resetting it back to
> enforcing, how about using a temporary variable instead? Maybe
> something like:
>
> if (!is_ima_appraise_enabled())
> pr_info( ...)
> else
> ima_appraise = temporary value
>
Yes, indeed it would be nice to keep the default state unchanged. Only
thing is that 'is_ima_appraise_enabled()' directly checks 'ima_appraise &
IMA_APPRAISE_ENFORCE', changing to a temp var would require the if() to
check it directly.
I'm going to send a v2 with it changed.
Thanks.
> > #endif
> > return 1;
> > }
>
>
--
bmeneg
PGP Key: http://bmeneg.com/pubkey.txt
Attachment:
signature.asc
Description: PGP signature
