| Message ID | 1508774387.3639.128.camel@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I've added this into my series as the third patch, but: Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > + ima_use_appraise_tcb = TRUE; Did you mean "true" rather than "TRUE"? > + entry = kzalloc(sizeof(*entry), GFP_KERNEL); > + if (entry) { > + memcpy(entry, &secure_boot_rules[i], > + sizeof(*entry)); kmemdup()? I guess also that oopsing is okay if the allocation fails. We've run out of memory during early boot, after all. > + INIT_LIST_HEAD(&entry->list); > + list_add_tail(&entry->list, &ima_policy_rules); Isn't the init redundant, given the following line? David
On Mon, 2017-10-30 at 15:55 +0000, David Howells wrote: > I've added this into my series as the third patch, but: > > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > + ima_use_appraise_tcb = TRUE; > > Did you mean "true" rather than "TRUE"? Yes, of course. Commit 9f4b6a254d7a "ima: Fix bool initialization/comparison" already addresses it. Please remove it from this patch. > > > + entry = kzalloc(sizeof(*entry), GFP_KERNEL); > > + if (entry) { > > + memcpy(entry, &secure_boot_rules[i], > > + sizeof(*entry)); > > kmemdup()? Probably > > I guess also that oopsing is okay if the allocation fails. We've run out of > memory during early boot, after all. If the memory allocation fails, the "secure_boot" policy will not be enabled for custom policies, but how is that "oopsing". If it fails, there needs to be some indication of the failure, which there currently isn't. Perhaps also prevent loading a custom policy. > > > + INIT_LIST_HEAD(&entry->list); > > + list_add_tail(&entry->list, &ima_policy_rules); > > Isn't the init redundant, given the following line? ok
Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > Did you mean "true" rather than "TRUE"? > > Yes, of course. Commit 9f4b6a254d7a "ima: Fix bool > initialization/comparison" already addresses it. Please remove it > from this patch. Is that with James? I don't seem to have a copy, and I don't want to cause a patch collision. > > I guess also that oopsing is okay if the allocation fails. We've run out of > > memory during early boot, after all. > > If the memory allocation fails, the "secure_boot" policy will not be > enabled for custom policies, but how is that "oopsing". Sorry - I overlooked the fact that the variable is not used if it's not zero. > If it fails, there needs to be some indication of the failure, which there > currently isn't. Perhaps also prevent loading a custom policy. Does it need to panic (probably fine as a small memory alloc failed)? If it doesn't set this policy what's the effect on things using is_ima_appraise_enabled() - assuming we get that far? David
On Mon, 2017-10-30 at 17:05 +0000, David Howells wrote: > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > > Did you mean "true" rather than "TRUE"? > > > > Yes, of course. Commit 9f4b6a254d7a "ima: Fix bool > > initialization/comparison" already addresses it. Please remove it > > from this patch. > > Is that with James? I don't seem to have a copy, and I don't want to cause a > patch collision. No, it's staged in my next branch. After the disaster during the last open window due to a linux-integrity patch, Linus requested linux- integrity be pulled independently of the security pull request. We'll see how that goes during the next open window. James is staging the subsystem patches independently of each other, in case of a similar problem, so that they can be pulled separately. There's a new "next-general" branch. > > > I guess also that oopsing is okay if the allocation fails. We've run out of > > > memory during early boot, after all. > > > > If the memory allocation fails, the "secure_boot" policy will not be > > enabled for custom policies, but how is that "oopsing". > > Sorry - I overlooked the fact that the variable is not used if it's not zero. > > > If it fails, there needs to be some indication of the failure, which there > > currently isn't. Perhaps also prevent loading a custom policy. > > Does it need to panic (probably fine as a small memory alloc failed)? If it > doesn't set this policy what's the effect on things using > is_ima_appraise_enabled() - assuming we get that far? Assuming that the memory is a temporary failure, the system continues to boot, and a custom policy is installed, there's a potential IMA- appraisal gap, meaning that if the custom policy doesn't include the secure boot rules, files that should be appraised (eg. kernel modules, kexec'ed kernel image, firmware) won't be. Mimi
On Mon, 30 Oct 2017, Mimi Zohar wrote: > James is staging the subsystem patches independently of each other, in > case of a similar problem, so that they can be pulled separately. > There's a new "next-general" branch. If you send me a pull request, I'm combining branches into next-testing, too, which is pulled into -next.
[Cc'ing Stephen Rothwell] On Tue, 2017-10-31 at 14:25 +1100, James Morris wrote: > On Mon, 30 Oct 2017, Mimi Zohar wrote: > > > James is staging the subsystem patches independently of each other, in > > case of a similar problem, so that they can be pulled separately. > > There's a new "next-general" branch. > > If you send me a pull request, I'm combining branches into next-testing, > too, which is pulled into -next. linux-next already has the IMA patches, which might be confusing. Mimi
Hi Mimi, On Wed, 08 Nov 2017 15:46:22 -0500 Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > [Cc'ing Stephen Rothwell] > > On Tue, 2017-10-31 at 14:25 +1100, James Morris wrote: > > On Mon, 30 Oct 2017, Mimi Zohar wrote: > > > > > James is staging the subsystem patches independently of each other, in > > > case of a similar problem, so that they can be pulled separately. > > > There's a new "next-general" branch. > > > > If you send me a pull request, I'm combining branches into next-testing, > > too, which is pulled into -next. > > linux-next already has the IMA patches, which might be confusing. Not if they are the same *commits* i.e. if the tree/branch that James merges is the same as the one that I already merge of yours (git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity#next)
On Thu, 2017-11-09 at 07:53 +1100, Stephen Rothwell wrote: > Hi Mimi, > > On Wed, 08 Nov 2017 15:46:22 -0500 Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > > [Cc'ing Stephen Rothwell] > > > > On Tue, 2017-10-31 at 14:25 +1100, James Morris wrote: > > > On Mon, 30 Oct 2017, Mimi Zohar wrote: > > > > > > > James is staging the subsystem patches independently of each other, in > > > > case of a similar problem, so that they can be pulled separately. > > > > There's a new "next-general" branch. > > > > > > If you send me a pull request, I'm combining branches into next-testing, > > > too, which is pulled into -next. > > > > linux-next already has the IMA patches, which might be confusing. > > Not if they are the same *commits* i.e. if the tree/branch that James > merges is the same as the one that I already merge of yours > (git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity#next) James' security tree is based on -rc3, while the integrity tree is based on -rc4. The rebased integrity patches are now in my security- next-integrity branch. Mimi
Hi Mimi, On Wed, 08 Nov 2017 16:04:07 -0500 Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > On Thu, 2017-11-09 at 07:53 +1100, Stephen Rothwell wrote: > > > > On Wed, 08 Nov 2017 15:46:22 -0500 Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > > > > [Cc'ing Stephen Rothwell] > > > > > > On Tue, 2017-10-31 at 14:25 +1100, James Morris wrote: > > > > On Mon, 30 Oct 2017, Mimi Zohar wrote: > > > > > > > > > James is staging the subsystem patches independently of each other, in > > > > > case of a similar problem, so that they can be pulled separately. > > > > > There's a new "next-general" branch. > > > > > > > > If you send me a pull request, I'm combining branches into next-testing, > > > > too, which is pulled into -next. > > > > > > linux-next already has the IMA patches, which might be confusing. > > > > Not if they are the same *commits* i.e. if the tree/branch that James > > merges is the same as the one that I already merge of yours > > (git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity#next) > > James' security tree is based on -rc3, while the integrity tree is > based on -rc4. The rebased integrity patches are now in my security- > next-integrity branch. So since those patches are now in James tree, you should drop them from the integrity tree.
On Thu, 2017-11-09 at 10:26 +1100, Stephen Rothwell wrote: > Hi Mimi, > > On Wed, 08 Nov 2017 16:04:07 -0500 Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > > On Thu, 2017-11-09 at 07:53 +1100, Stephen Rothwell wrote: > > > > > > On Wed, 08 Nov 2017 15:46:22 -0500 Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > > > > > > [Cc'ing Stephen Rothwell] > > > > > > > > On Tue, 2017-10-31 at 14:25 +1100, James Morris wrote: > > > > > On Mon, 30 Oct 2017, Mimi Zohar wrote: > > > > > > > > > > > James is staging the subsystem patches independently of each other, in > > > > > > case of a similar problem, so that they can be pulled separately. > > > > > > There's a new "next-general" branch. > > > > > > > > > > If you send me a pull request, I'm combining branches into next-testing, > > > > > too, which is pulled into -next. > > > > > > > > linux-next already has the IMA patches, which might be confusing. > > > > > > Not if they are the same *commits* i.e. if the tree/branch that James > > > merges is the same as the one that I already merge of yours > > > (git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity#next) > > > > James' security tree is based on -rc3, while the integrity tree is > > based on -rc4. The rebased integrity patches are now in my security- > > next-integrity branch. > > So since those patches are now in James tree, you should drop them from > the integrity tree. Ok, I had been planning on sending an independent pull request to Linus, as requested. Mimi
On Wed, 8 Nov 2017, Mimi Zohar wrote: > > So since those patches are now in James tree, you should drop them from > > the integrity tree. > > Ok, I had been planning on sending an independent pull request to > Linus, as requested. That was not requested. Linus wants separate branches to pull from, but this does not mean separate trees. The x86 and some other subsystems use separate branches in the same tree, which is the model we're now using generally with the security subsystem. It's _also_ possible to send pull requests independently (which is what the SELinux and AppArmor maintainers decided to do), although that was not what Linus was asking for. It's up to you if you want to send pull requests directly to Linus or continue to merge via the security tree. - James
On Fri, 2017-11-10 at 00:28 +1100, James Morris wrote: > On Wed, 8 Nov 2017, Mimi Zohar wrote: > > > > So since those patches are now in James tree, you should drop them from > > > the integrity tree. > > > > Ok, I had been planning on sending an independent pull request to > > Linus, as requested. > > That was not requested. Linus wants separate branches to pull from, but > this does not mean separate trees. The x86 and some other subsystems use > separate branches in the same tree, which is the model we're now using > generally with the security subsystem. > > It's _also_ possible to send pull requests independently (which is what > the SELinux and AppArmor maintainers decided to do), although that was not > what Linus was asking for. > > It's up to you if you want to send pull requests directly to Linus or > continue to merge via the security tree. Thank you for the clarification (and patience). There are a lot of interactions between the integrity subsystem and the other security subsystems, especially the TPM. Assuming it is acceptable, as you said, I'd really prefer continuing to have the integrity subsystem merged via the security tree. Mimi
On Thu, 9 Nov 2017, Mimi Zohar wrote: > Thank you for the clarification (and patience). There are a lot of > interactions between the integrity subsystem and the other security > subsystems, especially the TPM. Assuming it is acceptable, as you > said, I'd really prefer continuing to have the integrity subsystem > merged via the security tree. That's fine, whatever works for you.
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 95209a5f8595..81a7496171d0 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -196,9 +196,9 @@ static int __init policy_setup(char *str) if ((strcmp(p, "tcb") == 0) && !ima_policy) ima_policy = DEFAULT_TCB; else if (strcmp(p, "appraise_tcb") == 0) - ima_use_appraise_tcb = 1; + ima_use_appraise_tcb = TRUE; else if (strcmp(p, "secure_boot") == 0) - ima_use_secure_boot = 1; + ima_use_secure_boot = TRUE; } return 1; @@ -427,14 +427,21 @@ void ima_update_policy_flag(void) */ void __init ima_init_policy(void) { - int i, measure_entries, appraise_entries, secure_boot_entries; + int i; + int measure_entries = 0; + int appraise_entries = 0; + int secure_boot_entries = 0; + bool kernel_locked_down = kernel_is_locked_down(); /* if !ima_policy set entries = 0 so we load NO default rules */ - measure_entries = ima_policy ? ARRAY_SIZE(dont_measure_rules) : 0; - appraise_entries = ima_use_appraise_tcb ? - ARRAY_SIZE(default_appraise_rules) : 0; - secure_boot_entries = ima_use_secure_boot ? - ARRAY_SIZE(secure_boot_rules) : 0; + if (ima_policy) + measure_entries = ARRAY_SIZE(dont_measure_rules); + + if (ima_use_appraise_tcb) + appraise_entries = ARRAY_SIZE(default_appraise_rules); + + if (ima_use_secure_boot || kernel_locked_down) + secure_boot_entries = ARRAY_SIZE(secure_boot_rules); for (i = 0; i < measure_entries; i++) list_add_tail(&dont_measure_rules[i].list, &ima_default_rules); @@ -455,11 +462,26 @@ void __init ima_init_policy(void) /* * Insert the appraise rules requiring file signatures, prior to - * any other appraise rules. + * any other appraise rules. In secure boot lock-down mode, also + * require these appraise rules for custom policies. */ - for (i = 0; i < secure_boot_entries; i++) - list_add_tail(&secure_boot_rules[i].list, - &ima_default_rules); + for (i = 0; i < secure_boot_entries; i++) { + struct ima_rule_entry *entry; + + /* Include for builtin policies */ + list_add_tail(&secure_boot_rules[i].list, &ima_default_rules); + + /* Include for custom policies */ + if (kernel_locked_down) { + entry = kzalloc(sizeof(*entry), GFP_KERNEL); + if (entry) { + memcpy(entry, &secure_boot_rules[i], + sizeof(*entry)); + INIT_LIST_HEAD(&entry->list); + list_add_tail(&entry->list, &ima_policy_rules); + } + } + } for (i = 0; i < appraise_entries; i++) { list_add_tail(&default_appraise_rules[i].list,
Require the "secure_boot" rules, whether or not it is specified on the boot command line, for both the builtin and custom policies in secure boot lockdown mode. Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> --- security/integrity/ima/ima_policy.c | 46 +++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 12 deletions(-)