Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
From: Michael Ellerman <mpe@ellerman.id.au>
To: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>,
	Nayna Jain <nayna@linux.vnet.ibm.com>,
	Nayna Jain <nayna@linux.ibm.com>,
	linuxppc-dev@ozlabs.org, linux-efi@vger.kernel.org,
	linux-integrity@vger.kernel.org
Cc: linux-kernel@vger.kernel.org,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Jeremy Kerr <jk@ozlabs.org>,
	Matthew Garret <matthew.garret@nebula.com>,
	Mimi Zohar <zohar@linux.ibm.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Claudio Carvalho <cclaudio@linux.ibm.com>,
	George Wilson <gcwilson@linux.ibm.com>,
	Elaine Palmer <erpalmer@us.ibm.com>,
	Eric Ricther <erichte@linux.ibm.com>,
	Oliver O'Halloran <oohall@gmail.com>,
	Prakhar Srivastava <prsriva02@gmail.com>
Subject: Re: [PATCH v9 2/8] powerpc/ima: add support to initialize ima policy rules
Date: Tue, 29 Oct 2019 10:42:32 +1100
Message-ID: <87lft4h1xz.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <482b2f08-f810-6ed0-4b32-0d5e64246ece@linux.microsoft.com>

Hi Lakshmi,

Lakshmi Ramasubramanian <nramas@linux.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 */
}


  reply index

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-24  3:47 [PATCH v9 0/8] powerpc: Enabling IMA arch specific secure boot policies Nayna Jain
2019-10-24  3:47 ` [PATCH v9 1/8] powerpc: detect the secure boot mode of the system Nayna Jain
2019-10-24 17:26   ` Lakshmi Ramasubramanian
2019-10-25 16:49     ` Nayna Jain
2019-10-24  3:47 ` [PATCH v9 2/8] powerpc/ima: add support to initialize ima policy rules Nayna Jain
2019-10-24 17:35   ` Lakshmi Ramasubramanian
2019-10-25 17:02     ` Nayna Jain
2019-10-25 18:03       ` Lakshmi Ramasubramanian
2019-10-28 23:42         ` Michael Ellerman [this message]
2019-10-26 23:52       ` Mimi Zohar
2019-10-28 11:54         ` Mimi Zohar
2019-10-24  3:47 ` [PATCH v9 3/8] powerpc: detect the trusted boot state of the system Nayna Jain
2019-10-24 17:38   ` Lakshmi Ramasubramanian
2019-10-25 16:50     ` Nayna Jain
2019-10-24  3:47 ` [PATCH v9 4/8] powerpc/ima: define trusted boot policy Nayna Jain
2019-10-24 17:40   ` Lakshmi Ramasubramanian
2019-10-24  3:47 ` [PATCH v9 5/8] ima: make process_buffer_measurement() generic Nayna Jain
2019-10-24 15:20   ` Lakshmi Ramasubramanian
2019-10-25 17:24     ` Nayna Jain
2019-10-25 17:32       ` Lakshmi Ramasubramanian
2019-10-27  0:13         ` Mimi Zohar
2019-10-30 15:22   ` Lakshmi Ramasubramanian
2019-10-30 16:35     ` Mimi Zohar
2019-10-24  3:47 ` [PATCH v9 6/8] certs: add wrapper function to check blacklisted binary hash Nayna Jain
2019-10-24  3:47 ` [PATCH v9 7/8] ima: check against blacklisted hashes for files with modsig Nayna Jain
2019-10-24 17:48   ` Lakshmi Ramasubramanian
2019-10-25 17:36     ` Nayna Jain
2019-10-24  3:47 ` [PATCH v9 8/8] powerpc/ima: update ima arch policy to check for blacklist Nayna Jain
2019-10-28 12:10 ` [PATCH v9 0/8] powerpc: Enabling IMA arch specific secure boot policies Mimi Zohar

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87lft4h1xz.fsf@mpe.ellerman.id.au \
    --to=mpe@ellerman.id.au \
    --cc=ard.biesheuvel@linaro.org \
    --cc=benh@kernel.crashing.org \
    --cc=cclaudio@linux.ibm.com \
    --cc=erichte@linux.ibm.com \
    --cc=erpalmer@us.ibm.com \
    --cc=gcwilson@linux.ibm.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jk@ozlabs.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=matthew.garret@nebula.com \
    --cc=nayna@linux.ibm.com \
    --cc=nayna@linux.vnet.ibm.com \
    --cc=nramas@linux.microsoft.com \
    --cc=oohall@gmail.com \
    --cc=paulus@samba.org \
    --cc=prsriva02@gmail.com \
    --cc=zohar@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git