Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread David Woodhouse 
On Fri, 2013-08-16 at 11:20 -0400, John W. Linville wrote:
 All,
 
 The Linux Foundation and The UEFI Forum are hosting a UEFI Plugfest
 event in New Orleans on September 19-20, 2013.  This event will run
 concurrent with Linux Plumbers Conference, just after LinuxCon at the
 Hyatt Regency New Orleans.

Hm. It would be really useful to have a kernel build option which
*disables* all the workarounds we've ever put in for broken firmware.

Every deviation from the spec (or common sense), however minor, should
show up as a clear failure. Even the ones we *have* been able to work
around, because we still want them *fixed*.

And there's a school of thought that says we should brick as many
Samsung machines as possible, if they actually turn up without having
fixed that. A stress test for the NV storage is actually a really good
idea...

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread Matthew Garrett 
On Mon, Aug 19, 2013 at 09:25:35AM +0100, David Woodhouse wrote:

 Every deviation from the spec (or common sense), however minor, should
 show up as a clear failure. Even the ones we *have* been able to work
 around, because we still want them *fixed*.

Why? It's not like we can ever stop carrying that code.

-- 
Matthew Garrett | mj...@srcf.ucam.org
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread Borislav Petkov 
On Mon, Aug 19, 2013 at 09:25:35AM +0100, David Woodhouse wrote:
 Hm. It would be really useful to have a kernel build option which
 *disables* all the workarounds we've ever put in for broken firmware.

Yeah, cool!

I wonder if we could reach a high double-digit percentage of machines
not booting/barfing on such a clean kernel.

While at it, can we please replace the fw with coreboot? :-)

 Every deviation from the spec (or common sense), however minor, should
 show up as a clear failure. Even the ones we *have* been able to work
 around, because we still want them *fixed*.
 
 And there's a school of thought that says we should brick as many
 Samsung machines as possible,

Yep, and there's the rooted secure boot asus f*ckup which we should also
advertize while booting, enabling people to use it:

https://www.blackhat.com/us-13/archives.html#Bulygin

/me LOLs ominously...

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread James Bottomley 
On Mon, 2013-08-19 at 13:55 +0100, Matthew Garrett wrote:
 On Mon, Aug 19, 2013 at 09:25:35AM +0100, David Woodhouse wrote:
 
  Every deviation from the spec (or common sense), however minor, should
  show up as a clear failure. Even the ones we *have* been able to work
  around, because we still want them *fixed*.
 
 Why? It's not like we can ever stop carrying that code.

The reason for doing it is that we have a buildable reference
implementation that's fully spec compliant we can then make the basis of
a test suite for UEFI.

I am worried about it from another angle, though: history has shown
we're not very good at maintaining configurations which we don't really
use ... since every distro will turn this off (or the workarounds on),
it's going to be a bit of work for someone to make sure it still
functions.

James


--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread Matthew Garrett 
On Mon, Aug 19, 2013 at 08:22:45AM -0700, James Bottomley wrote:
 On Mon, 2013-08-19 at 13:55 +0100, Matthew Garrett wrote:
  On Mon, Aug 19, 2013 at 09:25:35AM +0100, David Woodhouse wrote:
  
   Every deviation from the spec (or common sense), however minor, should
   show up as a clear failure. Even the ones we *have* been able to work
   around, because we still want them *fixed*.
  
  Why? It's not like we can ever stop carrying that code.
 
 The reason for doing it is that we have a buildable reference
 implementation that's fully spec compliant we can then make the basis of
 a test suite for UEFI.

And why's that a benefit? Nobody's ever going to be able to ship an OS 
that doesn't implement these workarounds - they're de-facto part of the 
spec. It'd make more sense to document them officially.

-- 
Matthew Garrett | mj...@srcf.ucam.org
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/10] Add option to automatically enforce module signatures when in Secure Boot mode

2013-08-19 Thread Matthew Garrett 
UEFI Secure Boot provides a mechanism for ensuring that the firmware will
only load signed bootloaders and kernels. Certain use cases may also
require that all kernel modules also be signed. Add a configuration option
that enforces this automatically when enabled.

Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 Documentation/x86/zero-page.txt   |  2 ++
 arch/x86/Kconfig  | 10 ++
 arch/x86/boot/compressed/eboot.c  | 33 +
 arch/x86/include/uapi/asm/bootparam.h |  3 ++-
 arch/x86/kernel/setup.c   |  6 ++
 include/linux/module.h|  6 ++
 kernel/module.c   |  7 +++
 7 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt
index 199f453..ec38acf 100644
--- a/Documentation/x86/zero-page.txt
+++ b/Documentation/x86/zero-page.txt
@@ -30,6 +30,8 @@ OffsetProto   NameMeaning
 1E9/001ALL eddbuf_entries  Number of entries in eddbuf (below)
 1EA/001ALL edd_mbr_sig_buf_entries Number of entries in 
edd_mbr_sig_buffer
(below)
+1EB/001ALL kbd_status  Numlock is enabled
+1EC/001ALL secure_boot Secure boot is enabled in the firmware
 1EF/001ALL sentinelUsed to detect broken bootloaders
 290/040ALL edd_mbr_sig_buffer EDD MBR signatures
 2D0/A00ALL e820_mapE820 memory map table
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b32ebf9..6a6c19b 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1581,6 +1581,16 @@ config EFI_STUB
 
  See Documentation/x86/efi-stub.txt for more information.
 
+config EFI_SECURE_BOOT_SIG_ENFORCE
+def_bool n
+   prompt Force module signing when UEFI Secure Boot is enabled
+   ---help---
+ UEFI Secure Boot provides a mechanism for ensuring that the
+ firmware will only load signed bootloaders and kernels. Certain
+ use cases may also require that all kernel modules also be signed.
+ Say Y here to automatically enable module signature enforcement
+ when a system boots with UEFI Secure Boot enabled.
+
 config SECCOMP
def_bool y
prompt Enable seccomp to safely compute untrusted bytecode
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index b7388a4..145294d 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -861,6 +861,37 @@ fail:
return status;
 }
 
+static int get_secure_boot(efi_system_table_t *_table)
+{
+   u8 sb, setup;
+   unsigned long datasize = sizeof(sb);
+   efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
+   efi_status_t status;
+
+   status = efi_call_phys5(sys_table-runtime-get_variable,
+   LSecureBoot, var_guid, NULL, datasize, sb);
+
+   if (status != EFI_SUCCESS)
+   return 0;
+
+   if (sb == 0)
+   return 0;
+
+
+   status = efi_call_phys5(sys_table-runtime-get_variable,
+   LSetupMode, var_guid, NULL, datasize,
+   setup);
+
+   if (status != EFI_SUCCESS)
+   return 0;
+
+   if (setup == 1)
+   return 0;
+
+   return 1;
+}
+
+
 /*
  * Because the x86 boot code expects to be passed a boot_params we
  * need to create one ourselves (usually the bootloader would create
@@ -1169,6 +1200,8 @@ struct boot_params *efi_main(void *handle, 
efi_system_table_t *_table,
if (sys_table-hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
goto fail;
 
+   boot_params-secure_boot = get_secure_boot(sys_table);
+
setup_graphics(boot_params);
 
setup_efi_pci(boot_params);
diff --git a/arch/x86/include/uapi/asm/bootparam.h 
b/arch/x86/include/uapi/asm/bootparam.h
index c15ddaf..d35da96 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -131,7 +131,8 @@ struct boot_params {
__u8  eddbuf_entries;   /* 0x1e9 */
__u8  edd_mbr_sig_buf_entries;  /* 0x1ea */
__u8  kbd_status;   /* 0x1eb */
-   __u8  _pad5[3]; /* 0x1ec */
+   __u8  secure_boot;  /* 0x1ec */
+   __u8  _pad5[2]; /* 0x1ec */
/*
 * The sentinel is set to a nonzero value (0xff) in header.S.
 *
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f8ec578..deeb7bc 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1129,6 +1129,12 @@ void __init setup_arch(char **cmdline_p)
 
io_delay_init();
 
+#ifdef CONFIG_EFI_SECURE_BOOT_SIG_ENFORCE
+   if (boot_params.secure_boot) {
+

[PATCH 03/10] x86: Lock down IO port access when module security is enabled

2013-08-19 Thread Matthew Garrett 
IO port access would permit users to gain access to PCI configuration
registers, which in turn (on a lot of hardware) give access to MMIO register
space. This would potentially permit root to trigger arbitrary DMA, so lock
it down by default.

Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 arch/x86/kernel/ioport.c | 5 +++--
 drivers/char/mem.c   | 4 
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
index 4ddaf66..00b4403 100644
--- a/arch/x86/kernel/ioport.c
+++ b/arch/x86/kernel/ioport.c
@@ -15,6 +15,7 @@
 #include linux/thread_info.h
 #include linux/syscalls.h
 #include linux/bitmap.h
+#include linux/module.h
 #include asm/syscalls.h
 
 /*
@@ -28,7 +29,7 @@ asmlinkage long sys_ioperm(unsigned long from, unsigned long 
num, int turn_on)
 
if ((from + num = from) || (from + num  IO_BITMAP_BITS))
return -EINVAL;
-   if (turn_on  !capable(CAP_SYS_RAWIO))
+   if (turn_on  (!capable(CAP_SYS_RAWIO) || secure_modules()))
return -EPERM;
 
/*
@@ -103,7 +104,7 @@ SYSCALL_DEFINE1(iopl, unsigned int, level)
return -EINVAL;
/* Trying to gain more privileges? */
if (level  old) {
-   if (!capable(CAP_SYS_RAWIO))
+   if (!capable(CAP_SYS_RAWIO) || secure_modules())
return -EPERM;
}
regs-flags = (regs-flags  ~X86_EFLAGS_IOPL) | (level  12);
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index f895a8c..1af8664 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -28,6 +28,7 @@
 #include linux/export.h
 #include linux/io.h
 #include linux/aio.h
+#include linux/module.h
 
 #include asm/uaccess.h
 
@@ -563,6 +564,9 @@ static ssize_t write_port(struct file *file, const char 
__user *buf,
unsigned long i = *ppos;
const char __user *tmp = buf;
 
+   if (secure_modules())
+   return -EPERM;
+
if (!access_ok(VERIFY_READ, buf, count))
return -EFAULT;
while (count--  0  i  65536) {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/10] kexec: Disable at runtime if the kernel enforces module loading restrictions

2013-08-19 Thread Matthew Garrett 
kexec permits the loading and execution of arbitrary code in ring 0, which
is something that module signing enforcement is meant to prevent. It makes
sense to disable kexec in this situation.

Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 kernel/kexec.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index 59f7b55..1a7690f 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -32,6 +32,7 @@
 #include linux/vmalloc.h
 #include linux/swap.h
 #include linux/syscore_ops.h
+#include linux/module.h
 
 #include asm/page.h
 #include asm/uaccess.h
@@ -1645,6 +1646,9 @@ int kernel_kexec(void)
goto Unlock;
}
 
+   if (secure_modules())
+   return -EPERM;
+
 #ifdef CONFIG_KEXEC_JUMP
if (kexec_image-preserve_context) {
lock_system_sleep();
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/10] acpi: Ignore acpi_rsdp kernel parameter when module loading is restricted

2013-08-19 Thread Matthew Garrett 
From: Josh Boyer jwbo...@redhat.com

This option allows userspace to pass the RSDP address to the kernel, which
makes it possible for a user to circumvent any restrictions imposed on
loading modules. Disable it in that case.

Signed-off-by: Josh Boyer jwbo...@redhat.com
---
 drivers/acpi/osl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 6ab2c35..e4c4410 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -45,6 +45,7 @@
 #include linux/list.h
 #include linux/jiffies.h
 #include linux/semaphore.h
+#include linux/module.h
 
 #include asm/io.h
 #include asm/uaccess.h
@@ -245,7 +246,7 @@ early_param(acpi_rsdp, setup_acpi_rsdp);
 acpi_physical_address __init acpi_os_get_root_pointer(void)
 {
 #ifdef CONFIG_KEXEC
-   if (acpi_rsdp)
+   if (acpi_rsdp  !secure_modules())
return acpi_rsdp;
 #endif
 
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/10] Add secure_modules() call

2013-08-19 Thread Matthew Garrett 
Provide a single call to allow kernel code to determine whether the system
has been configured to either disable module loading entirely or to load
only modules signed with a trusted key.

Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 include/linux/module.h | 7 +++
 kernel/module.c| 9 +
 2 files changed, 16 insertions(+)

diff --git a/include/linux/module.h b/include/linux/module.h
index 46f1ea0..0c266b2 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -509,6 +509,8 @@ int unregister_module_notifier(struct notifier_block * nb);
 
 extern void print_modules(void);
 
+extern bool secure_modules(void);
+
 #else /* !CONFIG_MODULES... */
 
 /* Given an address, look for it in the exception tables. */
@@ -619,6 +621,11 @@ static inline int unregister_module_notifier(struct 
notifier_block * nb)
 static inline void print_modules(void)
 {
 }
+
+static inline bool secure_modules(void)
+{
+   return false;
+}
 #endif /* CONFIG_MODULES */
 
 #ifdef CONFIG_SYSFS
diff --git a/kernel/module.c b/kernel/module.c
index 2069158..801021e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3852,3 +3852,12 @@ void module_layout(struct module *mod,
 }
 EXPORT_SYMBOL(module_layout);
 #endif
+
+bool secure_modules(void)
+{
+#ifdef CONFIG_MODULE_SIG
+   return (sig_enforce || modules_disabled);
+#else
+   return modules_disabled;
+#endif
+}
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/10] Restrict /dev/mem and /dev/kmem when module loading is restricted

2013-08-19 Thread Matthew Garrett 
Allowing users to write to address space makes it possible for the kernel
to be subverted, avoiding module loading restrictions. Prevent this when
any restrictions have been imposed on loading modules.

Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 drivers/char/mem.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 1af8664..61406c8 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -159,6 +159,9 @@ static ssize_t write_mem(struct file *file, const char 
__user *buf,
unsigned long copied;
void *ptr;
 
+   if (secure_modules())
+   return -EPERM;
+
if (!valid_phys_addr_range(p, count))
return -EFAULT;
 
@@ -497,6 +500,9 @@ static ssize_t write_kmem(struct file *file, const char 
__user *buf,
char *kbuf; /* k-addr because vwrite() takes vmlist_lock rwlock */
int err = 0;
 
+   if (secure_modules())
+   return -EPERM;
+
if (p  (unsigned long) high_memory) {
unsigned long to_write = min_t(unsigned long, count,
   (unsigned long)high_memory - p);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

2013-08-19 Thread Matthew Garrett 
We have no way of validating what all of the Asus WMI methods do on a
given machine, and there's a risk that some will allow hardware state to
be manipulated in such a way that arbitrary code can be executed in the
kernel, circumventing module loading restrictions. Prevent that if any of
these features are enabled.

Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 drivers/platform/x86/asus-wmi.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 19c313b..8105530 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -1618,6 +1618,9 @@ static int show_dsts(struct seq_file *m, void *data)
int err;
u32 retval = -1;
 
+   if (secure_modules())
+   return -EPERM;
+
err = asus_wmi_get_devstate(asus, asus-debug.dev_id, retval);
 
if (err  0)
@@ -1634,6 +1637,9 @@ static int show_devs(struct seq_file *m, void *data)
int err;
u32 retval = -1;
 
+   if (!capable(CAP_COMPROMISE_KERNEL))
+   return -EPERM;
+
err = asus_wmi_set_devstate(asus-debug.dev_id, asus-debug.ctrl_param,
retval);
 
@@ -1658,6 +1664,9 @@ static int show_call(struct seq_file *m, void *data)
union acpi_object *obj;
acpi_status status;
 
+   if (!capable(CAP_COMPROMISE_KERNEL))
+   return -EPERM;
+
status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID,
 1, asus-debug.method_id,
 input, output);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/10] PCI: Lock down BAR access when module security is enabled

2013-08-19 Thread Matthew Garrett 
Any hardware that can potentially generate DMA has to be locked down from
userspace in order to avoid it being possible for an attacker to modify
kernel code, allowing them to circumvent disabled module loading or module
signing. Default to paranoid - in future we can potentially relax this for
sufficiently IOMMU-isolated devices.

Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 drivers/pci/pci-sysfs.c | 10 ++
 drivers/pci/proc.c  |  8 +++-
 drivers/pci/syscall.c   |  3 ++-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index c0dbe1f..cd4e35f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -29,6 +29,7 @@
 #include linux/slab.h
 #include linux/vgaarb.h
 #include linux/pm_runtime.h
+#include linux/module.h
 #include pci.h
 
 static int sysfs_initialized;  /* = 0 */
@@ -624,6 +625,9 @@ pci_write_config(struct file* filp, struct kobject *kobj,
loff_t init_off = off;
u8 *data = (u8*) buf;
 
+   if (secure_modules())
+   return -EPERM;
+
if (off  dev-cfg_size)
return 0;
if (off + count  dev-cfg_size) {
@@ -930,6 +934,9 @@ pci_mmap_resource(struct kobject *kobj, struct 
bin_attribute *attr,
resource_size_t start, end;
int i;
 
+   if (secure_modules())
+   return -EPERM;
+
for (i = 0; i  PCI_ROM_RESOURCE; i++)
if (res == pdev-resource[i])
break;
@@ -1037,6 +1044,9 @@ pci_write_resource_io(struct file *filp, struct kobject 
*kobj,
  struct bin_attribute *attr, char *buf,
  loff_t off, size_t count)
 {
+   if (secure_modules())
+   return -EPERM;
+
return pci_resource_io(filp, kobj, attr, buf, off, count, true);
 }
 
diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index cdc7836..e3d498b 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -117,6 +117,9 @@ proc_bus_pci_write(struct file *file, const char __user 
*buf, size_t nbytes, lof
int size = dev-cfg_size;
int cnt;
 
+   if (secure_modules())
+   return -EPERM;
+
if (pos = size)
return 0;
if (nbytes = size)
@@ -196,6 +199,9 @@ static long proc_bus_pci_ioctl(struct file *file, unsigned 
int cmd,
 #endif /* HAVE_PCI_MMAP */
int ret = 0;
 
+   if (secure_modules())
+   return -EPERM;
+
switch (cmd) {
case PCIIOC_CONTROLLER:
ret = pci_domain_nr(dev-bus);
@@ -234,7 +240,7 @@ static int proc_bus_pci_mmap(struct file *file, struct 
vm_area_struct *vma)
struct pci_filp_private *fpriv = file-private_data;
int i, ret;
 
-   if (!capable(CAP_SYS_RAWIO))
+   if (!capable(CAP_SYS_RAWIO) || secure_modules())
return -EPERM;
 
/* Make sure the caller is mapping a real resource for this device */
diff --git a/drivers/pci/syscall.c b/drivers/pci/syscall.c
index e1c1ec5..bffbf71 100644
--- a/drivers/pci/syscall.c
+++ b/drivers/pci/syscall.c
@@ -10,6 +10,7 @@
 #include linux/errno.h
 #include linux/pci.h
 #include linux/syscalls.h
+#include linux/module.h
 #include asm/uaccess.h
 #include pci.h
 
@@ -92,7 +93,7 @@ SYSCALL_DEFINE5(pciconfig_write, unsigned long, bus, unsigned 
long, dfn,
u32 dword;
int err = 0;
 
-   if (!capable(CAP_SYS_ADMIN))
+   if (!capable(CAP_SYS_ADMIN) || secure_modules())
return -EPERM;
 
dev = pci_get_bus_and_slot(bus, dfn);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/10] ACPI: Limit access to custom_method

2013-08-19 Thread Matthew Garrett 
custom_method effectively allows arbitrary access to system memory, making
it possible for an attacker to circumvent restrictions on module loading.
Disable it if any such restrictions have been enabled.

Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 drivers/acpi/custom_method.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
index 12b62f2..55a013f 100644
--- a/drivers/acpi/custom_method.c
+++ b/drivers/acpi/custom_method.c
@@ -7,6 +7,7 @@
 #include linux/kernel.h
 #include linux/uaccess.h
 #include linux/debugfs.h
+#include linux/module.h
 #include acpi/acpi_drivers.h
 
 #include internal.h
@@ -29,6 +30,9 @@ static ssize_t cm_write(struct file *file, const char __user 
* user_buf,
struct acpi_table_header table;
acpi_status status;
 
+   if (secure_modules())
+   return -EPERM;
+
if (!(*ppos)) {
/* parse the table header to get the table length */
if (count = sizeof(struct acpi_table_header))
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

2013-08-19 Thread Kees Cook 
On Mon, Aug 19, 2013 at 9:10 AM, Matthew Garrett
matthew.garr...@nebula.com wrote:
 We have no way of validating what all of the Asus WMI methods do on a
 given machine, and there's a risk that some will allow hardware state to
 be manipulated in such a way that arbitrary code can be executed in the
 kernel, circumventing module loading restrictions. Prevent that if any of
 these features are enabled.

 Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
 ---
  drivers/platform/x86/asus-wmi.c | 9 +
  1 file changed, 9 insertions(+)

 diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
 index 19c313b..8105530 100644
 --- a/drivers/platform/x86/asus-wmi.c
 +++ b/drivers/platform/x86/asus-wmi.c
 @@ -1618,6 +1618,9 @@ static int show_dsts(struct seq_file *m, void *data)
 int err;
 u32 retval = -1;

 +   if (secure_modules())
 +   return -EPERM;
 +
 err = asus_wmi_get_devstate(asus, asus-debug.dev_id, retval);

 if (err  0)
 @@ -1634,6 +1637,9 @@ static int show_devs(struct seq_file *m, void *data)
 int err;
 u32 retval = -1;

 +   if (!capable(CAP_COMPROMISE_KERNEL))

Looks like this and the next chunk weren't changed to the
secure_modules() API...

-Kees

 +   return -EPERM;
 +
 err = asus_wmi_set_devstate(asus-debug.dev_id, 
 asus-debug.ctrl_param,
 retval);

 @@ -1658,6 +1664,9 @@ static int show_call(struct seq_file *m, void *data)
 union acpi_object *obj;
 acpi_status status;

 +   if (!capable(CAP_COMPROMISE_KERNEL))
 +   return -EPERM;
 +
 status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID,
  1, asus-debug.method_id,
  input, output);
 --
 1.8.3.1




-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread James Bottomley 
On Mon, 2013-08-19 at 17:00 +0100, Matthew Garrett wrote:
 On Mon, Aug 19, 2013 at 08:22:45AM -0700, James Bottomley wrote:
  On Mon, 2013-08-19 at 13:55 +0100, Matthew Garrett wrote:
   On Mon, Aug 19, 2013 at 09:25:35AM +0100, David Woodhouse wrote:
   
Every deviation from the spec (or common sense), however minor, should
show up as a clear failure. Even the ones we *have* been able to work
around, because we still want them *fixed*.
   
   Why? It's not like we can ever stop carrying that code.
  
  The reason for doing it is that we have a buildable reference
  implementation that's fully spec compliant we can then make the basis of
  a test suite for UEFI.
 
 And why's that a benefit? Nobody's ever going to be able to ship an OS 
 that doesn't implement these workarounds - they're de-facto part of the 
 spec. It'd make more sense to document them officially.

The object of having a test suite conform to the spec is not to
perpetuate the cockups that occurred in round one of the implementation
and to force everyone to pay closer attention to what the spec says.
Otherwise the amount of workarounds is just going to grow without
bounds.

James


--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

2013-08-19 Thread Matthew Garrett 
On Mon, 2013-08-19 at 09:20 -0700, Kees Cook wrote:

 Looks like this and the next chunk weren't changed to the
 secure_modules() API...

Bother. Yeah, looks like my test config had this left out.

-- 
Matthew Garrett matthew.garr...@nebula.com
N�r��yb�X��ǧv�^�)޺{.n�+{�y^n�r���z���h����G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�

Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread Matthew Garrett 
On Mon, Aug 19, 2013 at 10:02:55AM -0700, James Bottomley wrote:

 The object of having a test suite conform to the spec is not to
 perpetuate the cockups that occurred in round one of the implementation
 and to force everyone to pay closer attention to what the spec says.
 Otherwise the amount of workarounds is just going to grow without
 bounds.

There's a benefit in having a test suite that prevents new errors from 
being introduced, but there's no benefit in failing on errors that we 
have to work around anyway. We have the code. We're never going to be 
able to remove the code.

-- 
Matthew Garrett | mj...@srcf.ucam.org
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/10] Add additional security checks when module loading is restricted

2013-08-19 Thread Matthew Garrett 
We have two in-kernel mechanisms for restricting module loading - disabling
it entirely, or limiting it to the loading of modules signed with a trusted
key. These can both be configured in such a way that even root is unable to
relax the restrictions.

However, right now, there's several other straightforward ways for root to
modify running kernel code. At the most basic level these allow root to
reset the configuration such that modules can be loaded again, rendering
the existing restrictions useless.

This patchset adds additional restrictions to various kernel entry points
that would otherwise make it straightforward for root to disable enforcement
of module loading restrictions. It also provides a patch that allows the
kernel to be configured such that module signing will be automatically
enabled when the system is booting via UEFI Secure Boot, allowing a stronger
guarantee of kernel integrity.

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/10] Add additional security checks when module loading is restricted

2013-08-19 Thread Kees Cook 
On Mon, Aug 19, 2013 at 10:26 AM, Matthew Garrett
matthew.garr...@nebula.com wrote:
 We have two in-kernel mechanisms for restricting module loading - disabling
 it entirely, or limiting it to the loading of modules signed with a trusted
 key. These can both be configured in such a way that even root is unable to
 relax the restrictions.

 However, right now, there's several other straightforward ways for root to
 modify running kernel code. At the most basic level these allow root to
 reset the configuration such that modules can be loaded again, rendering
 the existing restrictions useless.

 This patchset adds additional restrictions to various kernel entry points
 that would otherwise make it straightforward for root to disable enforcement
 of module loading restrictions. It also provides a patch that allows the
 kernel to be configured such that module signing will be automatically
 enabled when the system is booting via UEFI Secure Boot, allowing a stronger
 guarantee of kernel integrity.

I like this approach of attaching it to the module loading logic.
Given the LSM hook for loading modules, I think I'd like to add a hook
for the secure_modules() check so that an LSM can respond secure as
well, if it doing module loading mediation (for example, in the case
of Chrome OS's modules-only-from-rootfs). I'll work up a patch...

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 02/10] PCI: Lock down BAR access when module security is enabled

2013-08-19 Thread Matthew Garrett 
Any hardware that can potentially generate DMA has to be locked down from
userspace in order to avoid it being possible for an attacker to modify
kernel code, allowing them to circumvent disabled module loading or module
signing. Default to paranoid - in future we can potentially relax this for
sufficiently IOMMU-isolated devices.

Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 drivers/pci/pci-sysfs.c | 10 ++
 drivers/pci/proc.c  |  8 +++-
 drivers/pci/syscall.c   |  3 ++-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index c0dbe1f..cd4e35f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -29,6 +29,7 @@
 #include linux/slab.h
 #include linux/vgaarb.h
 #include linux/pm_runtime.h
+#include linux/module.h
 #include pci.h
 
 static int sysfs_initialized;  /* = 0 */
@@ -624,6 +625,9 @@ pci_write_config(struct file* filp, struct kobject *kobj,
loff_t init_off = off;
u8 *data = (u8*) buf;
 
+   if (secure_modules())
+   return -EPERM;
+
if (off  dev-cfg_size)
return 0;
if (off + count  dev-cfg_size) {
@@ -930,6 +934,9 @@ pci_mmap_resource(struct kobject *kobj, struct 
bin_attribute *attr,
resource_size_t start, end;
int i;
 
+   if (secure_modules())
+   return -EPERM;
+
for (i = 0; i  PCI_ROM_RESOURCE; i++)
if (res == pdev-resource[i])
break;
@@ -1037,6 +1044,9 @@ pci_write_resource_io(struct file *filp, struct kobject 
*kobj,
  struct bin_attribute *attr, char *buf,
  loff_t off, size_t count)
 {
+   if (secure_modules())
+   return -EPERM;
+
return pci_resource_io(filp, kobj, attr, buf, off, count, true);
 }
 
diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index cdc7836..e3d498b 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -117,6 +117,9 @@ proc_bus_pci_write(struct file *file, const char __user 
*buf, size_t nbytes, lof
int size = dev-cfg_size;
int cnt;
 
+   if (secure_modules())
+   return -EPERM;
+
if (pos = size)
return 0;
if (nbytes = size)
@@ -196,6 +199,9 @@ static long proc_bus_pci_ioctl(struct file *file, unsigned 
int cmd,
 #endif /* HAVE_PCI_MMAP */
int ret = 0;
 
+   if (secure_modules())
+   return -EPERM;
+
switch (cmd) {
case PCIIOC_CONTROLLER:
ret = pci_domain_nr(dev-bus);
@@ -234,7 +240,7 @@ static int proc_bus_pci_mmap(struct file *file, struct 
vm_area_struct *vma)
struct pci_filp_private *fpriv = file-private_data;
int i, ret;
 
-   if (!capable(CAP_SYS_RAWIO))
+   if (!capable(CAP_SYS_RAWIO) || secure_modules())
return -EPERM;
 
/* Make sure the caller is mapping a real resource for this device */
diff --git a/drivers/pci/syscall.c b/drivers/pci/syscall.c
index e1c1ec5..bffbf71 100644
--- a/drivers/pci/syscall.c
+++ b/drivers/pci/syscall.c
@@ -10,6 +10,7 @@
 #include linux/errno.h
 #include linux/pci.h
 #include linux/syscalls.h
+#include linux/module.h
 #include asm/uaccess.h
 #include pci.h
 
@@ -92,7 +93,7 @@ SYSCALL_DEFINE5(pciconfig_write, unsigned long, bus, unsigned 
long, dfn,
u32 dword;
int err = 0;
 
-   if (!capable(CAP_SYS_ADMIN))
+   if (!capable(CAP_SYS_ADMIN) || secure_modules())
return -EPERM;
 
dev = pci_get_bus_and_slot(bus, dfn);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 08/10] kexec: Disable at runtime if the kernel enforces module loading restrictions

2013-08-19 Thread Matthew Garrett 
kexec permits the loading and execution of arbitrary code in ring 0, which
is something that module signing enforcement is meant to prevent. It makes
sense to disable kexec in this situation.

Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 kernel/kexec.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index 59f7b55..1a7690f 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -32,6 +32,7 @@
 #include linux/vmalloc.h
 #include linux/swap.h
 #include linux/syscore_ops.h
+#include linux/module.h
 
 #include asm/page.h
 #include asm/uaccess.h
@@ -1645,6 +1646,9 @@ int kernel_kexec(void)
goto Unlock;
}
 
+   if (secure_modules())
+   return -EPERM;
+
 #ifdef CONFIG_KEXEC_JUMP
if (kexec_image-preserve_context) {
lock_system_sleep();
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 07/10] acpi: Ignore acpi_rsdp kernel parameter when module loading is restricted

2013-08-19 Thread Matthew Garrett 
From: Josh Boyer jwbo...@redhat.com

This option allows userspace to pass the RSDP address to the kernel, which
makes it possible for a user to circumvent any restrictions imposed on
loading modules. Disable it in that case.

Signed-off-by: Josh Boyer jwbo...@redhat.com
---
 drivers/acpi/osl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 6ab2c35..e4c4410 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -45,6 +45,7 @@
 #include linux/list.h
 #include linux/jiffies.h
 #include linux/semaphore.h
+#include linux/module.h
 
 #include asm/io.h
 #include asm/uaccess.h
@@ -245,7 +246,7 @@ early_param(acpi_rsdp, setup_acpi_rsdp);
 acpi_physical_address __init acpi_os_get_root_pointer(void)
 {
 #ifdef CONFIG_KEXEC
-   if (acpi_rsdp)
+   if (acpi_rsdp  !secure_modules())
return acpi_rsdp;
 #endif
 
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 06/10] Restrict /dev/mem and /dev/kmem when module loading is restricted

2013-08-19 Thread Matthew Garrett 
Allowing users to write to address space makes it possible for the kernel
to be subverted, avoiding module loading restrictions. Prevent this when
any restrictions have been imposed on loading modules.

Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 drivers/char/mem.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 1af8664..61406c8 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -159,6 +159,9 @@ static ssize_t write_mem(struct file *file, const char 
__user *buf,
unsigned long copied;
void *ptr;
 
+   if (secure_modules())
+   return -EPERM;
+
if (!valid_phys_addr_range(p, count))
return -EFAULT;
 
@@ -497,6 +500,9 @@ static ssize_t write_kmem(struct file *file, const char 
__user *buf,
char *kbuf; /* k-addr because vwrite() takes vmlist_lock rwlock */
int err = 0;
 
+   if (secure_modules())
+   return -EPERM;
+
if (p  (unsigned long) high_memory) {
unsigned long to_write = min_t(unsigned long, count,
   (unsigned long)high_memory - p);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 09/10] x86: Restrict MSR access when module loading is restricted

2013-08-19 Thread Matthew Garrett 
Writing to MSRs should not be allowed if module loading is restricted,
since it could lead to execution of arbitrary code in kernel mode. Based
on a patch by Kees Cook.

Cc: Kees Cook keesc...@chromium.org
Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 arch/x86/kernel/msr.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
index 88458fa..d08f7e3 100644
--- a/arch/x86/kernel/msr.c
+++ b/arch/x86/kernel/msr.c
@@ -103,6 +103,9 @@ static ssize_t msr_write(struct file *file, const char 
__user *buf,
int err = 0;
ssize_t bytes = 0;
 
+   if (secure_modules())
+   return -EPERM;
+
if (count % 8)
return -EINVAL; /* Invalid chunk size */
 
@@ -150,6 +153,10 @@ static long msr_ioctl(struct file *file, unsigned int ioc, 
unsigned long arg)
err = -EBADF;
break;
}
+   if (secure_modules()) {
+   err = -EPERM;
+   break;
+   }
if (copy_from_user(regs, uregs, sizeof regs)) {
err = -EFAULT;
break;
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread James Bottomley 
On Mon, 2013-08-19 at 18:21 +0100, Matthew Garrett wrote:
 On Mon, Aug 19, 2013 at 10:02:55AM -0700, James Bottomley wrote:
 
  The object of having a test suite conform to the spec is not to
  perpetuate the cockups that occurred in round one of the implementation
  and to force everyone to pay closer attention to what the spec says.
  Otherwise the amount of workarounds is just going to grow without
  bounds.
 
 There's a benefit in having a test suite that prevents new errors from 
 being introduced, but there's no benefit in failing on errors that we 
 have to work around anyway. We have the code. We're never going to be 
 able to remove the code.

It's not about us removing the code, it's about us having an accurate
compliance test.  There are two reasons for having a fully correct
compliance test

 1. Our work arounds have unintended consequences which have knock
on effects which mean that you don't know if a test failure is
real or an unintended consequence of a work around.
 2. New features in specs tend to build on previous features, so
we're going to have a hard time constructing accurate tests for
layered features where we've done a work around for the base
feature.

James




--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 03/10] x86: Lock down IO port access when module security is enabled

2013-08-19 Thread Matthew Garrett 
IO port access would permit users to gain access to PCI configuration
registers, which in turn (on a lot of hardware) give access to MMIO register
space. This would potentially permit root to trigger arbitrary DMA, so lock
it down by default.

Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 arch/x86/kernel/ioport.c | 5 +++--
 drivers/char/mem.c   | 4 
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
index 4ddaf66..00b4403 100644
--- a/arch/x86/kernel/ioport.c
+++ b/arch/x86/kernel/ioport.c
@@ -15,6 +15,7 @@
 #include linux/thread_info.h
 #include linux/syscalls.h
 #include linux/bitmap.h
+#include linux/module.h
 #include asm/syscalls.h
 
 /*
@@ -28,7 +29,7 @@ asmlinkage long sys_ioperm(unsigned long from, unsigned long 
num, int turn_on)
 
if ((from + num = from) || (from + num  IO_BITMAP_BITS))
return -EINVAL;
-   if (turn_on  !capable(CAP_SYS_RAWIO))
+   if (turn_on  (!capable(CAP_SYS_RAWIO) || secure_modules()))
return -EPERM;
 
/*
@@ -103,7 +104,7 @@ SYSCALL_DEFINE1(iopl, unsigned int, level)
return -EINVAL;
/* Trying to gain more privileges? */
if (level  old) {
-   if (!capable(CAP_SYS_RAWIO))
+   if (!capable(CAP_SYS_RAWIO) || secure_modules())
return -EPERM;
}
regs-flags = (regs-flags  ~X86_EFLAGS_IOPL) | (level  12);
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index f895a8c..1af8664 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -28,6 +28,7 @@
 #include linux/export.h
 #include linux/io.h
 #include linux/aio.h
+#include linux/module.h
 
 #include asm/uaccess.h
 
@@ -563,6 +564,9 @@ static ssize_t write_port(struct file *file, const char 
__user *buf,
unsigned long i = *ppos;
const char __user *tmp = buf;
 
+   if (secure_modules())
+   return -EPERM;
+
if (!access_ok(VERIFY_READ, buf, count))
return -EFAULT;
while (count--  0  i  65536) {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread Matthew Garrett 
On Mon, Aug 19, 2013 at 10:38:46AM -0700, James Bottomley wrote:

 It's not about us removing the code, it's about us having an accurate
 compliance test.  There are two reasons for having a fully correct
 compliance test
 
  1. Our work arounds have unintended consequences which have knock
 on effects which mean that you don't know if a test failure is
 real or an unintended consequence of a work around.

It doesn't matter. If a platform is supposed to run Linux 3.6 then it 
has to run Linux 3.6 regardless of whether or not the failure is due to 
a firmware bug or a bug in the kernel. The platform vendor will be 
obliged to fix it in the firmware no matter what the test suite says.

  2. New features in specs tend to build on previous features, so
 we're going to have a hard time constructing accurate tests for
 layered features where we've done a work around for the base
 feature.

Which is easily rectified if the specification is modified to describe 
reality instead.

-- 
Matthew Garrett | mj...@srcf.ucam.org
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 10/10] Add option to automatically enforce module signatures when in Secure Boot mode

2013-08-19 Thread Matthew Garrett 
UEFI Secure Boot provides a mechanism for ensuring that the firmware will
only load signed bootloaders and kernels. Certain use cases may also
require that all kernel modules also be signed. Add a configuration option
that enforces this automatically when enabled.

Signed-off-by: Matthew Garrett matthew.garr...@nebula.com
---
 Documentation/x86/zero-page.txt   |  2 ++
 arch/x86/Kconfig  | 10 ++
 arch/x86/boot/compressed/eboot.c  | 33 +
 arch/x86/include/uapi/asm/bootparam.h |  3 ++-
 arch/x86/kernel/setup.c   |  6 ++
 include/linux/module.h|  6 ++
 kernel/module.c   |  7 +++
 7 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt
index 199f453..ec38acf 100644
--- a/Documentation/x86/zero-page.txt
+++ b/Documentation/x86/zero-page.txt
@@ -30,6 +30,8 @@ OffsetProto   NameMeaning
 1E9/001ALL eddbuf_entries  Number of entries in eddbuf (below)
 1EA/001ALL edd_mbr_sig_buf_entries Number of entries in 
edd_mbr_sig_buffer
(below)
+1EB/001ALL kbd_status  Numlock is enabled
+1EC/001ALL secure_boot Secure boot is enabled in the firmware
 1EF/001ALL sentinelUsed to detect broken bootloaders
 290/040ALL edd_mbr_sig_buffer EDD MBR signatures
 2D0/A00ALL e820_mapE820 memory map table
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b32ebf9..6a6c19b 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1581,6 +1581,16 @@ config EFI_STUB
 
  See Documentation/x86/efi-stub.txt for more information.
 
+config EFI_SECURE_BOOT_SIG_ENFORCE
+def_bool n
+   prompt Force module signing when UEFI Secure Boot is enabled
+   ---help---
+ UEFI Secure Boot provides a mechanism for ensuring that the
+ firmware will only load signed bootloaders and kernels. Certain
+ use cases may also require that all kernel modules also be signed.
+ Say Y here to automatically enable module signature enforcement
+ when a system boots with UEFI Secure Boot enabled.
+
 config SECCOMP
def_bool y
prompt Enable seccomp to safely compute untrusted bytecode
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index b7388a4..145294d 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -861,6 +861,37 @@ fail:
return status;
 }
 
+static int get_secure_boot(efi_system_table_t *_table)
+{
+   u8 sb, setup;
+   unsigned long datasize = sizeof(sb);
+   efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
+   efi_status_t status;
+
+   status = efi_call_phys5(sys_table-runtime-get_variable,
+   LSecureBoot, var_guid, NULL, datasize, sb);
+
+   if (status != EFI_SUCCESS)
+   return 0;
+
+   if (sb == 0)
+   return 0;
+
+
+   status = efi_call_phys5(sys_table-runtime-get_variable,
+   LSetupMode, var_guid, NULL, datasize,
+   setup);
+
+   if (status != EFI_SUCCESS)
+   return 0;
+
+   if (setup == 1)
+   return 0;
+
+   return 1;
+}
+
+
 /*
  * Because the x86 boot code expects to be passed a boot_params we
  * need to create one ourselves (usually the bootloader would create
@@ -1169,6 +1200,8 @@ struct boot_params *efi_main(void *handle, 
efi_system_table_t *_table,
if (sys_table-hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
goto fail;
 
+   boot_params-secure_boot = get_secure_boot(sys_table);
+
setup_graphics(boot_params);
 
setup_efi_pci(boot_params);
diff --git a/arch/x86/include/uapi/asm/bootparam.h 
b/arch/x86/include/uapi/asm/bootparam.h
index c15ddaf..d35da96 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -131,7 +131,8 @@ struct boot_params {
__u8  eddbuf_entries;   /* 0x1e9 */
__u8  edd_mbr_sig_buf_entries;  /* 0x1ea */
__u8  kbd_status;   /* 0x1eb */
-   __u8  _pad5[3]; /* 0x1ec */
+   __u8  secure_boot;  /* 0x1ec */
+   __u8  _pad5[2]; /* 0x1ec */
/*
 * The sentinel is set to a nonzero value (0xff) in header.S.
 *
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f8ec578..deeb7bc 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1129,6 +1129,12 @@ void __init setup_arch(char **cmdline_p)
 
io_delay_init();
 
+#ifdef CONFIG_EFI_SECURE_BOOT_SIG_ENFORCE
+   if (boot_params.secure_boot) {
+

[RFC PATCH 4/4] x86: Fix EFI boot stub for large memory maps

2013-08-19 Thread Linn Crosetto 
This patch adds support for EFI memory maps larger than 128 entries when
booted using the EFI boot stub.

The e820 map in struct boot_params can only hold 128 entries, but the
memory map provided by EFI may be larger. If the map contained more than
128 entries, exit_boot() would write beyond the e820_map array in
boot_params and the system would eventually halt in the BUG_ON check in
sanitize_e820_map().

In the case we come in through efi_main(), the EFI memory map is used.
Instead of populating e820_map in boot_params, create the e820 from the
EFI memory map in setup_arch() via a memory_setup hook.

The EFI memory map may also be added in efi_init() if the command line
parameter add_efi_memmap is specified. This is left intact to allow
the command line parameter to remain effective.

Signed-off-by: Linn Crosetto l...@hp.com
---
 arch/x86/boot/compressed/eboot.c | 64 ++--
 arch/x86/kernel/setup.c  |  3 ++
 2 files changed, 6 insertions(+), 61 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index b7388a4..66b8d3d 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -973,6 +973,9 @@ struct boot_params *make_boot_params(void *handle, 
efi_system_table_t *_table)
if (status != EFI_SUCCESS)
goto fail2;
 
+   /* Use EFI memory map */
+   boot_params-e820_entries = 0;
+
return boot_params;
 fail2:
if (options_size)
@@ -986,8 +989,6 @@ static efi_status_t exit_boot(struct boot_params 
*boot_params,
  void *handle)
 {
struct efi_info *efi = boot_params-efi_info;
-   struct e820entry *e820_map = boot_params-e820_map[0];
-   struct e820entry *prev = NULL;
unsigned long size, key, desc_size, _size;
efi_memory_desc_t *mem_map;
efi_status_t status;
@@ -1049,65 +1050,6 @@ get_map:
/* Historic? */
boot_params-alt_mem_k = 32 * 1024;
 
-   /*
-* Convert the EFI memory map to E820.
-*/
-   nr_entries = 0;
-   for (i = 0; i  size / desc_size; i++) {
-   efi_memory_desc_t *d;
-   unsigned int e820_type = 0;
-   unsigned long m = (unsigned long)mem_map;
-
-   d = (efi_memory_desc_t *)(m + (i * desc_size));
-   switch (d-type) {
-   case EFI_RESERVED_TYPE:
-   case EFI_RUNTIME_SERVICES_CODE:
-   case EFI_RUNTIME_SERVICES_DATA:
-   case EFI_MEMORY_MAPPED_IO:
-   case EFI_MEMORY_MAPPED_IO_PORT_SPACE:
-   case EFI_PAL_CODE:
-   e820_type = E820_RESERVED;
-   break;
-
-   case EFI_UNUSABLE_MEMORY:
-   e820_type = E820_UNUSABLE;
-   break;
-
-   case EFI_ACPI_RECLAIM_MEMORY:
-   e820_type = E820_ACPI;
-   break;
-
-   case EFI_LOADER_CODE:
-   case EFI_LOADER_DATA:
-   case EFI_BOOT_SERVICES_CODE:
-   case EFI_BOOT_SERVICES_DATA:
-   case EFI_CONVENTIONAL_MEMORY:
-   e820_type = E820_RAM;
-   break;
-
-   case EFI_ACPI_MEMORY_NVS:
-   e820_type = E820_NVS;
-   break;
-
-   default:
-   continue;
-   }
-
-   /* Merge adjacent mappings */
-   if (prev  prev-type == e820_type 
-   (prev-addr + prev-size) == d-phys_addr)
-   prev-size += d-num_pages  12;
-   else {
-   e820_map-addr = d-phys_addr;
-   e820_map-size = d-num_pages  12;
-   e820_map-type = e820_type;
-   prev = e820_map++;
-   nr_entries++;
-   }
-   }
-
-   boot_params-e820_entries = nr_entries;
-
return EFI_SUCCESS;
 
 free_mem_map:
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f8ec578..9682c8c 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -920,6 +920,9 @@ void __init setup_arch(char **cmdline_p)
 
if (efi_enabled(EFI_BOOT))
efi_memblock_x86_reserve_range();
+
+   if (efi_memmap_needed())
+   x86_init.resources.memory_setup = efi_memory_setup;
 #endif
 
x86_init.oem.arch_setup();
-- 
1.7.11.3

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread David Woodhouse 
On Mon, 2013-08-19 at 10:38 -0700, James Bottomley wrote:
 It's not about us removing the code, it's about us having an accurate
 compliance test.  There are two reasons for having a fully correct
 compliance test
 
  1. Our work arounds have unintended consequences which have knock
 on effects which mean that you don't know if a test failure is
 real or an unintended consequence of a work around.
  2. New features in specs tend to build on previous features, so
 we're going to have a hard time constructing accurate tests for
 layered features where we've done a work around for the base
 feature.

3. Even if we can't *remove* the code, sometimes we can disable it at
runtime if we detect the BIOS is new enough that it shouldn't be broken.

Do we really want to declare that we can only use 50% of the available
NV storage space for *ever* more, just because some muppet thought they
could squeeze some non-upstream value add into their implementation of
the flash management? You seem to be suggesting that we should
retrospectively write that limitation into the UEFI spec, which is a
completely insane suggestion.

We absolutely want to be able to draw a line under it and declare that
any firmware built after $SOMEDATE is expected to be fixed, so we don't
have to do these stupid workarounds, and we can make full use of the
available space.

This type of model gets used for Windows all the time, doesn't it?
Especially with ACPI, they base some of their behaviour on the date that
the BIOS claims to be built, and only use newer features if it's new
enough that they're expected to be working?


4. We don't want people turning up to a plugfest with a buggy pile of
crap that we just *happen* to have worked around already, and going back
to their company with a happy no problems discovered report, and
thinking that they are doing a good job. If they turn up with a buggy
pile of crap, we want to make sure they *know* it's a buggy pile of crap
— they need to be sent home with their tail between their legs with a
clear message that they need to do better in future. And that will
*help* to avoid future bugs, hopefully.

The point of a plugfest is *not* just to gather a torture test together
and force the kernel developers to find a consistent set of workarounds
for *every* pathological brokenness out there. We could do that with a
big credit card and a buying spree of random machines.

Of *course* we should also do the tests with a fully-workarounded kernel
and be able to ensure that our kernel can boot on existing machines. But
that's a separate issue. That's about *us* learning and improving. But
it does need to work *both* ways for all parties to get the maximum
benefit.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread Matthew Garrett 
On Mon, Aug 19, 2013 at 09:09:54PM +0100, David Woodhouse wrote:

 Do we really want to declare that we can only use 50% of the available
 NV storage space for *ever* more, just because some muppet thought they
 could squeeze some non-upstream value add into their implementation of
 the flash management? You seem to be suggesting that we should
 retrospectively write that limitation into the UEFI spec, which is a
 completely insane suggestion.

We only reserve 3K now, and testing this in the existing UEFI test kit 
would be entirely achievable. Including the ability to remove this check 
from the kernel is just an invitation for someone to build a kernel 
without it and then be surprised when their machine fucks up.

 We absolutely want to be able to draw a line under it and declare that
 any firmware built after $SOMEDATE is expected to be fixed, so we don't
 have to do these stupid workarounds, and we can make full use of the
 available space.

We have no way to guarantee that. Most board vendors don't turn up to 
the plugfests and aren't going to run anything we ship.

-- 
Matthew Garrett | mj...@srcf.ucam.org
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread Matthew Garrett 
On Mon, Aug 19, 2013 at 09:21:11PM +0100, David Woodhouse wrote:
 On Mon, 2013-08-19 at 21:19 +0100, Matthew Garrett wrote:
  
  We have no way to guarantee that. Most board vendors don't turn up to 
  the plugfests and aren't going to run anything we ship.
 
 Oh well, let's just wring our hands and not bother to turn up to the
 plugfest at all then. No point trying to *improve* the situation, after
 all.

The plugfests have, from our perspective, always been useful in 
identifying new implementation interpretations before hardware ships. 
But even then, it's usually too late to modify the firmware. Vendors who 
care about Linux compatibility have already tested Linux before we turn 
up.

-- 
Matthew Garrett | mj...@srcf.ucam.org
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/4] EFI boot stub memory map fix

2013-08-19 Thread Yinghai Lu 
On Mon, Aug 19, 2013 at 1:06 PM, H. Peter Anvin h...@zytor.com wrote:
 I would strongly disagree that option 2 is the cleaner solution.

Agreed.


 Linn Crosetto l...@hp.com wrote:
I realize the EFI stub for ARM patches are in flight,

https://lkml.org/lkml/2013/8/9/554

and overlap with some of the files but I wanted to send these out for
comment.

This series fixes a problem with EFI memory maps larger than 128
entries when
booting using the EFI boot stub, which results in overflowing the
e820_map in
boot_params and an eventual halt when checking the map size in
sanitize_e820_map().

The fix implemented is to add the EFI memory map from setup_arch() via
a
memory_setup hook.

Two options were considered:

 1. Use the SETUP_E820_EXT setup_data type to add the extra entries.

2. Create a memory_setup function to be enabled when the EFI memory map
is
needed.

Option 2 appeared to be the cleaner solution, reducing duplication with
existing code, given a reasonable mechanism for determining when to
replace the default memory_setup function.

If boot_loader could create setup_data with SETUP_E820_EXT,
efi_stub should go that path too.
We should not add another path.

Thanks

Yinghai
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread David Woodhouse 
On Mon, 2013-08-19 at 21:39 +0100, Matthew Garrett wrote:
 The plugfests have, from our perspective, always been useful in 
 identifying new implementation interpretations before hardware ships. 
 But even then, it's usually too late to modify the firmware. Vendors who 
 care about Linux compatibility have already tested Linux before we turn 
 up.

You effectively seem to be suggesting that nothing will ever get better
on the UEFI side, and the only benefit of the plugfest is that we get to
see the latest brokenness and try to come up with a workaround for it
before the consumers are afflicted with it?

That's a really pessimistic view, and I'd really like us to be a little
more optimistic. Things can't be, or at least can't *stay*, that bad.
Surely?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

Re: [RFC PATCH 0/4] EFI boot stub memory map fix

2013-08-19 Thread Linn Crosetto 
On Mon, Aug 19, 2013 at 01:47:41PM -0700, Yinghai Lu wrote:
 On Mon, Aug 19, 2013 at 1:06 PM, H. Peter Anvin h...@zytor.com wrote:
  I would strongly disagree that option 2 is the cleaner solution.
 
 Agreed.
 
 
  Linn Crosetto l...@hp.com wrote:
 I realize the EFI stub for ARM patches are in flight,
 
 https://lkml.org/lkml/2013/8/9/554
 
 and overlap with some of the files but I wanted to send these out for
 comment.
 
 This series fixes a problem with EFI memory maps larger than 128
 entries when
 booting using the EFI boot stub, which results in overflowing the
 e820_map in
 boot_params and an eventual halt when checking the map size in
 sanitize_e820_map().
 
 The fix implemented is to add the EFI memory map from setup_arch() via
 a
 memory_setup hook.
 
 Two options were considered:
 
  1. Use the SETUP_E820_EXT setup_data type to add the extra entries.
 
 2. Create a memory_setup function to be enabled when the EFI memory map
 is
 needed.
 
 Option 2 appeared to be the cleaner solution, reducing duplication with
 existing code, given a reasonable mechanism for determining when to
 replace the default memory_setup function.
 
 If boot_loader could create setup_data with SETUP_E820_EXT,
 efi_stub should go that path too.
 We should not add another path.

My consideration was in leveraging do_add_efi_memmap(), which duplicates what is
done with the map in exit_boot(), and can already be called via the
add_efi_memmap parameter. One complication with SETUP_E820_EXT is in
determining the size needed, since a call to allocate_pool will change the
memory map. 

I will send another version which uses SETUP_E820_EXT.

Thanks,
Linn
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: UEFI Plugfest 2013 -- New Orleans

2013-08-19 Thread Matthew Garrett 
On Mon, Aug 19, 2013 at 10:06:51PM +0100, David Woodhouse wrote:

 You effectively seem to be suggesting that nothing will ever get better
 on the UEFI side, and the only benefit of the plugfest is that we get to
 see the latest brokenness and try to come up with a workaround for it
 before the consumers are afflicted with it?

Pretty much. There's a decent chance that board vendors already have the 
broken code before we end up testing against it.

 That's a really pessimistic view, and I'd really like us to be a little
 more optimistic. Things can't be, or at least can't *stay*, that bad.
 Surely?

Most vendors don't care about testing against Linux, and we can't make 
them care. What they're more likely to test against is the SCT, and 
extending that to cover a wider range of test cases (such as exhausting 
variable space) is much more likely to result in things being caught 
before anything is shipped - but even then, board vendors are going to 
take IBV code, perform value add, never run a test suite and just make 
sure it boots Windows.

-- 
Matthew Garrett | mj...@srcf.ucam.org
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html