Re: [PATCH] x86: Lock down MSR writing in secure boot
On Fri, Feb 8, 2013 at 11:42 AM, H. Peter Anvin h...@zytor.com wrote: On 02/08/2013 11:18 AM, Kees Cook wrote: No. CAP_RAWIO is for reading. Writing needs a much stronger check. If so, I suspect we need to do this for *all* raw I/O... but I keep wondering how much more sensitive writing really is than reading. Well, I think there's a reasonable distinction between systems that expect to strictly enforce user-space/kernel-space separation (CAP_COMPROMISE_KERNEL) and things that are fiddling with hardware (CAP_SYS_RAWIO). For example, even things like /dev/mem already have this separation (although it is stronger). You can't open /dev/mem without CAP_SYS_RAWIO, but if you do, you still can't write to RAM in /dev/mem. This might be one of the earliest examples of this distinction, actually. I think it's likely that after a while, we can convert some of these proposed CAP_COMPROMISE_KERNEL checks in always-deny once we figure out how to deal with those areas more safely. -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86: Lock down MSR writing in secure boot
Analogy fail. The /dev/mem lockout applies to system RAM, not MMIO. I fear COMPROMISE_KERNEL is becoming the new SYS_ADMIN, which in turn is the new root. Why? Because it is inhebtly about a usage model, not about a specific resource. Kees Cook keesc...@chromium.org wrote: On Fri, Feb 8, 2013 at 11:42 AM, H. Peter Anvin h...@zytor.com wrote: On 02/08/2013 11:18 AM, Kees Cook wrote: No. CAP_RAWIO is for reading. Writing needs a much stronger check. If so, I suspect we need to do this for *all* raw I/O... but I keep wondering how much more sensitive writing really is than reading. Well, I think there's a reasonable distinction between systems that expect to strictly enforce user-space/kernel-space separation (CAP_COMPROMISE_KERNEL) and things that are fiddling with hardware (CAP_SYS_RAWIO). For example, even things like /dev/mem already have this separation (although it is stronger). You can't open /dev/mem without CAP_SYS_RAWIO, but if you do, you still can't write to RAM in /dev/mem. This might be one of the earliest examples of this distinction, actually. I think it's likely that after a while, we can convert some of these proposed CAP_COMPROMISE_KERNEL checks in always-deny once we figure out how to deal with those areas more safely. -Kees -- Kees Cook Chrome OS Security -- Sent from my mobile phone. Please excuse brevity and lack of formatting. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC][PATCH 0/7] Chainsaw efivars.c
From: Matt Fleming matt.flem...@intel.com drivers/firmware/efivars.c has grown pretty large and is ~2K lines. Inside efivars.c there's currently, o code for handling EFI variables at the firmware-level o sysfs code for exposing EFI variables o a new EFI variable filesystem o a persistent storage backend all intertwined and smushed together. This situation is only going to get worse as new EFI support is added. We need an interface that hides the EFI variable operations in use so code isn't tempted to access them directly, e.g. efivarfs currently uses '__efivars' which means it doesn't work for CONFIG_GOOGLE_SMI as that uses different variable ops. With this interface in place, we can start moving independent code out into separate files, allowing users to only turn on the functionality that they want. This patch series introduces the new efivar_entry API, and splits out the major parts of efivars.c into new files. There's more work TODO, but this is at least a start. The series is also available on the 'chainsaw' branch at, git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/linux.git Comments welcome. Matt Fleming (7): efivars: Keep a private global pointer to efivars efi: Move utf16_strlen() to efi.h efivars: New efivar_entry API drivers/firmware: Create a new EFI drivers directory efivars: Move pstore code out of efivars.c efi: Add generic variable operations efivarfs: Move to fs/efivarfs MAINTAINERS| 12 +- drivers/firmware/Kconfig | 18 +- drivers/firmware/Makefile |1 + drivers/firmware/efi/Kconfig | 56 + drivers/firmware/efi/Makefile |6 + drivers/firmware/efi/generic-ops.c | 52 + drivers/firmware/efi/pstore.c | 239 + drivers/firmware/efi/sysfs.c | 549 ++ drivers/firmware/efivars.c | 2068 +--- drivers/firmware/google/Kconfig|6 +- drivers/firmware/google/gsmi.c | 30 +- fs/Kconfig |1 + fs/Makefile|1 + fs/efivarfs/Kconfig| 12 + fs/efivarfs/Makefile |7 + fs/efivarfs/file.c | 130 +++ fs/efivarfs/inode.c| 179 fs/efivarfs/internal.h | 18 + fs/efivarfs/super.c| 233 include/linux/efi.h| 120 ++- 20 files changed, 2120 insertions(+), 1618 deletions(-) create mode 100644 drivers/firmware/efi/Kconfig create mode 100644 drivers/firmware/efi/Makefile create mode 100644 drivers/firmware/efi/generic-ops.c create mode 100644 drivers/firmware/efi/pstore.c create mode 100644 drivers/firmware/efi/sysfs.c create mode 100644 fs/efivarfs/Kconfig create mode 100644 fs/efivarfs/Makefile create mode 100644 fs/efivarfs/file.c create mode 100644 fs/efivarfs/inode.c create mode 100644 fs/efivarfs/internal.h create mode 100644 fs/efivarfs/super.c -- 1.7.11.7 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC][PATCH 1/7] efivars: Keep a private global pointer to efivars
From: Matt Fleming matt.flem...@intel.com
This may seem like a step backwards in terms of modularity, but we
don't need to track more than one 'struct efivars' at one time. There
is no synchronisation done between multiple EFI variable operations,
and according to Mike no one is using both the generic EFI var ops and
CONFIG_GOOGLE_SMI.
Make this explicit by returning -EBUSY if someone has already called
register_efivars().
Cc: Mike Waychison mi...@google.com
Signed-off-by: Matt Fleming matt.flem...@intel.com
---
drivers/firmware/efivars.c | 31 ---
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 5eafa22..24585c3 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -132,8 +132,11 @@ struct efivar_attribute {
ssize_t (*store)(struct efivar_entry *entry, const char *buf, size_t
count);
};
-static struct efivars __efivars;
-static struct efivar_operations ops;
+static struct efivars generic_efivars;
+static struct efivar_operations generic_ops;
+
+/* Private pointer to registered efivars */
+static struct efivars *__efivars;
#define PSTORE_EFI_ATTRIBUTES \
(EFI_VARIABLE_NON_VOLATILE | \
@@ -974,7 +977,7 @@ static int efivarfs_create(struct inode *dir, struct dentry
*dentry,
umode_t mode, bool excl)
{
struct inode *inode;
- struct efivars *efivars = __efivars;
+ struct efivars *efivars = __efivars;
struct efivar_entry *var;
int namelen, i = 0, err = 0;
@@ -1140,7 +1143,7 @@ static int efivarfs_fill_super(struct super_block *sb,
void *data, int silent)
struct inode *inode = NULL;
struct dentry *root;
struct efivar_entry *entry, *n;
- struct efivars *efivars = __efivars;
+ struct efivars *efivars = __efivars;
char *name;
efivarfs_sb = sb;
@@ -1835,6 +1838,12 @@ int register_efivars(struct efivars *efivars,
unsigned long variable_name_size = 1024;
int error = 0;
+ if (__efivars) {
+ return -EBUSY;
+ }
+
+ __efivars = efivars;
+
variable_name = kzalloc(variable_name_size, GFP_KERNEL);
if (!variable_name) {
printk(KERN_ERR efivars: Memory allocation failed.\n);
@@ -1937,12 +1946,12 @@ efivars_init(void)
return -ENOMEM;
}
- ops.get_variable = efi.get_variable;
- ops.set_variable = efi.set_variable;
- ops.get_next_variable = efi.get_next_variable;
- ops.query_variable_info = efi.query_variable_info;
+ generic_ops.get_variable = efi.get_variable;
+ generic_ops.set_variable = efi.set_variable;
+ generic_ops.get_next_variable = efi.get_next_variable;
+ generic_ops.query_variable_info = efi.query_variable_info;
- error = register_efivars(__efivars, ops, efi_kobj);
+ error = register_efivars(generic_efivars, generic_ops, efi_kobj);
if (error)
goto err_put;
@@ -1958,7 +1967,7 @@ efivars_init(void)
return 0;
err_unregister:
- unregister_efivars(__efivars);
+ unregister_efivars(generic_efivars);
err_put:
kobject_put(efi_kobj);
return error;
@@ -1968,7 +1977,7 @@ static void __exit
efivars_exit(void)
{
if (efi_enabled(EFI_RUNTIME_SERVICES)) {
- unregister_efivars(__efivars);
+ unregister_efivars(generic_efivars);
kobject_put(efi_kobj);
}
}
--
1.7.11.7
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC][PATCH 4/7] drivers/firmware: Create a new EFI drivers directory
From: Matt Fleming matt.flem...@intel.com efivars.c has grown far too large and needs to be divided up. Move the sysfs support code for EFI variables into efi/ so that it is more self-contained. This also allows enabling the efivarfs code without requiring that the sysfs code be enabled. Having EFI variables accessible to userland via two different methods is just confusing, especially given that no synchronisation is done between them. Cc: Seiji Aguchi seiji.agu...@hds.com Cc: Matthew Garrett mj...@srcf.ucam.org Cc: Jeremy Kerr jeremy.k...@canonical.com Cc: Tony Luck tony.l...@intel.com Cc: Mike Waychison mi...@google.com Signed-off-by: Matt Fleming matt.flem...@intel.com --- MAINTAINERS | 1 + drivers/firmware/Kconfig | 18 +- drivers/firmware/Makefile | 1 + drivers/firmware/efi/Kconfig | 32 +++ drivers/firmware/efi/Makefile | 4 + drivers/firmware/efi/sysfs.c | 549 ++ drivers/firmware/efivars.c| 530 +--- include/linux/efi.h | 4 +- 8 files changed, 594 insertions(+), 545 deletions(-) create mode 100644 drivers/firmware/efi/Kconfig create mode 100644 drivers/firmware/efi/Makefile create mode 100644 drivers/firmware/efi/sysfs.c diff --git a/MAINTAINERS b/MAINTAINERS index 212c255..e37219d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2892,6 +2892,7 @@ F:arch/x86/boot/compressed/eboot.[ch] F: arch/x86/include/asm/efi.h F: arch/x86/platform/efi/* F: drivers/firmware/efivars.c +F: drivers/firmware/efi/* F: include/linux/efi*.h EFIFB FRAMEBUFFER DRIVER diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index 9b00072..9387630 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -36,23 +36,6 @@ config FIRMWARE_MEMMAP See also Documentation/ABI/testing/sysfs-firmware-memmap. -config EFI_VARS - tristate EFI Variable Support via sysfs - depends on EFI - default n - help - If you say Y here, you are able to get EFI (Extensible Firmware - Interface) variable information via sysfs. You may read, - write, create, and destroy EFI variables through this interface. - - Note that using this driver in concert with efibootmgr requires - at least test release version 0.5.0-test3 or later, which is - available from Matt Domsch's website located at: - http://linux.dell.com/efibootmgr/testing/efibootmgr-0.5.0-test3.tar.gz - - Subsequent efibootmgr releases may be found at: - http://linux.dell.com/efibootmgr - config EFI_PCDP bool Console device selection via EFI PCDP or HCDP table depends on ACPI EFI IA64 @@ -146,5 +129,6 @@ config ISCSI_IBFT Otherwise, say N. source drivers/firmware/google/Kconfig +source drivers/firmware/efi/Kconfig endmenu diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile index 5a7e273..31bf68c 100644 --- a/drivers/firmware/Makefile +++ b/drivers/firmware/Makefile @@ -14,3 +14,4 @@ obj-$(CONFIG_ISCSI_IBFT) += iscsi_ibft.o obj-$(CONFIG_FIRMWARE_MEMMAP) += memmap.o obj-$(CONFIG_GOOGLE_FIRMWARE) += google/ +obj-$(CONFIG_EFI) += efi/ diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig new file mode 100644 index 000..a4f213c --- /dev/null +++ b/drivers/firmware/efi/Kconfig @@ -0,0 +1,32 @@ +menu EFI (Extensible Firmware Interface) Support + depends on EFI + +config EFI_VARS + bool EFI Variable Support + depends on EFI + default n + help + This option enables the EFI variable support code. + + This is required for all EFI variable drivers. + + If unsure, say N. + +config EFI_VARS_SYSFS + tristate EFI Variable Support via sysfs + depends on EFI_VARS + default n + help + If you say Y here, you are able to get EFI (Extensible Firmware + Interface) variable information via sysfs. You may read, + write, create, and destroy EFI variables through this interface. + + Note that using this driver in concert with efibootmgr requires + at least test release version 0.5.0-test3 or later, which is + available from Matt Domsch's website located at: + http://linux.dell.com/efibootmgr/testing/efibootmgr-0.5.0-test3.tar.gz + + Subsequent efibootmgr releases may be found at: + http://linux.dell.com/efibootmgr + +endmenu diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile new file mode 100644 index 000..73ec68b --- /dev/null +++ b/drivers/firmware/efi/Makefile @@ -0,0 +1,4 @@ +# +# Makefile for linux kernel +# +obj-$(CONFIG_EFI_VARS_SYSFS) += sysfs.o diff --git a/drivers/firmware/efi/sysfs.c b/drivers/firmware/efi/sysfs.c new file mode 100644 index 000..d066eb5 --- /dev/null +++ b/drivers/firmware/efi/sysfs.c @@ -0,0 +1,549 @@ +/*
[RFC][PATCH 5/7] efivars: Move pstore code out of efivars.c
From: Matt Fleming matt.flem...@intel.com
Move the persistence storage code to efi/pstore.c now that it uses the
new efivar API, helping us to reduce the size of efivars.c. This move
also allows us to delete the code that was previously necessary when
CONFIG_PSTORE was disabled.
Cc: Anton Vorontsov cbouatmai...@gmail.com
Cc: Colin Cross ccr...@android.com
Cc: Kees Cook keesc...@chromium.org
Cc: Matthew Garrett mj...@srcf.ucam.org
Cc: Tony Luck tony.l...@intel.com
Signed-off-by: Matt Fleming matt.flem...@intel.com
---
Folks, I haven't put a copyright notice at the top of pstore.c. Please
suggest one and I'll incorporate it.
MAINTAINERS | 2 +-
drivers/firmware/efi/Kconfig | 12 ++
drivers/firmware/efi/Makefile | 1 +
drivers/firmware/efi/pstore.c | 239 +++
drivers/firmware/efivars.c| 287 --
include/linux/efi.h | 28 +
6 files changed, 281 insertions(+), 288 deletions(-)
create mode 100644 drivers/firmware/efi/pstore.c
diff --git a/MAINTAINERS b/MAINTAINERS
index e37219d..c31e5a4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6091,7 +6091,7 @@ S:Maintained
T: git git://git.infradead.org/users/cbou/linux-pstore.git
F: fs/pstore/
F: include/linux/pstore*
-F: drivers/firmware/efivars.c
+F: drivers/firmware/efi/pstore.c
F: drivers/acpi/apei/erst.c
PTP HARDWARE CLOCK SUPPORT
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index a4f213c..f664de2 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -29,4 +29,16 @@ config EFI_VARS_SYSFS
Subsequent efibootmgr releases may be found at:
http://linux.dell.com/efibootmgr
+config EFI_VARS_PSTORE
+ tristate EFI Variable Persistent Storage
+ depends on PSTORE
+ depends on EFI_VARS_SYSFS
+ default n
+ help
+ This option enables panic and oops messages to be stored in
+ EFI variables, which allows them to be examined after the
+ machine has been rebooted.
+
+ If unsure, say N.
+
endmenu
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 73ec68b..0b8d7e0 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -2,3 +2,4 @@
# Makefile for linux kernel
#
obj-$(CONFIG_EFI_VARS_SYSFS) += sysfs.o
+obj-$(CONFIG_EFI_VARS_PSTORE) += pstore.o
diff --git a/drivers/firmware/efi/pstore.c b/drivers/firmware/efi/pstore.c
new file mode 100644
index 000..8924f29
--- /dev/null
+++ b/drivers/firmware/efi/pstore.c
@@ -0,0 +1,239 @@
+#include linux/efi.h
+#include linux/module.h
+#include linux/pstore.h
+
+#define DUMP_NAME_LEN 52
+
+#define PSTORE_EFI_ATTRIBUTES \
+ (EFI_VARIABLE_NON_VOLATILE | \
+EFI_VARIABLE_BOOTSERVICE_ACCESS | \
+EFI_VARIABLE_RUNTIME_ACCESS)
+
+static int efi_pstore_open(struct pstore_info *psi)
+{
+ efivar_entry_iter_begin();
+ psi-data = NULL;
+ return 0;
+}
+
+static int efi_pstore_close(struct pstore_info *psi)
+{
+ efivar_entry_iter_end();
+ psi-data = NULL;
+ return 0;
+}
+
+struct pstore_read_data {
+ u64 *id;
+ enum pstore_type_id *type;
+ int *count;
+ struct timespec *timespec;
+ char **buf;
+};
+
+static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
+{
+ struct pstore_read_data *cb_data = data;
+ char name[DUMP_NAME_LEN];
+ int i;
+ int cnt;
+ unsigned int part;
+ unsigned long time, size;
+ efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
+
+ if (efi_guidcmp(entry-var.VendorGuid, vendor))
+ return 0;
+
+ for (i = 0; i DUMP_NAME_LEN; i++)
+ name[i] = entry-var.VariableName[i];
+
+ if (sscanf(name, dump-type%u-%u-%d-%lu,
+ cb_data-type, part, cnt, time) == 4) {
+ *cb_data-id = part;
+ *cb_data-count = cnt;
+ cb_data-timespec-tv_sec = time;
+ cb_data-timespec-tv_nsec = 0;
+ } else if (sscanf(name, dump-type%u-%u-%lu,
+ cb_data-type, part, time) == 3) {
+ /*
+* Check if an old format,
+* which doesn't support holding
+* multiple logs, remains.
+*/
+ *cb_data-id = part;
+ *cb_data-count = 0;
+ cb_data-timespec-tv_sec = time;
+ cb_data-timespec-tv_nsec = 0;
+ } else
+ return 0;
+
+ efivar_entry_size(entry, size);
+ *cb_data-buf = kmalloc(size, GFP_KERNEL);
+ if (*cb_data-buf == NULL)
+ return -ENOMEM;
+ memcpy(*cb_data-buf, entry-var.Data, size);
+ return size;
+}
+
+static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
+ int *count, struct timespec *timespec,
+
[RFC][PATCH 6/7] efi: Add generic variable operations
From: Matt Fleming matt.flem...@intel.com
Some machines have an EFI variable interface that does not conform to
the UEFI specification, e.g. CONFIG_GOOGLE_SMI. Add the necessary code
and Kconfig glue so that it's only possible to choose one set of EFI
variable operations.
Cc: Mike Waychison mi...@google.com
Signed-off-by: Matt Fleming matt.flem...@intel.com
---
drivers/firmware/efi/Kconfig | 12 +
drivers/firmware/efi/Makefile | 1 +
drivers/firmware/efi/generic-ops.c | 52 ++
drivers/firmware/efivars.c | 19 +-
drivers/firmware/google/Kconfig| 6 ++---
5 files changed, 69 insertions(+), 21 deletions(-)
create mode 100644 drivers/firmware/efi/generic-ops.c
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f664de2..e83ef8f 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -12,9 +12,21 @@ config EFI_VARS
If unsure, say N.
+config EFI_VARS_GENERIC_OPS
+ bool Generic EFI Variable Operations
+ depends on EFI_VARS
+ default y
+ help
+ This option enables the use of the generic EFI variable
+ operations, those that are compliant with the UEFI
+ specification.
+
+ If unsure, say Y.
+
config EFI_VARS_SYSFS
tristate EFI Variable Support via sysfs
depends on EFI_VARS
+ depends on EFI_VARS_GENERIC_OPS || GOOGLE_SMI
default n
help
If you say Y here, you are able to get EFI (Extensible Firmware
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 0b8d7e0..ef5066f 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -3,3 +3,4 @@
#
obj-$(CONFIG_EFI_VARS_SYSFS) += sysfs.o
obj-$(CONFIG_EFI_VARS_PSTORE) += pstore.o
+obj-$(CONFIG_EFI_VARS_GENERIC_OPS) += generic-ops.o
diff --git a/drivers/firmware/efi/generic-ops.c
b/drivers/firmware/efi/generic-ops.c
new file mode 100644
index 000..b474800
--- /dev/null
+++ b/drivers/firmware/efi/generic-ops.c
@@ -0,0 +1,52 @@
+/*
+ * Copyright (C) 2013 Intel Corporation matt.flem...@intel.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include linux/efi.h
+#include linux/kconfig.h
+
+extern struct kobject *efi_kobj;
+
+static struct efivars generic_efivars;
+static struct efivar_operations generic_ops;
+
+int generic_register(void)
+{
+ int error;
+
+ generic_ops.get_variable = efi.get_variable;
+ generic_ops.set_variable = efi.set_variable;
+ generic_ops.get_next_variable = efi.get_next_variable;
+ generic_ops.query_variable_info = efi.query_variable_info;
+
+ error = efivars_register(generic_efivars, generic_ops);
+ if (error)
+ return error;
+
+#if defined(CONFIG_EFI_VARS_SYSFS) || defined(CONFIG_EFI_VARS_SYSFS_MODULE)
+ efivars_sysfs_init(efi_kobj);
+#endif
+ return 0;
+}
+
+void generic_unregister(void)
+{
+ efivars_unregister(generic_efivars);
+}
+
+module_init(generic_register);
+module_exit(generic_unregister);
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index eaaddae..c2275b8 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -40,9 +40,6 @@
#include asm/uaccess.h
-static struct efivars generic_efivars;
-static struct efivar_operations generic_ops;
-
/* Private pointer to registered efivars */
static struct efivars *__efivars;
@@ -791,7 +788,7 @@ static struct attribute_group efi_subsys_attr_group = {
.attrs = efi_subsys_attrs,
};
-static struct kobject *efi_kobj;
+struct kobject *efi_kobj;
/**
* efivar_init - build the initial list of EFI variables
@@ -1358,19 +1355,6 @@ efivars_init(void)
return -ENOMEM;
}
- generic_ops.get_variable = efi.get_variable;
- generic_ops.set_variable = efi.set_variable;
- generic_ops.get_next_variable = efi.get_next_variable;
- generic_ops.query_variable_info = efi.query_variable_info;
-
- error = efivars_register(generic_efivars, generic_ops);
- if (error)
- return error;
-
-#if defined(CONFIG_EFI_VARS_SYSFS) || defined(CONFIG_EFI_VARS_SYSFS_MODULE)
- efivars_sysfs_init(efi_kobj);
[RFC][PATCH 7/7] efivarfs: Move to fs/efivarfs
From: Matt Fleming matt.flem...@intel.com
Now that efivarfs uses the efivar API, move it out of efivars.c and
into fs/efivarfs where it belongs. This move allows us to turn on
other EFI varible code, like CONFIG_EFI_VARS_SYSFS without requiring
that the efivarfs code also be built, and vice versa.
Cc: Matthew Garrett matthew.garr...@nebula.com
Cc: Jeremy Kerr jeremy.k...@canonical.com
Signed-off-by: Matt Fleming matt.flem...@intel.com
---
MAINTAINERS| 9 +
drivers/firmware/efivars.c | 487 -
fs/Kconfig | 1 +
fs/Makefile| 1 +
fs/efivarfs/Kconfig| 12 ++
fs/efivarfs/Makefile | 7 +
fs/efivarfs/file.c | 130
fs/efivarfs/inode.c| 179 +
fs/efivarfs/internal.h | 18 ++
fs/efivarfs/super.c| 233 ++
10 files changed, 590 insertions(+), 487 deletions(-)
create mode 100644 fs/efivarfs/Kconfig
create mode 100644 fs/efivarfs/Makefile
create mode 100644 fs/efivarfs/file.c
create mode 100644 fs/efivarfs/inode.c
create mode 100644 fs/efivarfs/internal.h
create mode 100644 fs/efivarfs/super.c
diff --git a/MAINTAINERS b/MAINTAINERS
index c31e5a4..d2b3b29 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2895,6 +2895,15 @@ F: drivers/firmware/efivars.c
F: drivers/firmware/efi/*
F: include/linux/efi*.h
+EFI VARIABLE FILESYSTEM
+M: Matthew Garrett matthew.garr...@nebula.com
+M: Jeremy Kerr jeremy.k...@canonical.com
+M: Matt Fleming matt.flem...@intel.com
+T: git git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
+L: linux-efi@vger.kernel.org
+S: Maintained
+F: fs/efivarfs/
+
EFIFB FRAMEBUFFER DRIVER
L: linux-fb...@vger.kernel.org
M: Peter Jones pjo...@redhat.com
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index c2275b8..3445334 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -228,12 +228,6 @@ efivar_validate(struct efi_variable *var, u8 *data,
unsigned long len)
}
EXPORT_SYMBOL_GPL(efivar_validate);
-static int efivarfs_file_open(struct inode *inode, struct file *file)
-{
- file-private_data = inode-i_private;
- return 0;
-}
-
static int efi_status_to_err(efi_status_t status)
{
int err;
@@ -267,485 +261,6 @@ static int efi_status_to_err(efi_status_t status)
return err;
}
-static ssize_t efivarfs_file_write(struct file *file,
- const char __user *userbuf, size_t count, loff_t *ppos)
-{
- struct efivar_entry *var = file-private_data;
- void *data;
- u32 attributes;
- struct inode *inode = file-f_mapping-host;
- unsigned long datasize = count - sizeof(attributes);
- u64 storage_size, remaining_size, max_size;
- ssize_t bytes = 0;
- bool set = false;
- int err;
-
- if (count sizeof(attributes))
- return -EINVAL;
-
- if (copy_from_user(attributes, userbuf, sizeof(attributes)))
- return -EFAULT;
-
- if (attributes ~(EFI_VARIABLE_MASK))
- return -EINVAL;
-
- /*
-* Ensure that the user can't allocate arbitrarily large
-* amounts of memory. Pick a default size of 64K if
-* QueryVariableInfo() isn't supported by the firmware.
-*/
- err = efivar_query_info(attributes, storage_size,
- remaining_size, max_size);
- if (err) {
- if (err != -ENOSYS)
- return err;
-
- remaining_size = 65536;
- }
-
- if (datasize remaining_size)
- return -ENOSPC;
-
- data = kmalloc(datasize, GFP_KERNEL);
- if (!data)
- return -ENOMEM;
-
- if (copy_from_user(data, userbuf + sizeof(attributes), datasize)) {
- bytes = -EFAULT;
- goto out;
- }
-
- bytes = efivar_entry_set_get_size(var, attributes, datasize,
- data, set);
- if (!set bytes)
- goto out;
-
- if (bytes == -ENOENT) {
- drop_nlink(inode);
- d_delete(file-f_dentry);
- dput(file-f_dentry);
- } else {
- mutex_lock(inode-i_mutex);
- i_size_write(inode, datasize + sizeof(attributes));
- mutex_unlock(inode-i_mutex);
- }
-
- bytes = count;
-
-out:
- kfree(data);
-
- return bytes;
-}
-
-static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
- size_t count, loff_t *ppos)
-{
- struct efivar_entry *var = file-private_data;
- unsigned long datasize = 0;
- u32 attributes;
- void *data;
- ssize_t size = 0;
- int err;
-
- err = efivar_entry_size(var, datasize);
- if (err)
- return err;
-
- data = kmalloc(datasize +
Re: [PATCH] x86: Lock down MSR writing in secure boot
On Fri, 2013-02-08 at 13:02 -0800, Kees Cook wrote: I don't find it unreasonable to drop all caps and lose access to sensitive things. :) That's sort of the point, really. I think a cap is the best match. It seems like it should either be a cap or a namespace flag, but the latter seems messy. Yeah, I think it's an expected outcome, but it means that if (say) qemu drops privileges, qemu can no longer access PCI resources - even on non-secure boot systems. That breaks existing userspace.
Re: [PATCH] x86: Lock down MSR writing in secure boot
On Fri, Feb 8, 2013 at 4:07 PM, Matthew Garrett matthew.garr...@nebula.com wrote: On Fri, 2013-02-08 at 13:02 -0800, Kees Cook wrote: I don't find it unreasonable to drop all caps and lose access to sensitive things. :) That's sort of the point, really. I think a cap is the best match. It seems like it should either be a cap or a namespace flag, but the latter seems messy. Yeah, I think it's an expected outcome, but it means that if (say) qemu drops privileges, qemu can no longer access PCI resources - even on non-secure boot systems. That breaks existing userspace. Right. We've had a few reports in Fedora of things breaking on non-SB systems because of this. The qemu one is the latest, but the general problem is people think dropping all caps blindly is making their apps safer. Then they find they can't do things they could do before the new cap was added. It's messy. I've thought of treating CAP_COMPROMISE_KERNEL as a hidden cap, where only the kernel can grant or drop it. Peter Jones suggested it might work to allow it to be dropped iff it is the only cap being changed. Either way, it's a special cap and I have no idea how acceptable something like that would be. Really though, the main issue is that you cannot introduce new caps to enforce finer grained access without breaking something. josh -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC][PATCH 3/7] efivars: New efivar_entry API
Matt, There isn't really a formalised interface for dealing with EFI variables or struct efivar_entry. This has led to the efivarfs code directly accessing '__efivars'. I agree with you. I have some comment on a pstore part. @@ -1363,86 +1195,90 @@ static int efi_pstore_write(enum pstore_type_id type, for (i = 0; i DUMP_NAME_LEN; i++) efi_name[i] = name[i]; - efivars-ops-set_variable(efi_name, vendor, PSTORE_EFI_ATTRIBUTES, -size, psi-buf); + entry = kzalloc(sizeof(*entry), GFP_KERNEL); + if (!entry) + return -ENOMEM; Please don't allocate memory dynamically in the efi_pstore_write() because efi_pstore_write() is called in an interrupt context like oops and panic. Also, a whole process of query_info() + entry_set() should be protected by a single spin lock() Because if efi_pstore_write() runs concurrently, a result of query_info() may be meaningless... So, how about introducing query_info_nolock and entry_set_nolock()? spin_lock() query_info_nolock() entry_set_nolock() spin_unlock() IMO, efivarfs_file_write() should be fixed as above. As for efi_pstore_erase() and efi_pstore_read(), it will take some time to fully understand your change. Seiji -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86: Lock down MSR writing in secure boot
On 02/08/2013 01:02 PM, Kees Cook wrote: On Fri, Feb 8, 2013 at 12:34 PM, Matthew Garrett matthew.garr...@nebula.com wrote: On Fri, 2013-02-08 at 12:28 -0800, Kees Cook wrote: Maybe a capability isn't the right way to go, I'm not sure. I'll leave that to Matthew. Whatever the flag, it should be an immutable state of the boot. Though, it probably makes sense as a cap just so that non-secure-boot systems can still remove it from containers, etc. There was interest in ensuring that this wasn't something special-cased to UEFI Secure Boot, so using a capability seemed like the most straightforward way - it's fundamentally a restriction on what an otherwise privileged user is able to do, so it seemed like it fit the model. But I'm not wed to it in the slightest, and in fact it causes problems for some userspace (anything that drops all capabilities suddenly finds itself unable to do something that it expects to be able to do), so if anyone has any suggestions for a better approach… I don't find it unreasonable to drop all caps and lose access to sensitive things. :) That's sort of the point, really. I think a cap is the best match. It seems like it should either be a cap or a namespace flag, but the latter seems messy. Caps are fine; the problem is the putting it all under one cap. The semi-problem here is that to preserve backwards compatibility we really should have a way to have hierarchical caps in Linux (which we currently don't), but it is not really an issue for this. Also, keep in mind that there is a very simple way to deny MSR access completely, which is to not include the driver in your kernel (and not allow module loading, but if you can load modules you can just load a module to muck with whatever MSR you want.) I am still wondering if there are any legitimate uses of CAP_RAWIO ~CAP_COMPROMISE_KERNEL that can't be used to subvert the latter. I am not sure there are. -hpa -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 -next 0/2] make efivars/efi_pstore interrupt-safe
On Fri, 2013-02-08 at 22:31 +, Seiji Aguchi wrote: Matt, Can you please take a look at this patchset which removes create_sysfs_entries() from efi_pstore_write()? It has been updated in accordance with Mike's comment and fixes an actual bug. http://comments.gmane.org/gmane.linux.kernel.efi/406 OK, I'll find some time to review these. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86: Lock down MSR writing in secure boot
On Fri, Feb 08, 2013 at 02:30:52PM -0800, H. Peter Anvin wrote: Also, keep in mind that there is a very simple way to deny MSR access completely, which is to not include the driver in your kernel (and not allow module loading, but if you can load modules you can just load a module to muck with whatever MSR you want.) I was contemplating that too. What is the use case of having msr.ko in a secure boot environment? Isn't that an all-no-tools, you-can't-do-sh*t-except-what-you're-explicitly-allowed-to environment which simply doesn't need to write MSRs in the first place? -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86: Lock down MSR writing in secure boot
On 02/08/2013 01:14 PM, Josh Boyer wrote: On Fri, Feb 8, 2013 at 4:07 PM, Matthew Garrett matthew.garrett-05XSO3Yj/jvqt0dzr+a...@public.gmane.org wrote: On Fri, 2013-02-08 at 13:02 -0800, Kees Cook wrote: I don't find it unreasonable to drop all caps and lose access to sensitive things. :) That's sort of the point, really. I think a cap is the best match. It seems like it should either be a cap or a namespace flag, but the latter seems messy. Yeah, I think it's an expected outcome, but it means that if (say) qemu drops privileges, qemu can no longer access PCI resources - even on non-secure boot systems. That breaks existing userspace. Right. We've had a few reports in Fedora of things breaking on non-SB systems because of this. The qemu one is the latest, but the general problem is people think dropping all caps blindly is making their apps safer. Then they find they can't do things they could do before the new cap was added. It's messy. Why not require CAP_COMPROMISE_KERNEL to open (with O_RDWR or O_WRONLY) /dev/msr? After all, sudo /dev/null /dev/msr will cause a privileged write() call on the fd as long as the capability is in your bounding set. --Andy -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC][PATCH 3/7] efivars: New efivar_entry API
It's probably still possible to have a CPU creating an EFI variable via the
sysfs files while another CPU is in the pstore write path, but I'm
not convinced making the query_variable_info() and set_variable() atomic is
something that we need to worry about.
When I talked about query_variable_info() with Tony,
Matthew commented as follows.
http://marc.info/?l=linux-kernelm=134305325801789w=2
snip
Running out of space in EFI isn't a well-tested scenario, and I wouldn't
expect all firmware to handle it gracefully. This is made worse by EFI 1
not providing any information about available storage.
snip
I'm just concerned that set_variable() may run in the out of space situation
if query_variable_info() and set_variable() are not atomic.
Seiji
--
Matt Fleming, Intel Open Source Technology Center
N�r��yb�X��ǧv�^�){.n�+{�y^n�r���z���h��� �G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�
Re: [PATCH] x86: Lock down MSR writing in secure boot
On Sat, 2013-02-09 at 00:06 +0100, Borislav Petkov wrote: On Fri, Feb 08, 2013 at 02:30:52PM -0800, H. Peter Anvin wrote: Also, keep in mind that there is a very simple way to deny MSR access completely, which is to not include the driver in your kernel (and not allow module loading, but if you can load modules you can just load a module to muck with whatever MSR you want.) I was contemplating that too. What is the use case of having msr.ko in a secure boot environment? Isn't that an all-no-tools, you-can't-do-sh*t-except-what-you're-explicitly-allowed-to environment which simply doesn't need to write MSRs in the first place? Well, sure, distributions could build every kernel twice. That seems a little excessive, though.
Re: [PATCH] x86: Lock down MSR writing in secure boot
On Fri, 2013-02-08 at 17:22 -0800, H. Peter Anvin wrote: You don't have to build the kernel twice to exclude a loadable module. I guess you could just strip the signatures off any modules you don't want to support under Secure Boot, but that breaks some other use cases.
Re: [PATCH] x86: Lock down MSR writing in secure boot
On Fri, Feb 8, 2013 at 5:29 PM, Matthew Garrett matthew.garr...@nebula.com wrote: On Fri, 2013-02-08 at 17:22 -0800, H. Peter Anvin wrote: You don't have to build the kernel twice to exclude a loadable module. I guess you could just strip the signatures off any modules you don't want to support under Secure Boot, but that breaks some other use cases. Also, _reading_ MSRs from userspace arguably has utility that doesn't compromise ring-0. So excluding the driver entirely seems like overkill. -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
