(no subject)

[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.

V3 addresses some review feedback and also locks down uswsusp.

[PATCH V3 01/11] Add secure_modules() call

[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.garrett@nebula.com>
---
 include/linux/module.h |  7 +++++++
 kernel/module.c        | 10 ++++++++++
 2 files changed, 17 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..0e94acf 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3852,3 +3852,13 @@ 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
+}
+EXPORT_SYMBOL(secure_modules);
-- 
1.8.3.1

Re: [PATCH V3 01/11] Add secure_modules() call

[James Morris]

On Tue, 3 Sep 2013, Matthew Garrett wrote:

> 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.garrett@nebula.com>

Reviewed-by: James Morris <jmorris@namei.org>


> ---
>  include/linux/module.h |  7 +++++++
>  kernel/module.c        | 10 ++++++++++
>  2 files changed, 17 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..0e94acf 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3852,3 +3852,13 @@ 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
> +}
> +EXPORT_SYMBOL(secure_modules);
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH V3 01/11] Add secure_modules() call

[joeyli]

於 二，2013-09-03 於 19:50 -0400，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.garrett@nebula.com>

Tested-by: Lee, Chun-Yi <jlee@suse.com>

> ---
>  include/linux/module.h |  7 +++++++
>  kernel/module.c        | 10 ++++++++++
>  2 files changed, 17 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..0e94acf 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3852,3 +3852,13 @@ 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
> +}
> +EXPORT_SYMBOL(secure_modules);

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

[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.garrett@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

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

[James Morris]

On Tue, 3 Sep 2013, Matthew Garrett wrote:

> 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.garrett@nebula.com>


Reviewed-by: James Morris <jmorris@namei.org>



> ---
>  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-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
James Morris
<jmorris@namei.org>

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

On Tue, 2013-09-03 at 19:50 -0400, Matthew Garrett wrote:
> 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.

Can you elaborate on what you mean by "sufficiently IOMMU-isolated", and
what's missing before we can do that?

If a given device is protected by an active IOMMU, and if there's no
driver loaded and hence no active DMA mappings for the device in
question, then we ought to be able to prod at it safely, right? It can't
DMA anywhere anyway.

If there's a driver loaded but still no active DMA mappings, that's
should still be OK, albeit harder to check.

If there are active mappings, that's less clear... we can still only
scribble on memory ranges which were already *mapped* for this device to
write to (ring buffers, receive buffers, etc.). But it's still probably
best not to allow it?

And there are non-DMA considerations too, aren't there? What about just
writing some fun stuff to a memory BAR and then writing to PCI config to
map that BAR to an address that we can get executed by kernel code?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

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

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1836 bytes --]

On Wed, 2013-09-04 at 17:57 +0100, David Woodhouse wrote:
> On Tue, 2013-09-03 at 19:50 -0400, Matthew Garrett wrote:
> > 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.
> 
> Can you elaborate on what you mean by "sufficiently IOMMU-isolated", and
> what's missing before we can do that?

Do we have in-kernel API to guarantee that a given PCI device is
actively isolated by an IOMMU such that it can't modify any host kernel
pages that aren't explicitly intended to be writable by the device? That
seems to be the biggest constraint.

> If a given device is protected by an active IOMMU, and if there's no
> driver loaded and hence no active DMA mappings for the device in
> question, then we ought to be able to prod at it safely, right? It can't
> DMA anywhere anyway.

How does virt passthrough work in this case? The current situation
appears to be that qemu just passes the BARs through to the guest, and
it's the guest that sets things up. We'd need to be able to ensure that
there's no way the guest driver can cause DMA into the host kernel.

> And there are non-DMA considerations too, aren't there? What about just
> writing some fun stuff to a memory BAR and then writing to PCI config to
> map that BAR to an address that we can get executed by kernel code?

Yes, that's why config space is locked down for now.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]

On Wed, 2013-09-04 at 17:04 +0000, Matthew Garrett wrote:
> Do we have in-kernel API to guarantee that a given PCI device is
> actively isolated by an IOMMU such that it can't modify any host kernel
> pages that aren't explicitly intended to be writable by the device? That
> seems to be the biggest constraint.

We don't, but it's not hard to add one if we have a consensus on exactly
what it needs to mean.

> How does virt passthrough work in this case? The current situation
> appears to be that qemu just passes the BARs through to the guest, and
> it's the guest that sets things up. We'd need to be able to ensure that
> there's no way the guest driver can cause DMA into the host kernel.

We set up the IOMMU page tables so that the virtual bus addresses seen
by the PCI device are 1:1 mapped to the guest "physical" address space.

That is, what the PCI device sees as a "physical" address is equivalent
to what the guest sees as a "physical" address space. It can access
memory which belongs to that guest, and nothing else. So that should be
fine.

(Currently, the guest sees no IOMMU. There's just that permanent 1:1
mapping of all of the guest's memory so that it's visible to the device.
We may later implement a virtual IOMMU within qemu, and then we'll have
more dynamic mappings. But the principle will remain the same: PCI
devices assigned to a KVM guest can only 'see' memory pages which belong
to that guest.

> > And there are non-DMA considerations too, aren't there? What about just
> > writing some fun stuff to a memory BAR and then writing to PCI config to
> > map that BAR to an address that we can get executed by kernel code?
> 
> Yes, that's why config space is locked down for now.

OK.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

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

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1233 bytes --]

On Wed, 2013-09-04 at 19:58 +0100, David Woodhouse wrote:
> On Wed, 2013-09-04 at 17:04 +0000, Matthew Garrett wrote:

> > How does virt passthrough work in this case? The current situation
> > appears to be that qemu just passes the BARs through to the guest, and
> > it's the guest that sets things up. We'd need to be able to ensure that
> > there's no way the guest driver can cause DMA into the host kernel.
> 
> We set up the IOMMU page tables so that the virtual bus addresses seen
> by the PCI device are 1:1 mapped to the guest "physical" address space.
> 
> That is, what the PCI device sees as a "physical" address is equivalent
> to what the guest sees as a "physical" address space. It can access
> memory which belongs to that guest, and nothing else. So that should be
> fine.

But presumably the guest's view of RAM is what gets written to the BARs?
I guess if we know it's constrained then there's no need to restrict the
addresses that can be set - we know that they'll be unable to overlap
the host RAM.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

On Wed, 2013-09-04 at 19:01 +0000, Matthew Garrett wrote:
> But presumably the guest's view of RAM is what gets written to the BARs?

You're talking about the MMIO BARs of the devices which are given to the
guest, right? The register into which we write the 'ring buffer
address', and for that matter also the addresses which are written
*into* that ring buffer, etc.

It is indeed the guest's "physical address" which is written there. The
guest knows nothing of *host* physical addresses.

For the normal MMU, the guest sets up its page tables and, by the magic
of KVM, guest virtual addresses are translated twice — once to guest
*physical* addresses, and then to real physical addresses for stuff to
actually work.

For DMA, the guest hands 'guest physical' addresses directly to the
device. And we've set up the IOMMU to have a mapping of all of guest
physical address space, to the appropriate host physical pages.

> I guess if we know it's constrained then there's no need to restrict the
> addresses that can be set - we know that they'll be unable to overlap
> the host RAM.

There is no need to restrict the addresses that can be set. The only
addresses it can reach are pages which belong to the guest.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

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

[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.garrett@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

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

[James Morris]

On Tue, 3 Sep 2013, Matthew Garrett wrote:

> 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.garrett@nebula.com>


Reviewed-by: James Morris <jmorris@namei.org>




> ---
>  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-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
James Morris
<jmorris@namei.org>

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

[H. Peter Anvin]

On 09/03/2013 04:50 PM, Matthew Garrett wrote:
> 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.garrett@nebula.com>

Seriously... just deny CAP_SYS_RAWIO to any system in secure mode.

	-hpa

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

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1005 bytes --]

On Wed, 2013-09-04 at 20:52 -0700, H. Peter Anvin wrote:
> On 09/03/2013 04:50 PM, Matthew Garrett wrote:
> > 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.garrett@nebula.com>
> 
> Seriously... just deny CAP_SYS_RAWIO to any system in secure mode.

No. CAP_SYS_RAWIO blocks things that we don't want blocked (x86
microcode updates, various disk ioctls, *device firmware uploads* and a
few others) - the semantics just don't match. We could relax those
permissions, but then we'd potentially break someone else's security
considerations.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[H. Peter Anvin]

Sigh.  That capability really is fscked up.

Matthew Garrett <matthew.garrett@nebula.com> wrote:
>On Wed, 2013-09-04 at 20:52 -0700, H. Peter Anvin wrote:
>> On 09/03/2013 04:50 PM, Matthew Garrett wrote:
>> > 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.garrett@nebula.com>
>> 
>> Seriously... just deny CAP_SYS_RAWIO to any system in secure mode.
>
>No. CAP_SYS_RAWIO blocks things that we don't want blocked (x86
>microcode updates, various disk ioctls, *device firmware uploads* and a
>few others) - the semantics just don't match. We could relax those
>permissions, but then we'd potentially break someone else's security
>considerations.

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

[PATCH V3 04/11] ACPI: Limit access to custom_method

[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.garrett@nebula.com>
---
 drivers/acpi/custom_method.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
index 12b62f2..50647b3 100644
--- a/drivers/acpi/custom_method.c
+++ b/drivers/acpi/custom_method.c
@@ -29,6 +29,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

Re: [PATCH V3 04/11] ACPI: Limit access to custom_method

[James Morris]

On Tue, 3 Sep 2013, Matthew Garrett wrote:

> 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.garrett@nebula.com>

Reviewed-by: James Morris <jmorris@namei.org>




> ---
>  drivers/acpi/custom_method.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
> index 12b62f2..50647b3 100644
> --- a/drivers/acpi/custom_method.c
> +++ b/drivers/acpi/custom_method.c
> @@ -29,6 +29,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-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
James Morris
<jmorris@namei.org>

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

[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.garrett@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..db18ef66 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 (secure_modules())
+		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 (secure_modules())
+		return -EPERM;
+
 	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID,
 				     1, asus->debug.method_id,
 				     &input, &output);
-- 
1.8.3.1

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

[James Morris]

On Tue, 3 Sep 2013, Matthew Garrett 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.garrett@nebula.com>

Reviewed-by: James Morris <jmorris@namei.org>


-- 
James Morris
<jmorris@namei.org>

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

[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.garrett@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

Re: [PATCH V3 06/11] Restrict /dev/mem and /dev/kmem when module loading is restricted

[James Morris]

On Tue, 3 Sep 2013, Matthew Garrett wrote:

> 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.garrett@nebula.com>


Reviewed-by: James Morris <jmorris@namei.org>



> ---
>  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-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
James Morris
<jmorris@namei.org>

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

[Matthew Garrett]

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 circumvent any restrictions imposed on
loading modules. Disable it in that case.

Signed-off-by: Josh Boyer <jwboyer@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

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

[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.garrett@nebula.com>
---
 kernel/kexec.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index 59f7b55..3e2b63a 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>
@@ -943,6 +944,13 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 		return -EPERM;
 
 	/*
+	 * kexec can be used to circumvent module loading restrictions, so
+	 * prevent loading in that case
+	 */
+	if (secure_modules())
+		return -EPERM;
+
+	/*
 	 * Verify we have a legal set of flags
 	 * This leaves us room for future extensions.
 	 */
-- 
1.8.3.1

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

[James Morris]

On Tue, 3 Sep 2013, Matthew Garrett wrote:

> 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.garrett@nebula.com>

Reviewed-by: James Morris <jmorris@namei.org>


-- 
James Morris
<jmorris@namei.org>

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

[jerry.hoemann]

On Tue, Sep 03, 2013 at 07:50:15PM -0400, Matthew Garrett wrote:
> 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.garrett@nebula.com>


Matthew,

Disabling kexec will disable kdump, correct?

Are there plans to enable kdump on a system where secure
boot is enabled?


thanks

Jerry



> ---
>  kernel/kexec.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index 59f7b55..3e2b63a 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>
> @@ -943,6 +944,13 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
>  		return -EPERM;
>  
>  	/*
> +	 * kexec can be used to circumvent module loading restrictions, so
> +	 * prevent loading in that case
> +	 */
> +	if (secure_modules())
> +		return -EPERM;
> +
> +	/*
>  	 * Verify we have a legal set of flags
>  	 * This leaves us room for future extensions.
>  	 */
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

----------------------------------------------------------------------------
Jerry Hoemann            Software Engineer              Hewlett-Packard/MODL

3404 E Harmony Rd. MS 57                        phone:  (970) 898-1022
Ft. Collins, CO 80528                           FAX:    (970) 898-XXXX
                                                email:  jerry.hoemann@hp.com
----------------------------------------------------------------------------

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

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 811 bytes --]

On Wed, 2013-09-04 at 14:09 -0600, jerry.hoemann@hp.com wrote:
> On Tue, Sep 03, 2013 at 07:50:15PM -0400, Matthew Garrett wrote:
> > 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.garrett@nebula.com>
> 
> 
> Matthew,
> 
> Disabling kexec will disable kdump, correct?

Yes.

> Are there plans to enable kdump on a system where secure
> boot is enabled?

Yes, Vivek Goyal (cc:ed) is working on that.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Josh Boyer]

On Wed, Sep 4, 2013 at 4:09 PM,  <jerry.hoemann@hp.com> wrote:
> On Tue, Sep 03, 2013 at 07:50:15PM -0400, Matthew Garrett wrote:
>> 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.garrett@nebula.com>
>
>
> Matthew,
>
> Disabling kexec will disable kdump, correct?

Yes.

> Are there plans to enable kdump on a system where secure
> boot is enabled?

Vivek Goyal has been working on this.  I've not seen the code yet, but
I believe it should be posted somewhere relatively soon.  We're also
planning on talking about it at the Secure Boot microconference at
Linux Plumbers in two weeks.

josh

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

[Greg KH]

On Tue, Sep 03, 2013 at 07:50:15PM -0400, Matthew Garrett wrote:
> 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.

I see no match between kexec and signed kernel modules.

In fact, I personally _want_ signed kernel modules, and still the option
to run kexec.  kexec is to run a whole new kernel/OS, not a tiny kernel
module.

If you apply this, you break everyone who is currently relying on kexec
(i.e. kdump, bootloaders, etc.), from using signed kernel modules, which
personally, seems like a very bad idea.

Please just rely on the existing permission checks for kexec, to add on
another layer of restrictions seems unneeded, and will force some users
to have to patch this out.

thanks,

greg k-h

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

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1723 bytes --]

On Sat, 2013-09-07 at 23:40 -0700, Greg KH wrote:
> On Tue, Sep 03, 2013 at 07:50:15PM -0400, Matthew Garrett wrote:
> > 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.
> 
> I see no match between kexec and signed kernel modules.

sig_enforce is there to prevent anyone (including root) from installing
new kernel code in the running kernel. Allowing kexec to run untrusted
code allows root to install new kernel code in the running kernel. At
the most trivial level, grab the address of sig_enforce from kallsyms,
jump to a kernel that doesn't enforce STRICT_DEVMEM, modify sig_enforce,
jump back to the old kernel.


> In fact, I personally _want_ signed kernel modules, and still the option
> to run kexec.  kexec is to run a whole new kernel/OS, not a tiny kernel
> module.

No, kexec is to run anything. It's expressly not limited to launching
new kernels. It's easiest to demonstrate an attack using a Linux kernel,
but you could launch a toy payload that did nothing other than modify
one byte and then returned to the launch kernel.

> If you apply this, you break everyone who is currently relying on kexec
> (i.e. kdump, bootloaders, etc.), from using signed kernel modules, which
> personally, seems like a very bad idea.

Enforcing signed modules provides you with no additional security if you
have kexec enabled. It's better to make that obvious.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Greg KH]

On Sun, Sep 08, 2013 at 06:44:08AM +0000, Matthew Garrett wrote:
> On Sat, 2013-09-07 at 23:40 -0700, Greg KH wrote:
> > On Tue, Sep 03, 2013 at 07:50:15PM -0400, Matthew Garrett wrote:
> > > 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.
> > 
> > I see no match between kexec and signed kernel modules.
> 
> sig_enforce is there to prevent anyone (including root) from installing
> new kernel code in the running kernel.

No, it's to enforce kernel modules to be signed or not.  That's all.

> Allowing kexec to run untrusted code allows root to install new kernel
> code in the running kernel.

Which has nothing to do with signed kernel modules.

> At the most trivial level, grab the address of sig_enforce from
> kallsyms, jump to a kernel that doesn't enforce STRICT_DEVMEM, modify
> sig_enforce, jump back to the old kernel.

Which proves what?

> > In fact, I personally _want_ signed kernel modules, and still the option
> > to run kexec.  kexec is to run a whole new kernel/OS, not a tiny kernel
> > module.
> 
> No, kexec is to run anything. It's expressly not limited to launching
> new kernels. It's easiest to demonstrate an attack using a Linux kernel,
> but you could launch a toy payload that did nothing other than modify
> one byte and then returned to the launch kernel.

Fair enough, but again, this has nothing to do with signed kernel
modules.

If you are really worried about kexec doing something like this, then
add signed kexec binary support, but don't suddenly stop kexec from
working just because signed modules are enabled, the two have nothing to
do with each other.

> > If you apply this, you break everyone who is currently relying on kexec
> > (i.e. kdump, bootloaders, etc.), from using signed kernel modules, which
> > personally, seems like a very bad idea.
> 
> Enforcing signed modules provides you with no additional security if you
> have kexec enabled. It's better to make that obvious.

Then document the heck out of it, don't disable a valid use case just
because it possibly could be used in some way that is different from the
original system.

If you take this to an extreme, kexec shouldn't be here at all, as it
can do anything in the kernel wherever it wants to.

kexec has nothing to do with signed modules, don't tie them together.

thanks,

greg k-h

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

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 625 bytes --]

On Sun, 2013-09-08 at 00:24 -0700, Greg KH wrote:
> On Sun, Sep 08, 2013 at 06:44:08AM +0000, Matthew Garrett wrote:

> > At the most trivial level, grab the address of sig_enforce from
> > kallsyms, jump to a kernel that doesn't enforce STRICT_DEVMEM, modify
> > sig_enforce, jump back to the old kernel.
> 
> Which proves what?

sig_enforce can be set, but once it's set can't be unset. Why do you
think that is?

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Kees Cook]

On Sun, Sep 8, 2013 at 12:24 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sun, Sep 08, 2013 at 06:44:08AM +0000, Matthew Garrett wrote:
>> On Sat, 2013-09-07 at 23:40 -0700, Greg KH wrote:
>> > If you apply this, you break everyone who is currently relying on kexec
>> > (i.e. kdump, bootloaders, etc.), from using signed kernel modules, which
>> > personally, seems like a very bad idea.
>>
>> Enforcing signed modules provides you with no additional security if you
>> have kexec enabled. It's better to make that obvious.
>
> Then document the heck out of it, don't disable a valid use case just
> because it possibly could be used in some way that is different from the
> original system.
>
> If you take this to an extreme, kexec shouldn't be here at all, as it
> can do anything in the kernel wherever it wants to.
>
> kexec has nothing to do with signed modules, don't tie them together.

It's not accurate to say it has "nothing to do" with signed modules.
The purpose of signed modules is to ensure the integrity of the
running system against the root user.

It was, however, incomplete. Terrible analogy follows: signed modules
was locking the front door, but we have all sorts of windows still
open. This closes those windows. You're trying to say that shutting
windows has nothing to do with lumber locks. While technically true,
this is about the intent of the barriers.

Anyone currently using signed modules (with sig_enforce) AND kexec is
deluding themselves about what the state of their system's ring-0
security stance is. Those people should be running without
sig_enforce, and if they want both sig_enforce and kexec, then I would
expect a follow-up patch from them to provide signed kexec support.

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Greg KH]

On Sun, Sep 08, 2013 at 08:51:27AM -0700, Kees Cook wrote:
> On Sun, Sep 8, 2013 at 12:24 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Sun, Sep 08, 2013 at 06:44:08AM +0000, Matthew Garrett wrote:
> >> On Sat, 2013-09-07 at 23:40 -0700, Greg KH wrote:
> >> > If you apply this, you break everyone who is currently relying on kexec
> >> > (i.e. kdump, bootloaders, etc.), from using signed kernel modules, which
> >> > personally, seems like a very bad idea.
> >>
> >> Enforcing signed modules provides you with no additional security if you
> >> have kexec enabled. It's better to make that obvious.
> >
> > Then document the heck out of it, don't disable a valid use case just
> > because it possibly could be used in some way that is different from the
> > original system.
> >
> > If you take this to an extreme, kexec shouldn't be here at all, as it
> > can do anything in the kernel wherever it wants to.
> >
> > kexec has nothing to do with signed modules, don't tie them together.
> 
> It's not accurate to say it has "nothing to do" with signed modules.
> The purpose of signed modules is to ensure the integrity of the
> running system against the root user.

For one type of thing, modules, not for all types of ways a root user
could do "bad" things.

If you want to create a flag/option for "don't trust a privileged root
user", great, I have no objection to that.

I do object to changing the functionality of random other options based
on the state of signed kernel modules being enabled or not.

> It was, however, incomplete. Terrible analogy follows: signed modules
> was locking the front door, but we have all sorts of windows still
> open. This closes those windows. You're trying to say that shutting
> windows has nothing to do with lumber locks. While technically true,
> this is about the intent of the barriers.

Then be specific about the intent and don't say that "if we lock the
front door with our key, suddenly all of the windows close and lock
automatically".  I wanted fresh air in some of those windows for various
reasons.  Provide a "lock this house completely down" option that does
this instead, don't overload an already usable option to do more than it
was supposed to do.

That way people can pick and choose the security they want, based on
their specific situation and systems.

> Anyone currently using signed modules (with sig_enforce) AND kexec is
> deluding themselves about what the state of their system's ring-0
> security stance is. Those people should be running without
> sig_enforce, and if they want both sig_enforce and kexec, then I would
> expect a follow-up patch from them to provide signed kexec support.

I want both, but I don't need signed kexec support because I want to use
kexec for a program that I "know" is correct because I validated the
disk image it was on before I mounted it.  We already have other ways to
"verify" things without having to add individual verification of
specific pieces.

Just like ChromeOS was "trusting" kernel modules on partitions that it
"knew" were ok because they were verified before mounting.  You didn't
need signed kernel modules to be able to create your own chain-of-trust
any more than we need to provide signed kexec for some people to be able
to trust the binary they are going to kexec.

Not to say that signed kexec support isn't a good idea, I'm all for
that, but it's a tangential issue here.

thanks,

greg k-h

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

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 851 bytes --]

On Sun, 2013-09-08 at 09:18 -0700, Greg KH wrote:

> I want both, but I don't need signed kexec support because I want to use
> kexec for a program that I "know" is correct because I validated the
> disk image it was on before I mounted it.  We already have other ways to
> "verify" things without having to add individual verification of
> specific pieces.

The kernel has no way to know that your kexec payload is coming from a
verified image. It'll just as happily take something from an unverified
image. If you've ensured that there's no way an attacker can call
kexec_load() on an unverified image, then you don't need signed modules.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Greg KH]

On Sun, Sep 08, 2013 at 04:24:47PM +0000, Matthew Garrett wrote:
> On Sun, 2013-09-08 at 09:18 -0700, Greg KH wrote:
> 
> > I want both, but I don't need signed kexec support because I want to use
> > kexec for a program that I "know" is correct because I validated the
> > disk image it was on before I mounted it.  We already have other ways to
> > "verify" things without having to add individual verification of
> > specific pieces.
> 
> The kernel has no way to know that your kexec payload is coming from a
> verified image. It'll just as happily take something from an unverified
> image. If you've ensured that there's no way an attacker can call
> kexec_load() on an unverified image, then you don't need signed modules.

But I want, for other reasons (i.e. safety in layers), signed kernel
modules.  I also might actually want some debugfs files in some random
driver (like this series removes).

The point is that having a "lockdown" mode is good, I'm not disagreeing
there.  Just don't force it on people if they don't want it.  Allow them
to pick "lock everything down", or "I want signed modules", or "I don't
want kexec".

Don't lump all of this together such that people can not make that
choice between different things, because some people (i.e. me
specifically), do want them.

Heck, look at Red Hat.  They have been shipping signed kernel modules
for _years_ and yet they do not disable kexec.  Have they been "doing it
wrong" all of this time?   Perhaps people want signed modules just for
support reasons, not "security" reasons.

Don't take away those options.

thanks,

greg k-h

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

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1230 bytes --]

On Sun, 2013-09-08 at 09:39 -0700, Greg KH wrote:

> But I want, for other reasons (i.e. safety in layers), signed kernel
> modules.  I also might actually want some debugfs files in some random
> driver (like this series removes).

You want a configuration that makes no sense. There's no reason that the
kernel should make that easy.

> Heck, look at Red Hat.  They have been shipping signed kernel modules
> for _years_ and yet they do not disable kexec.  Have they been "doing it
> wrong" all of this time?   Perhaps people want signed modules just for
> support reasons, not "security" reasons.

Then they do what Red Hat does and don't set CONFIG_SIG_ENFORCE.

> Don't take away those options.

Where's the option to let me let unprivileged users set random MSRs?
Where's the option to let root disable STRICT_DEVMEM at runtime? We
don't offer these options because *they make no sense*. Locking your
door while leaving your window open may make you feel better, but it
doesn't improve the security of your house.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Greg KH]

On Sun, Sep 08, 2013 at 04:59:40PM +0000, Matthew Garrett wrote:
> On Sun, 2013-09-08 at 09:39 -0700, Greg KH wrote:
> 
> > But I want, for other reasons (i.e. safety in layers), signed kernel
> > modules.  I also might actually want some debugfs files in some random
> > driver (like this series removes).
> 
> You want a configuration that makes no sense. There's no reason that the
> kernel should make that easy.

It makes sense to me in that it is something I can do today, why stop
that?

> > Heck, look at Red Hat.  They have been shipping signed kernel modules
> > for _years_ and yet they do not disable kexec.  Have they been "doing it
> > wrong" all of this time?   Perhaps people want signed modules just for
> > support reasons, not "security" reasons.
> 
> Then they do what Red Hat does and don't set CONFIG_SIG_ENFORCE.

But what if I only want signed modules to be loaded?  Isn't that a valid
thing to want, yet still be able to also do other things with my system?

I sure think it is, and I have a real-world valid use case for this
exact type of thing, as I have already described, which meets my
security guarantees just fine.  I can go into details if you need me to
do so.

> > Don't take away those options.
> 
> Where's the option to let me let unprivileged users set random MSRs?

Can that happen today?

> Where's the option to let root disable STRICT_DEVMEM at runtime?

Again, can that happen today?

> We don't offer these options because *they make no sense*. Locking
> your door while leaving your window open may make you feel better, but
> it doesn't improve the security of your house.

Yes it does, it keeps the people who only try the front door from coming
in, a very common threat model.  It isn't "perfect" security, but I'm
not saying that it is, and no one else is either.

It's doing one thing, and doing it well.  The fact that you want to now
have that "one thing" mean "something else" is what I am objecting to.

Security is a series of "levels", all of which depend on different
threat models and scenarios.  Provide the ability to achieve different
levels of security, but don't force your idea of system-wide security on
others who may not agree with it.

thanks,

greg k-h

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

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2694 bytes --]

On Sun, 2013-09-08 at 10:22 -0700, Greg KH wrote:
> On Sun, Sep 08, 2013 at 04:59:40PM +0000, Matthew Garrett wrote:
> > On Sun, 2013-09-08 at 09:39 -0700, Greg KH wrote:
> > 
> > > But I want, for other reasons (i.e. safety in layers), signed kernel
> > > modules.  I also might actually want some debugfs files in some random
> > > driver (like this series removes).
> > 
> > You want a configuration that makes no sense. There's no reason that the
> > kernel should make that easy.
> 
> It makes sense to me in that it is something I can do today, why stop
> that?

You used to be able to modify MSRs without CAP_SYS_ADMIN. Why stop that?

> > > Heck, look at Red Hat.  They have been shipping signed kernel modules
> > > for _years_ and yet they do not disable kexec.  Have they been "doing it
> > > wrong" all of this time?   Perhaps people want signed modules just for
> > > support reasons, not "security" reasons.
> > 
> > Then they do what Red Hat does and don't set CONFIG_SIG_ENFORCE.
> 
> But what if I only want signed modules to be loaded?  Isn't that a valid
> thing to want, yet still be able to also do other things with my system?

If you only want signed modules to be loaded then you can't permit the
kexec of untrusted objects, because that would allow unsigned modules to
be loaded.

> > Where's the option to let me let unprivileged users set random MSRs?
> 
> Can that happen today?

No. We took it away.

> > Where's the option to let root disable STRICT_DEVMEM at runtime?
> 
> Again, can that happen today?

No, because it makes no sense - the entire point of STRICT_DEVMEM is to
restrict what root can do.

> > We don't offer these options because *they make no sense*. Locking
> > your door while leaving your window open may make you feel better, but
> > it doesn't improve the security of your house.
> 
> Yes it does, it keeps the people who only try the front door from coming
> in, a very common threat model.  It isn't "perfect" security, but I'm
> not saying that it is, and no one else is either.

Why would someone only try the front door when there's an obviously open
window next to it?

> Security is a series of "levels", all of which depend on different
> threat models and scenarios.  Provide the ability to achieve different
> levels of security, but don't force your idea of system-wide security on
> others who may not agree with it.

Providing a security feature that can be trivially circumvented isn't
security, it's security theatre.
-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[James Bottomley]

On Sun, 2013-09-08 at 08:51 -0700, Kees Cook wrote:
> On Sun, Sep 8, 2013 at 12:24 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Sun, Sep 08, 2013 at 06:44:08AM +0000, Matthew Garrett wrote:
> >> On Sat, 2013-09-07 at 23:40 -0700, Greg KH wrote:
> >> > If you apply this, you break everyone who is currently relying on kexec
> >> > (i.e. kdump, bootloaders, etc.), from using signed kernel modules, which
> >> > personally, seems like a very bad idea.
> >>
> >> Enforcing signed modules provides you with no additional security if you
> >> have kexec enabled. It's better to make that obvious.
> >
> > Then document the heck out of it, don't disable a valid use case just
> > because it possibly could be used in some way that is different from the
> > original system.
> >
> > If you take this to an extreme, kexec shouldn't be here at all, as it
> > can do anything in the kernel wherever it wants to.
> >
> > kexec has nothing to do with signed modules, don't tie them together.
> 
> It's not accurate to say it has "nothing to do" with signed modules.
> The purpose of signed modules is to ensure the integrity of the
> running system against the root user.

That's not true if you look at the use cases.  Distros use signed
modules to taint the kernel:  insert an unsigned one and the kernel
taints; insert a properly signed one and it doesn't.  They use it for
support to tell if you've been adhering to your contract.  That use case
has nothing to do with security.

> It was, however, incomplete. Terrible analogy follows: signed modules
> was locking the front door, but we have all sorts of windows still
> open. This closes those windows. You're trying to say that shutting
> windows has nothing to do with lumber locks. While technically true,
> this is about the intent of the barriers.
> 
> Anyone currently using signed modules (with sig_enforce) AND kexec is
> deluding themselves about what the state of their system's ring-0
> security stance is. Those people should be running without
> sig_enforce, and if they want both sig_enforce and kexec, then I would
> expect a follow-up patch from them to provide signed kexec support.

The analogy is rubbish.  I can give away CAP_SYS_MODULE and enforce what
modules those I've given the permission to can insert by signing them.
I keep CAP_SYS_BOOT, so they can't use kexec to subvert this.

Your analogy seems to be giving away the whole root and then crying Dr
it hurts when I do this ...

James

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

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1128 bytes --]

On Sun, 2013-09-08 at 10:11 -0700, James Bottomley wrote:

> That's not true if you look at the use cases.  Distros use signed
> modules to taint the kernel:  insert an unsigned one and the kernel
> taints; insert a properly signed one and it doesn't.  They use it for
> support to tell if you've been adhering to your contract.  That use case
> has nothing to do with security.

That use case has nothing to do with this patch, either. It's completely
unaffected. This only triggers if the kernel is configured to refuse the
loading of unsigned modules.

> The analogy is rubbish.  I can give away CAP_SYS_MODULE and enforce what
> modules those I've given the permission to can insert by signing them.
> I keep CAP_SYS_BOOT, so they can't use kexec to subvert this.

Yeah, that's a good argument for why capabilities are mostly pointless.
If I have CAP_SYS_BOOT I can give myself any other capabilities. Why
bother?

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[James Bottomley]

On Sun, 2013-09-08 at 17:15 +0000, Matthew Garrett wrote:
> On Sun, 2013-09-08 at 10:11 -0700, James Bottomley wrote:
> 
> > That's not true if you look at the use cases.  Distros use signed
> > modules to taint the kernel:  insert an unsigned one and the kernel
> > taints; insert a properly signed one and it doesn't.  They use it for
> > support to tell if you've been adhering to your contract.  That use case
> > has nothing to do with security.
> 
> That use case has nothing to do with this patch, either. It's completely
> unaffected. This only triggers if the kernel is configured to refuse the
> loading of unsigned modules.
> 
> > The analogy is rubbish.  I can give away CAP_SYS_MODULE and enforce what
> > modules those I've given the permission to can insert by signing them.
> > I keep CAP_SYS_BOOT, so they can't use kexec to subvert this.
> 
> Yeah, that's a good argument for why capabilities are mostly pointless.
> If I have CAP_SYS_BOOT I can give myself any other capabilities. Why
> bother?

It's an argument that CAP_SYS_BOOT is too powerful yes, but if you
recall, I said I keep that one.  In the rather lame analogy, what I do
by giving away CAP_SYS_MODULE and enforcing module signing while keeping
CAP_SYS_BOOT is allow people into my conservatory to play with the
plants but not into my house to steal the silver ... saying CAP_SYS_BOOT
is too powerful doesn't affect that use case because I haven't given
away CAP_SYS_BOOT.

James

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

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 908 bytes --]


> It's an argument that CAP_SYS_BOOT is too powerful yes, but if you
> recall, I said I keep that one.  In the rather lame analogy, what I do
> by giving away CAP_SYS_MODULE and enforcing module signing while keeping
> CAP_SYS_BOOT is allow people into my conservatory to play with the
> plants but not into my house to steal the silver ... saying CAP_SYS_BOOT
> is too powerful doesn't affect that use case because I haven't given
> away CAP_SYS_BOOT.

Ok, sorry, I had your meaning inverted. Yes, permitting the loading of
signed modules while preventing the use of kexec is a completely
reasonable configuration - so reasonable that it's what this patch
causes the kernel to do automatically. 

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[James Bottomley]

On Sun, 2013-09-08 at 17:27 +0000, Matthew Garrett wrote:
> > It's an argument that CAP_SYS_BOOT is too powerful yes, but if you
> > recall, I said I keep that one.  In the rather lame analogy, what I do
> > by giving away CAP_SYS_MODULE and enforcing module signing while keeping
> > CAP_SYS_BOOT is allow people into my conservatory to play with the
> > plants but not into my house to steal the silver ... saying CAP_SYS_BOOT
> > is too powerful doesn't affect that use case because I haven't given
> > away CAP_SYS_BOOT.
> 
> Ok, sorry, I had your meaning inverted. Yes, permitting the loading of
> signed modules while preventing the use of kexec is a completely
> reasonable configuration - so reasonable that it's what this patch
> causes the kernel to do automatically. 

Well, no, it doesn't because with this patch, *I* can't use kexec ...
you've just locked me out of my own house.

James

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

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1321 bytes --]

On Sun, 2013-09-08 at 10:32 -0700, James Bottomley wrote:
> On Sun, 2013-09-08 at 17:27 +0000, Matthew Garrett wrote:
> > > It's an argument that CAP_SYS_BOOT is too powerful yes, but if you
> > > recall, I said I keep that one.  In the rather lame analogy, what I do
> > > by giving away CAP_SYS_MODULE and enforcing module signing while keeping
> > > CAP_SYS_BOOT is allow people into my conservatory to play with the
> > > plants but not into my house to steal the silver ... saying CAP_SYS_BOOT
> > > is too powerful doesn't affect that use case because I haven't given
> > > away CAP_SYS_BOOT.
> > 
> > Ok, sorry, I had your meaning inverted. Yes, permitting the loading of
> > signed modules while preventing the use of kexec is a completely
> > reasonable configuration - so reasonable that it's what this patch
> > causes the kernel to do automatically. 
> 
> Well, no, it doesn't because with this patch, *I* can't use kexec ...
> you've just locked me out of my own house.

Hm. Ok, that's a more compelling argument than Greg's. Let me think
about whether there's a convenient way of supporting this.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH V3 09/11] uswsusp: Disable when module loading is restricted

[Matthew Garrett]

uswsusp allows a user process to dump and then restore kernel state, which
makes it possible to avoid module loading restrictions. Prevent this when
any restrictions have been imposed on loading modules.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 kernel/power/user.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/power/user.c b/kernel/power/user.c
index 4ed81e7..15cb72f 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -24,6 +24,7 @@
 #include <linux/console.h>
 #include <linux/cpu.h>
 #include <linux/freezer.h>
+#include <linux/module.h>
 
 #include <asm/uaccess.h>
 
@@ -48,6 +49,9 @@ static int snapshot_open(struct inode *inode, struct file *filp)
 	struct snapshot_data *data;
 	int error;
 
+	if (secure_modules())
+		return -EPERM;
+
 	lock_system_sleep();
 
 	if (!atomic_add_unless(&snapshot_device_available, -1, 0)) {
-- 
1.8.3.1

Re: [PATCH V3 09/11] uswsusp: Disable when module loading is restricted

[James Morris]

On Tue, 3 Sep 2013, Matthew Garrett wrote:

> uswsusp allows a user process to dump and then restore kernel state, which
> makes it possible to avoid module loading restrictions. Prevent this when
> any restrictions have been imposed on loading modules.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>


Reviewed-by: James Morris <jmorris@namei.org>


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH V3 09/11] uswsusp: Disable when module loading is restricted

[joeyli]

於 二，2013-09-03 於 19:50 -0400，Matthew Garrett 提到：
> uswsusp allows a user process to dump and then restore kernel state, which
> makes it possible to avoid module loading restrictions. Prevent this when
> any restrictions have been imposed on loading modules.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>

Tested-by: Lee, Chun-Yi <jlee@suse.com>

> ---
>  kernel/power/user.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/power/user.c b/kernel/power/user.c
> index 4ed81e7..15cb72f 100644
> --- a/kernel/power/user.c
> +++ b/kernel/power/user.c
> @@ -24,6 +24,7 @@
>  #include <linux/console.h>
>  #include <linux/cpu.h>
>  #include <linux/freezer.h>
> +#include <linux/module.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -48,6 +49,9 @@ static int snapshot_open(struct inode *inode, struct file *filp)
>  	struct snapshot_data *data;
>  	int error;
>  
> +	if (secure_modules())
> +		return -EPERM;
> +
>  	lock_system_sleep();
>  
>  	if (!atomic_add_unless(&snapshot_device_available, -1, 0)) {

Thanks
Joey Lee

[PATCH V3 10/11] x86: Restrict MSR access when module loading is restricted

[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 <keescook@chromium.org>
Signed-off-by: Matthew Garrett <matthew.garrett@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

Re: [PATCH V3 10/11] x86: Restrict MSR access when module loading is restricted

[James Morris]

On Tue, 3 Sep 2013, Matthew Garrett wrote:

> 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 <keescook@chromium.org>
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>


Reviewed-by: James Morris <jmorris@namei.org>



-- 
James Morris
<jmorris@namei.org>

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

[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.garrett@nebula.com>
---
 Documentation/x86/zero-page.txt       |  2 ++
 arch/x86/Kconfig                      | 10 ++++++++++
 arch/x86/boot/compressed/eboot.c      | 36 +++++++++++++++++++++++++++++++++++
 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, 69 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 @@ Offset	Proto	Name		Meaning
 1E9/001	ALL	eddbuf_entries	Number of entries in eddbuf (below)
 1EA/001	ALL	edd_mbr_sig_buf_entries	Number of entries in edd_mbr_sig_buffer
 				(below)
+1EB/001	ALL     kbd_status      Numlock is enabled
+1EC/001	ALL     secure_boot	Secure boot is enabled in the firmware
 1EF/001	ALL	sentinel	Used to detect broken bootloaders
 290/040	ALL	edd_mbr_sig_buffer EDD MBR signatures
 2D0/A00	ALL	e820_map	E820 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..53bfe4f 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -12,6 +12,7 @@
 #include <asm/efi.h>
 #include <asm/setup.h>
 #include <asm/desc.h>
+#include <asm/bootparam_utils.h>
 
 #undef memcpy			/* Use memcpy from misc.c */
 
@@ -861,6 +862,37 @@ fail:
 	return status;
 }
 
+static int get_secure_boot(void)
+{
+	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,
+				L"SecureBoot", &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,
+				L"SetupMode", &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 +1201,10 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
 	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
 		goto fail;
 
+	sanitize_boot_params(boot_params);
+
+	boot_params->secure_boot = get_secure_boot();
+
 	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..85d7685 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];					/* 0x1ed */
 	/*
 	 * 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) {
+		enforce_signed_modules();
+	}
+#endif
+
 	/*
 	 * Parse the ACPI tables for possible boot-time SMP configuration.
 	 */
diff --git a/include/linux/module.h b/include/linux/module.h
index 0c266b2..5a6374a 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -184,6 +184,12 @@ const struct exception_table_entry *search_exception_tables(unsigned long add);
 
 struct notifier_block;
 
+#ifdef CONFIG_MODULE_SIG
+extern void enforce_signed_modules(void);
+#else
+static inline void enforce_signed_modules(void) {};
+#endif
+
 #ifdef CONFIG_MODULES
 
 extern int modules_disabled; /* for sysctl */
diff --git a/kernel/module.c b/kernel/module.c
index 0e94acf..974139b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3853,6 +3853,13 @@ void module_layout(struct module *mod,
 EXPORT_SYMBOL(module_layout);
 #endif
 
+#ifdef CONFIG_MODULE_SIG
+void enforce_signed_modules(void)
+{
+	sig_enforce = true;
+}
+#endif
+
 bool secure_modules(void)
 {
 #ifdef CONFIG_MODULE_SIG
-- 
1.8.3.1

Re: [PATCH V3 11/11] Add option to automatically enforce module signatures when in Secure Boot mode

[James Morris]

On Tue, 3 Sep 2013, Matthew Garrett wrote:

> +	status = efi_call_phys5(sys_table->runtime->get_variable,
> +				L"SecureBoot", &var_guid, NULL, &datasize, &sb);
> +


What's 'L' ?


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH V3 11/11] Add option to automatically enforce module signatures when in Secure Boot mode

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 542 bytes --]

On Wed, 2013-09-04 at 11:42 +1000, James Morris wrote:
> On Tue, 3 Sep 2013, Matthew Garrett wrote:
> 
> > +	status = efi_call_phys5(sys_table->runtime->get_variable,
> > +				L"SecureBoot", &var_guid, NULL, &datasize, &sb);
> > +
> 
> 
> What's 'L' ?

Wide-character string constant. UEFI variable names are 16-bits per
character.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH V3 11/11] Add option to automatically enforce module signatures when in Secure Boot mode

[joeyli]

於 二，2013-09-03 於 19:50 -0400，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.garrett@nebula.com>

Tested-by: Lee, Chun-Yi <jlee@suse.com>

Thanks
Joey Lee

> ---
>  Documentation/x86/zero-page.txt       |  2 ++
>  arch/x86/Kconfig                      | 10 ++++++++++
>  arch/x86/boot/compressed/eboot.c      | 36 +++++++++++++++++++++++++++++++++++
>  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, 69 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 @@ Offset	Proto	Name		Meaning
>  1E9/001	ALL	eddbuf_entries	Number of entries in eddbuf (below)
>  1EA/001	ALL	edd_mbr_sig_buf_entries	Number of entries in edd_mbr_sig_buffer
>  				(below)
> +1EB/001	ALL     kbd_status      Numlock is enabled
> +1EC/001	ALL     secure_boot	Secure boot is enabled in the firmware
>  1EF/001	ALL	sentinel	Used to detect broken bootloaders
>  290/040	ALL	edd_mbr_sig_buffer EDD MBR signatures
>  2D0/A00	ALL	e820_map	E820 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..53bfe4f 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -12,6 +12,7 @@
>  #include <asm/efi.h>
>  #include <asm/setup.h>
>  #include <asm/desc.h>
> +#include <asm/bootparam_utils.h>
>  
>  #undef memcpy			/* Use memcpy from misc.c */
>  
> @@ -861,6 +862,37 @@ fail:
>  	return status;
>  }
>  
> +static int get_secure_boot(void)
> +{
> +	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,
> +				L"SecureBoot", &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,
> +				L"SetupMode", &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 +1201,10 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
>  	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
>  		goto fail;
>  
> +	sanitize_boot_params(boot_params);
> +
> +	boot_params->secure_boot = get_secure_boot();
> +
>  	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..85d7685 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];					/* 0x1ed */
>  	/*
>  	 * 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) {
> +		enforce_signed_modules();
> +	}
> +#endif
> +
>  	/*
>  	 * Parse the ACPI tables for possible boot-time SMP configuration.
>  	 */
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 0c266b2..5a6374a 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -184,6 +184,12 @@ const struct exception_table_entry *search_exception_tables(unsigned long add);
>  
>  struct notifier_block;
>  
> +#ifdef CONFIG_MODULE_SIG
> +extern void enforce_signed_modules(void);
> +#else
> +static inline void enforce_signed_modules(void) {};
> +#endif
> +
>  #ifdef CONFIG_MODULES
>  
>  extern int modules_disabled; /* for sysctl */
> diff --git a/kernel/module.c b/kernel/module.c
> index 0e94acf..974139b 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3853,6 +3853,13 @@ void module_layout(struct module *mod,
>  EXPORT_SYMBOL(module_layout);
>  #endif
>  
> +#ifdef CONFIG_MODULE_SIG
> +void enforce_signed_modules(void)
> +{
> +	sig_enforce = true;
> +}
> +#endif
> +
>  bool secure_modules(void)
>  {
>  #ifdef CONFIG_MODULE_SIG

Re: [PATCH V3 11/11] Add option to automatically enforce module signatures when in Secure Boot mode

[joeyli]

於 二，2013-09-03 於 19:50 -0400，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.garrett@nebula.com>
> ---
>  Documentation/x86/zero-page.txt       |  2 ++
>  arch/x86/Kconfig                      | 10 ++++++++++
>  arch/x86/boot/compressed/eboot.c      | 36 +++++++++++++++++++++++++++++++++++
>  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, 69 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 @@ Offset	Proto	Name		Meaning
>  1E9/001	ALL	eddbuf_entries	Number of entries in eddbuf (below)
>  1EA/001	ALL	edd_mbr_sig_buf_entries	Number of entries in edd_mbr_sig_buffer
>  				(below)
> +1EB/001	ALL     kbd_status      Numlock is enabled
> +1EC/001	ALL     secure_boot	Secure boot is enabled in the firmware
>  1EF/001	ALL	sentinel	Used to detect broken bootloaders
>  290/040	ALL	edd_mbr_sig_buffer EDD MBR signatures
>  2D0/A00	ALL	e820_map	E820 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

Maybe need add "select MODULE_SIG" to here for auto enable kernel module
signature check when user select this option?

> +	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.

Thanks a lot!
Joey Lee

Re: [PATCH V3 11/11] Add option to automatically enforce module signatures when in Secure Boot mode

[Matt Fleming]

On Tue, 03 Sep, at 07:50:18PM, Matthew Garrett wrote:
> 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.garrett@nebula.com>
> ---
>  Documentation/x86/zero-page.txt       |  2 ++
>  arch/x86/Kconfig                      | 10 ++++++++++
>  arch/x86/boot/compressed/eboot.c      | 36 +++++++++++++++++++++++++++++++++++
>  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, 69 insertions(+), 1 deletion(-)

[...]

> @@ -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) {
> +		enforce_signed_modules();
> +	}
> +#endif
> +

I'd advise checking efi_enabled(EFI_BOOT) along with .secure_boot to
guard against garbage values in boot_params.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V3 11/11] Add option to automatically enforce module signatures when in Secure Boot mode

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 473 bytes --]

On Thu, 2013-09-05 at 11:16 +0100, Matt Fleming wrote:

> I'd advise checking efi_enabled(EFI_BOOT) along with .secure_boot to
> guard against garbage values in boot_params.

We've called sanitize_boot_params(), so we can assert that there are no
garbage values.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re:

[Kees Cook]

On Tue, Sep 3, 2013 at 4:50 PM, Matthew Garrett
<matthew.garrett@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.
>
> V3 addresses some review feedback and also locks down uswsusp.

Looks good to me. Consider the entire series:

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS Security

Re:

[Josh Boyer]

On Wed, Sep 4, 2013 at 11:53 AM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Sep 3, 2013 at 4:50 PM, Matthew Garrett
> <matthew.garrett@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.
>>
>> V3 addresses some review feedback and also locks down uswsusp.
>
> Looks good to me. Consider the entire series:
>
> Acked-by: Kees Cook <keescook@chromium.org>

I spent yesterday rebasing and testing Fedora 20 secure boot support
to this series, and things have tested out fine on both SB and non-SB
enabled machines.

For the series:

Reviewed-by: Josh Boyer <jwboyer@fedoraproject.org>
Tested-by: Josh Boyer <jwboyer@fedoraproject.org>

josh