Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Young <dyoung@redhat.com>
To: Matthew Garrett <matthewgarrett@google.com>
Cc: jmorris@namei.org, linux-security-module@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
	Josh Boyer <jwboyer@redhat.com>,
	bhe@redhat.com, kasong@redhat.com, Borislav Petkov <bp@suse.de>,
	David Howells <dhowells@redhat.com>,
	Matthew Garrett <mjg59@google.com>,
	Kees Cook <keescook@chromium.org>,
	linux-acpi@vger.kernel.org
Subject: Re: [PATCH V38 15/29] acpi: Ignore acpi_rsdp kernel param when the kernel has been locked down
Date: Wed, 14 Aug 2019 10:51:13 +0800
Message-ID: <20190814025113.GA7058@dhcp-128-65.nay.redhat.com> (raw)
In-Reply-To: <20190808000721.124691-16-matthewgarrett@google.com>

On 08/07/19 at 05:07pm, Matthew Garrett wrote:
> From: Josh Boyer <jwboyer@redhat.com>
> 
> This option allows userspace to pass the RSDP address to the kernel, which
> makes it possible for a user to modify the workings of hardware. Reject
> the option when the kernel is locked down. This requires some reworking
> of the existing RSDP command line logic, since the early boot code also
> makes use of a command-line passed RSDP when locating the SRAT table
> before the lockdown code has been initialised. This is achieved by
> separating the command line RSDP path in the early boot code from the
> generic RSDP path, and then copying the command line RSDP into boot
> params in the kernel proper if lockdown is not enabled. If lockdown is
> enabled and an RSDP is provided on the command line, this will only be
> used when parsing SRAT (which shouldn't permit kernel code execution)
> and will be ignored in the rest of the kernel.
> 
> (Modified by Matthew Garrett in order to handle the early boot RSDP
> environment)
> 
> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> cc: Dave Young <dyoung@redhat.com>
> cc: linux-acpi@vger.kernel.org
> ---
>  arch/x86/boot/compressed/acpi.c | 19 +++++++++++++------
>  arch/x86/include/asm/acpi.h     |  9 +++++++++
>  arch/x86/include/asm/x86_init.h |  2 ++
>  arch/x86/kernel/acpi/boot.c     |  5 +++++
>  arch/x86/kernel/x86_init.c      |  1 +
>  drivers/acpi/osl.c              | 14 +++++++++++++-
>  include/linux/acpi.h            |  6 ++++++
>  7 files changed, 49 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> index 15255f388a85..149795c369f2 100644
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -26,7 +26,7 @@ struct mem_vector immovable_mem[MAX_NUMNODES*2];
>   */
>  #define MAX_ADDR_LEN 19
>  
> -static acpi_physical_address get_acpi_rsdp(void)
> +static acpi_physical_address get_cmdline_acpi_rsdp(void)
>  {
>  	acpi_physical_address addr = 0;
>  
> @@ -278,10 +278,7 @@ acpi_physical_address get_rsdp_addr(void)
>  {
>  	acpi_physical_address pa;
>  
> -	pa = get_acpi_rsdp();
> -
> -	if (!pa)
> -		pa = boot_params->acpi_rsdp_addr;
> +	pa = boot_params->acpi_rsdp_addr;
>  
>  	/*
>  	 * Try to get EFI data from setup_data. This can happen when we're a
> @@ -311,7 +308,17 @@ static unsigned long get_acpi_srat_table(void)
>  	char arg[10];
>  	u8 *entry;
>  
> -	rsdp = (struct acpi_table_rsdp *)(long)boot_params->acpi_rsdp_addr;
> +	/*
> +	 * Check whether we were given an RSDP on the command line. We don't
> +	 * stash this in boot params because the kernel itself may have
> +	 * different ideas about whether to trust a command-line parameter.
> +	 */
> +	rsdp = (struct acpi_table_rsdp *)get_cmdline_acpi_rsdp();
> +
> +	if (!rsdp)
> +		rsdp = (struct acpi_table_rsdp *)(long)
> +			boot_params->acpi_rsdp_addr;
> +
>  	if (!rsdp)
>  		return 0;
>  
> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> index aac686e1e005..bc9693c9107e 100644
> --- a/arch/x86/include/asm/acpi.h
> +++ b/arch/x86/include/asm/acpi.h
> @@ -117,6 +117,12 @@ static inline bool acpi_has_cpu_in_madt(void)
>  	return !!acpi_lapic;
>  }
>  
> +#define ACPI_HAVE_ARCH_SET_ROOT_POINTER
> +static inline void acpi_arch_set_root_pointer(u64 addr)
> +{
> +	x86_init.acpi.set_root_pointer(addr);
> +}
> +
>  #define ACPI_HAVE_ARCH_GET_ROOT_POINTER
>  static inline u64 acpi_arch_get_root_pointer(void)
>  {
> @@ -125,6 +131,7 @@ static inline u64 acpi_arch_get_root_pointer(void)
>  
>  void acpi_generic_reduced_hw_init(void);
>  
> +void x86_default_set_root_pointer(u64 addr);
>  u64 x86_default_get_root_pointer(void);
>  
>  #else /* !CONFIG_ACPI */
> @@ -138,6 +145,8 @@ static inline void disable_acpi(void) { }
>  
>  static inline void acpi_generic_reduced_hw_init(void) { }
>  
> +static inline void x86_default_set_root_pointer(u64 addr) { }
> +
>  static inline u64 x86_default_get_root_pointer(void)
>  {
>  	return 0;
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index ac0934189017..19435858df5f 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -134,10 +134,12 @@ struct x86_hyper_init {
>  
>  /**
>   * struct x86_init_acpi - x86 ACPI init functions
> + * @set_root_poitner:		set RSDP address
>   * @get_root_pointer:		get RSDP address
>   * @reduced_hw_early_init:	hardware reduced platform early init
>   */
>  struct x86_init_acpi {
> +	void (*set_root_pointer)(u64 addr);
>  	u64 (*get_root_pointer)(void);
>  	void (*reduced_hw_early_init)(void);
>  };
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 17b33ef604f3..04205ce127a1 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -1760,6 +1760,11 @@ void __init arch_reserve_mem_area(acpi_physical_address addr, size_t size)
>  	e820__update_table_print();
>  }
>  
> +void x86_default_set_root_pointer(u64 addr)
> +{
> +	boot_params.acpi_rsdp_addr = addr;
> +}
> +
>  u64 x86_default_get_root_pointer(void)
>  {
>  	return boot_params.acpi_rsdp_addr;
> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> index 1bef687faf22..18a799c8fa28 100644
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -95,6 +95,7 @@ struct x86_init_ops x86_init __initdata = {
>  	},
>  
>  	.acpi = {
> +		.set_root_pointer	= x86_default_set_root_pointer,
>  		.get_root_pointer	= x86_default_get_root_pointer,
>  		.reduced_hw_early_init	= acpi_generic_reduced_hw_init,
>  	},
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 9c0edf2fc0dd..d43df3a3fa8d 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -26,6 +26,7 @@
>  #include <linux/list.h>
>  #include <linux/jiffies.h>
>  #include <linux/semaphore.h>
> +#include <linux/security.h>
>  
>  #include <asm/io.h>
>  #include <linux/uaccess.h>
> @@ -180,8 +181,19 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
>  	acpi_physical_address pa;
>  
>  #ifdef CONFIG_KEXEC
> -	if (acpi_rsdp)
> +	/*
> +	 * We may have been provided with an RSDP on the command line,
> +	 * but if a malicious user has done so they may be pointing us
> +	 * at modified ACPI tables that could alter kernel behaviour -
> +	 * so, we check the lockdown status before making use of
> +	 * it. If we trust it then also stash it in an architecture
> +	 * specific location (if appropriate) so it can be carried
> +	 * over further kexec()s.
> +	 */
> +	if (acpi_rsdp && !security_locked_down(LOCKDOWN_ACPI_TABLES)) {
> +		acpi_arch_set_root_pointer(acpi_rsdp);
>  		return acpi_rsdp;
> +	}
>  #endif
>  	pa = acpi_arch_get_root_pointer();
>  	if (pa)
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index e40e1e27ed8e..6b35f2f4cab3 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -643,6 +643,12 @@ bool acpi_gtdt_c3stop(int type);
>  int acpi_arch_timer_mem_init(struct arch_timer_mem *timer_mem, int *timer_count);
>  #endif
>  
> +#ifndef ACPI_HAVE_ARCH_SET_ROOT_POINTER
> +static inline void acpi_arch_set_root_pointer(u64 addr)
> +{
> +}
> +#endif
> +
>  #ifndef ACPI_HAVE_ARCH_GET_ROOT_POINTER
>  static inline u64 acpi_arch_get_root_pointer(void)
>  {
> -- 
> 2.22.0.770.g0f2c4a37fd-goog
> 

I'm not sure why this series get posted quickly again.  As for this
patch it was restructured according the early rsdp parsing code, so
personally I think keep the original Reviewed-by is questionable and it
needs another review.  But I did not find time to read it carefully.

Add several cc to void it slipped.

Thanks
Dave

  reply index

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-08  0:06 [PATCH V38 00/29] security: Add support for locking down the kernel Matthew Garrett
2019-08-08  0:06 ` [PATCH V38 01/29] security: Support early LSMs Matthew Garrett
2019-08-08  0:06 ` [PATCH V38 02/29] security: Add a "locked down" LSM hook Matthew Garrett
2019-08-08  0:06 ` [PATCH V38 03/29] security: Add a static lockdown policy LSM Matthew Garrett
2019-08-08  0:06 ` [PATCH V38 04/29] Enforce module signatures if the kernel is locked down Matthew Garrett
2019-08-08  0:06 ` [PATCH V38 05/29] Restrict /dev/{mem,kmem,port} when " Matthew Garrett
2019-08-08  0:06 ` [PATCH V38 06/29] kexec_load: Disable at runtime if " Matthew Garrett
2019-08-08  0:06 ` [PATCH V38 07/29] Copy secure_boot flag in boot params across kexec reboot Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 08/29] kexec_file: split KEXEC_VERIFY_SIG into KEXEC_SIG and KEXEC_SIG_FORCE Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 09/29] kexec_file: Restrict at runtime if the kernel is locked down Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 10/29] hibernate: Disable when " Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 11/29] PCI: Lock down BAR access " Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 12/29] x86: Lock down IO port " Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 13/29] x86/msr: Restrict MSR " Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 14/29] ACPI: Limit access to custom_method " Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 15/29] acpi: Ignore acpi_rsdp kernel param when the kernel has been " Matthew Garrett
2019-08-14  2:51   ` Dave Young [this message]
2019-08-14  7:26   ` Borislav Petkov
2019-08-14 17:14     ` Matthew Garrett
2019-08-14 17:47       ` Borislav Petkov
2019-08-14 18:02         ` Matthew Garrett
2019-08-14  7:28   ` Borislav Petkov
2019-08-08  0:07 ` [PATCH V38 16/29] acpi: Disable ACPI table override if the kernel is " Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 17/29] Prohibit PCMCIA CIS storage when " Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 18/29] Lock down TIOCSSERIAL Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 19/29] Lock down module params that specify hardware parameters (eg. ioport) Matthew Garrett
2019-08-08 11:12   ` Jessica Yu
2019-08-08 16:33     ` James Morris
2019-08-09 20:58     ` [PATCH V39] " Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 20/29] x86/mmiotrace: Lock down the testmmiotrace module Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 21/29] Lock down /proc/kcore Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 22/29] Lock down tracing and perf kprobes when in confidentiality mode Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 23/29] bpf: Restrict bpf when kernel lockdown is " Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 24/29] Lock down perf when " Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 25/29] kexec: Allow kexec_file() with appropriate IMA policy when locked down Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 26/29] debugfs: Restrict debugfs when the kernel is " Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 27/29] tracefs: Restrict tracefs " Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 28/29] efi: Restrict efivar_ssdt_load " Matthew Garrett
2019-08-08  0:07 ` [PATCH V38 29/29] lockdown: Print current->comm in restriction messages Matthew Garrett
2019-08-10  6:08 ` [PATCH V38 00/29] security: Add support for locking down the kernel James Morris
2019-08-12 17:06   ` Matthew Garrett
2019-08-12 17:39     ` James Morris
2019-08-12 22:29       ` James Morris

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=20190814025113.GA7058@dhcp-128-65.nay.redhat.com \
    --to=dyoung@redhat.com \
    --cc=bhe@redhat.com \
    --cc=bp@suse.de \
    --cc=dhowells@redhat.com \
    --cc=jmorris@namei.org \
    --cc=jwboyer@redhat.com \
    --cc=kasong@redhat.com \
    --cc=keescook@chromium.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=matthewgarrett@google.com \
    --cc=mjg59@google.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-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/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-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

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


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