| Message ID | 20200904194100.761848-4-bmeneg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | integrity: improve user feedback for invalid bootparams | expand |
Hi Bruno, > + bool sb_state = arch_ima_get_secureboot(); > + int appraisal_state = ima_appraise; > > if (strncmp(str, "off", 3) == 0) > - ima_appraise = 0; > + appraisal_state = 0; > else if (strncmp(str, "log", 3) == 0) > - ima_appraise = IMA_APPRAISE_LOG; > + appraisal_state = IMA_APPRAISE_LOG; > else if (strncmp(str, "fix", 3) == 0) > - ima_appraise = IMA_APPRAISE_FIX; > + appraisal_state = IMA_APPRAISE_FIX; > else if (strncmp(str, "enforce", 7) == 0) > - ima_appraise = IMA_APPRAISE_ENFORCE; > + appraisal_state = IMA_APPRAISE_ENFORCE; > else > pr_err("invalid \"%s\" appraise option", str); > + > + /* If appraisal state was changed, but secure boot is enabled, > + * keep its default */ > + if (sb_state) { > + if (!(appraisal_state & IMA_APPRAISE_ENFORCE)) > + pr_info("Secure boot enabled: ignoring ima_appraise=%s option", > + str); > + else > + ima_appraise = appraisal_state; > + } Shouldn't the "else" clause be here. No need to re-post the entire patch set. thanks, Mimi > #endif > return 1; > }
On Fri, Sep 04, 2020 at 05:07:08PM -0400, Mimi Zohar wrote: > Hi Bruno, > > > + bool sb_state = arch_ima_get_secureboot(); > > + int appraisal_state = ima_appraise; > > > > if (strncmp(str, "off", 3) == 0) > > - ima_appraise = 0; > > + appraisal_state = 0; > > else if (strncmp(str, "log", 3) == 0) > > - ima_appraise = IMA_APPRAISE_LOG; > > + appraisal_state = IMA_APPRAISE_LOG; > > else if (strncmp(str, "fix", 3) == 0) > > - ima_appraise = IMA_APPRAISE_FIX; > > + appraisal_state = IMA_APPRAISE_FIX; > > else if (strncmp(str, "enforce", 7) == 0) > > - ima_appraise = IMA_APPRAISE_ENFORCE; > > + appraisal_state = IMA_APPRAISE_ENFORCE; > > else > > pr_err("invalid \"%s\" appraise option", str); > > + > > + /* If appraisal state was changed, but secure boot is enabled, > > + * keep its default */ > > + if (sb_state) { > > + if (!(appraisal_state & IMA_APPRAISE_ENFORCE)) > > + pr_info("Secure boot enabled: ignoring ima_appraise=%s option", > > + str); > > + else > > + ima_appraise = appraisal_state; > > + } > > Shouldn't the "else" clause be here. No need to re-post the entire > patch set. Yes, of course it should. Sorry. Sending the v3 for this patch. > > thanks, > > Mimi > > > #endif > > return 1; > > } > >
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 2193b51c2743..d17808245592 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -19,22 +19,29 @@ 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(); + int appraisal_state = ima_appraise; if (strncmp(str, "off", 3) == 0) - ima_appraise = 0; + appraisal_state = 0; else if (strncmp(str, "log", 3) == 0) - ima_appraise = IMA_APPRAISE_LOG; + appraisal_state = IMA_APPRAISE_LOG; else if (strncmp(str, "fix", 3) == 0) - ima_appraise = IMA_APPRAISE_FIX; + appraisal_state = IMA_APPRAISE_FIX; else if (strncmp(str, "enforce", 7) == 0) - ima_appraise = IMA_APPRAISE_ENFORCE; + appraisal_state = IMA_APPRAISE_ENFORCE; else pr_err("invalid \"%s\" appraise option", str); + + /* If appraisal state was changed, but secure boot is enabled, + * keep its default */ + if (sb_state) { + if (!(appraisal_state & IMA_APPRAISE_ENFORCE)) + pr_info("Secure boot enabled: ignoring ima_appraise=%s option", + str); + else + ima_appraise = appraisal_state; + } #endif return 1; }
Only prompt the unknown/invalid appraisal option if secureboot is enabled and if the current state differentiates from the original one. Signed-off-by: Bruno Meneguele <bmeneg@redhat.com> --- Changelog: v2: - update commit message (Mimi) - work with a temporary var instead of directly with ima_appraise (Mimi) security/integrity/ima/ima_appraise.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)