Message-ID: <87lft4h1xz.fsf@mpe.ellerman.id.au>
Date:   Tue, 29 Oct 2019 10:42:32 +1100
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>,
        Nayna Jain <nayna@...ux.vnet.ibm.com>,
        Nayna Jain <nayna@...ux.ibm.com>, linuxppc-dev@...abs.org,
        linux-efi@...r.kernel.org, linux-integrity@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Jeremy Kerr <jk@...abs.org>,
        Matthew Garret <matthew.garret@...ula.com>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Claudio Carvalho <cclaudio@...ux.ibm.com>,
        George Wilson <gcwilson@...ux.ibm.com>,
        Elaine Palmer <erpalmer@...ibm.com>,
        Eric Ricther <erichte@...ux.ibm.com>,
        Oliver O'Halloran <oohall@...il.com>,
        Prakhar Srivastava <prsriva02@...il.com>
Subject: Re: [PATCH v9 2/8] powerpc/ima: add support to initialize ima policy rules

Hi Lakshmi,

Lakshmi Ramasubramanian <nramas@...ux.microsoft.com> 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;

	brk.address = bp_info->addr & ~7UL;
	brk.type = HW_BRK_TYPE_TRANSLATE;
	brk.len = 8;
	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
		brk.type |= HW_BRK_TYPE_READ;
	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
		brk.type |= HW_BRK_TYPE_WRITE;
#ifdef CONFIG_HAVE_HW_BREAKPOINT
	/*
	 * Check if the request is for 'range' breakpoints. We can
	 * support it if range < 8 bytes.
	 */
	if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
		len = bp_info->addr2 - bp_info->addr;
	else if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_EXACT)
		len = 1;
	else
		return -EINVAL;
	bp = thread->ptrace_bps[0];
	if (bp)
		return -ENOSPC;

	/* Create a new breakpoint request if one doesn't exist already */
	hw_breakpoint_init(&attr);
	attr.bp_addr = (unsigned long)bp_info->addr & ~HW_BREAKPOINT_ALIGN;
	attr.bp_len = len;
	arch_bp_generic_fields(brk.type, &attr.bp_type);

	thread->ptrace_bps[0] = bp = register_user_hw_breakpoint(&attr,
					       ptrace_triggered, NULL, child);
	if (IS_ERR(bp)) {
		thread->ptrace_bps[0] = NULL;
		return PTR_ERR(bp);
	}

	return 1;
#endif /* CONFIG_HAVE_HW_BREAKPOINT */

	if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT)
		return -EINVAL;

	if (child->thread.hw_brk.address)
		return -ENOSPC;

	if (!ppc_breakpoint_available())
		return -ENODEV;

	child->thread.hw_brk = brk;

	return 1;
#endif /* !CONFIG_PPC_ADV_DEBUG_DVCS */
}

Powered by blists - more mailing lists