Re: [PATCH v9 2/8] powerpc/ima: add support to initialize ima policy rules
Hi Lakshmi,
Lakshmi Ramasubramanian writes:
> On 10/25/2019 10:02 AM, Nayna Jain wrote:
>
> >> Is there any way to not use conditional compilation in
> >> the above array definition? Maybe define different functions to get
> >> "secure_rules" for when CONFIG_MODULE_SIG_FORCE is defined and when
> >> it is not defined.
> >
> > How will you decide which function to be called ?
>
> Define the array in the C file:
>
> const char *const secure_rules_kernel_check[] = {
> "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> NULL
> };
>
> const char *const secure_rules_kernel_module_check[] = {
> "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> NULL
> };
>
> And, in the header file :
But there's no reason for any of this to be in a header, it's all
contained in one file.
Moving things into a header purely to avoid a single #ifdef in a C file
is a backward step.
> extern const char *const secure_rules_kernel_check;
> extern const char *const secure_rules_kernel_module_check;
>
> #ifdef CONFIG_MODULE_SIG_FORCE
> const char *secure_rules() { return secure_rules_kernel_check; }
> #else
> const char *secure_rules() { return secure_rules_kernel_module_check;}
> #endif // #ifdef CONFIG_MODULE_SIG_FORCE
>
> If you want to avoid duplication, secure_rules_kernel_check and
> secure_rules_kernel_module_check could be defined in separate C files
> and conditionally compiled (in Makefile).
Again that's just lots of added complication for no real benefit.
> I was just trying to suggest the guidelines given in
> "Section 21) Conditional Compilation" in coding-style.rst.
>
> It says:
> Whenever possible don't use preprocessor conditionals (#ifdef, #if) in
> .c files;...
The key phrase being "guideline" :)
That suggestion is aimed at avoiding code with lots of ifdefs sprinkled
through the body of functions. Code written in that way can be very hard
to read because you have to mentally pre-process it first, and then read
the C-level logic. See below for an example.
Moving the pre-processing out of line into helpers means when you're
reading the function you can just reason about the C control flow.
The reference to ".c files" is really talking about moving logic that is
#ifdef'ed into static inline helpers. Those typically go in headers, but
they don't have to if there's no other reason for them to be in a
header.
So where the code is all in one C file it would be completely fine to
have an #ifdef in the C file around a static inline helper.
But in this case where the #ifdef is just in an array I think it's
entirely fine to just keep the #ifdef. Its presence there doesn't
complicate the logic in anyway.
cheers
This is a "good" (bad) example of what we're trying to avoid:
static long ppc_set_hwdebug(struct task_struct *child,
struct ppc_hw_breakpoint *bp_info)
{
#ifdef CONFIG_HAVE_HW_BREAKPOINT
int len = 0;
struct thread_struct *thread = &(child->thread);
struct perf_event *bp;
struct perf_event_attr attr;
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
#ifndef CONFIG_PPC_ADV_DEBUG_REGS
struct arch_hw_breakpoint brk;
#endif
if (bp_info->version != 1)
return -ENOTSUPP;
#ifdef CONFIG_PPC_ADV_DEBUG_REGS
/*
* Check for invalid flags and combinations
*/
if ((bp_info->trigger_type == 0) ||
(bp_info->trigger_type & ~(PPC_BREAKPOINT_TRIGGER_EXECUTE |
PPC_BREAKPOINT_TRIGGER_RW)) ||
(bp_info->addr_mode & ~PPC_BREAKPOINT_MODE_MASK) ||
(bp_info->condition_mode &
~(PPC_BREAKPOINT_CONDITION_MODE |
PPC_BREAKPOINT_CONDITION_BE_ALL)))
return -EINVAL;
#if CONFIG_PPC_ADV_DEBUG_DVCS == 0
if (bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE)
return -EINVAL;
#endif
if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_EXECUTE) {
if ((bp_info->trigger_type != PPC_BREAKPOINT_TRIGGER_EXECUTE) ||
(bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE))
return -EINVAL;
return set_instruction_bp(child, bp_info);
}
if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_EXACT)
return set_dac(child, bp_info);
#ifdef CONFIG_PPC_ADV_DEBUG_DAC_RANGE
return set_dac_range(child, bp_info);
#else
return -EINVAL;
#endif
#else /* !CONFIG_PPC_ADV_DEBUG_DVCS */
/*
* We only support one data breakpoint
*/
if ((bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_RW) == 0 ||
(bp_info->trigger_type & ~PPC_BREAKPOINT_TRIGGER_RW) != 0 ||
bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE)
return -EINVAL;
if ((unsigned long)bp_info->addr >= TASK_SIZE)
return -EIO;
Re: [PATCH v9 2/8] powerpc/ima: add support to initialize ima policy rules
On Sat, 2019-10-26 at 19:52 -0400, Mimi Zohar wrote:
> On Fri, 2019-10-25 at 12:02 -0500, Nayna Jain wrote:
> > On 10/24/19 12:35 PM, Lakshmi Ramasubramanian wrote:
> > > On 10/23/2019 8:47 PM, Nayna Jain wrote:
> > >
> > >> +/*
> > >> + * The "secure_rules" are enabled only on "secureboot" enabled systems.
> > >> + * These rules verify the file signatures against known good values.
> > >> + * The "appraise_type=imasig|modsig" option allows the known good
> > >> signature
> > >> + * to be stored as an xattr or as an appended signature.
> > >> + *
> > >> + * To avoid duplicate signature verification as much as possible,
> > >> the IMA
> > >> + * policy rule for module appraisal is added only if
> > >> CONFIG_MODULE_SIG_FORCE
> > >> + * is not enabled.
> > >> + */
> > >> +static const char *const secure_rules[] = {
> > >> + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> > >> +#ifndef CONFIG_MODULE_SIG_FORCE
> > >> + "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> > >> +#endif
> > >> + NULL
> > >> +};
> > >
> > > Is there any way to not use conditional compilation in the above array
> > > definition? Maybe define different functions to get "secure_rules" for
> > > when CONFIG_MODULE_SIG_FORCE is defined and when it is not defined.
> >
> > How will you decide which function to be called ?
>
> You could call "is_module_sig_enforced()".
Calling is_module_sig_enforce() would prevent verifying the same
kernel module appended signature twice, when CONFIG_MODULE_SIG is
enabled, but not CONFIG_MODULE_SIG_FORCE. This comes at the expense
of having to define additional policies.
Unlike for the kernel image, there is no coordination between lockdown
and IMA for kernel modules signature verification. I suggest
deferring defining additional policies to when the lockdown/IMA
coordination is addressed.
Mimi
Re: [PATCH v9 2/8] powerpc/ima: add support to initialize ima policy rules
On Fri, 2019-10-25 at 12:02 -0500, Nayna Jain wrote:
> On 10/24/19 12:35 PM, Lakshmi Ramasubramanian wrote:
> > On 10/23/2019 8:47 PM, Nayna Jain wrote:
> >
> >> +/*
> >> + * The "secure_rules" are enabled only on "secureboot" enabled systems.
> >> + * These rules verify the file signatures against known good values.
> >> + * The "appraise_type=imasig|modsig" option allows the known good
> >> signature
> >> + * to be stored as an xattr or as an appended signature.
> >> + *
> >> + * To avoid duplicate signature verification as much as possible,
> >> the IMA
> >> + * policy rule for module appraisal is added only if
> >> CONFIG_MODULE_SIG_FORCE
> >> + * is not enabled.
> >> + */
> >> +static const char *const secure_rules[] = {
> >> + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> >> +#ifndef CONFIG_MODULE_SIG_FORCE
> >> + "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> >> +#endif
> >> + NULL
> >> +};
> >
> > Is there any way to not use conditional compilation in the above array
> > definition? Maybe define different functions to get "secure_rules" for
> > when CONFIG_MODULE_SIG_FORCE is defined and when it is not defined.
>
> How will you decide which function to be called ?
You could call "is_module_sig_enforced()".
Mimi
Re: [PATCH v9 2/8] powerpc/ima: add support to initialize ima policy rules
On 10/25/2019 10:02 AM, Nayna Jain wrote:
>> Is there any way to not use conditional compilation in
>> the above array definition? Maybe define different functions to get
>> "secure_rules" for when CONFIG_MODULE_SIG_FORCE is defined and when
>> it is not defined.
>
> How will you decide which function to be called ?
Define the array in the C file:
const char *const secure_rules_kernel_check[] = {
"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
NULL
};
const char *const secure_rules_kernel_module_check[] = {
"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
NULL
};
And, in the header file :
extern const char *const secure_rules_kernel_check;
extern const char *const secure_rules_kernel_module_check;
#ifdef CONFIG_MODULE_SIG_FORCE
const char *secure_rules() { return secure_rules_kernel_check; }
#else
const char *secure_rules() { return secure_rules_kernel_module_check;}
#endif // #ifdef CONFIG_MODULE_SIG_FORCE
If you want to avoid duplication, secure_rules_kernel_check and
secure_rules_kernel_module_check could be defined in separate C files
and conditionally compiled (in Makefile).
I was just trying to suggest the guidelines given in
"Section 21) Conditional Compilation" in coding-style.rst.
It says:
Whenever possible don't use preprocessor conditionals (#ifdef, #if) in
.c files;...
Feel free to do what you think is appropriate.
thanks,
-lakshmi
Re: [PATCH v9 2/8] powerpc/ima: add support to initialize ima policy rules
On 10/24/19 12:35 PM, Lakshmi Ramasubramanian wrote:
On 10/23/2019 8:47 PM, Nayna Jain wrote:
+/*
+ * The "secure_rules" are enabled only on "secureboot" enabled systems.
+ * These rules verify the file signatures against known good values.
+ * The "appraise_type=imasig|modsig" option allows the known good
signature
+ * to be stored as an xattr or as an appended signature.
+ *
+ * To avoid duplicate signature verification as much as possible,
the IMA
+ * policy rule for module appraisal is added only if
CONFIG_MODULE_SIG_FORCE
+ * is not enabled.
+ */
+static const char *const secure_rules[] = {
+ "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+#ifndef CONFIG_MODULE_SIG_FORCE
+ "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+#endif
+ NULL
+};
Is there any way to not use conditional compilation in the above array
definition? Maybe define different functions to get "secure_rules" for
when CONFIG_MODULE_SIG_FORCE is defined and when it is not defined.
How will you decide which function to be called ?
Thanks & Regards,
- Nayna
Re: [PATCH v9 2/8] powerpc/ima: add support to initialize ima policy rules
On 10/23/2019 8:47 PM, Nayna Jain wrote:
+/*
+ * The "secure_rules" are enabled only on "secureboot" enabled systems.
+ * These rules verify the file signatures against known good values.
+ * The "appraise_type=imasig|modsig" option allows the known good signature
+ * to be stored as an xattr or as an appended signature.
+ *
+ * To avoid duplicate signature verification as much as possible, the IMA
+ * policy rule for module appraisal is added only if CONFIG_MODULE_SIG_FORCE
+ * is not enabled.
+ */
+static const char *const secure_rules[] = {
+ "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+#ifndef CONFIG_MODULE_SIG_FORCE
+ "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+#endif
+ NULL
+};
Is there any way to not use conditional compilation in the above array
definition? Maybe define different functions to get "secure_rules" for
when CONFIG_MODULE_SIG_FORCE is defined and when it is not defined.
Just a suggestion.
-lakshmi
