[PATCH 1/5] Add ucs2 -> utf8 helper functions

[PATCH 1/5] Add ucs2 -> utf8 helper functions

[Peter Jones]

This adds ucs2_utf8size(), which tells us how big our ucs2 string is in
bytes, and ucs2_as_utf8, which translates from ucs2 to utf8..
---
 include/linux/ucs2_string.h |  4 +++
 lib/ucs2_string.c           | 62 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/include/linux/ucs2_string.h b/include/linux/ucs2_string.h
index cbb20af..bb679b4 100644
--- a/include/linux/ucs2_string.h
+++ b/include/linux/ucs2_string.h
@@ -11,4 +11,8 @@ unsigned long ucs2_strlen(const ucs2_char_t *s);
 unsigned long ucs2_strsize(const ucs2_char_t *data, unsigned long maxlength);
 int ucs2_strncmp(const ucs2_char_t *a, const ucs2_char_t *b, size_t len);
 
+unsigned long ucs2_utf8size(const ucs2_char_t *src);
+unsigned long ucs2_as_utf8(u8 *dest, const ucs2_char_t *src,
+			   unsigned long maxlength);
+
 #endif /* _LINUX_UCS2_STRING_H_ */
diff --git a/lib/ucs2_string.c b/lib/ucs2_string.c
index 6f500ef..17dd74e 100644
--- a/lib/ucs2_string.c
+++ b/lib/ucs2_string.c
@@ -49,3 +49,65 @@ ucs2_strncmp(const ucs2_char_t *a, const ucs2_char_t *b, size_t len)
         }
 }
 EXPORT_SYMBOL(ucs2_strncmp);
+
+unsigned long
+ucs2_utf8size(const ucs2_char_t *src)
+{
+	unsigned long i;
+	unsigned long j = 0;
+
+	for (i = 0; i < ucs2_strlen(src); i++) {
+		u16 c = src[i];
+
+		if (c > 0x800)
+			j += 3;
+		else if (c > 0x80)
+			j += 2;
+		else
+			j += 1;
+	}
+
+	return j;
+}
+EXPORT_SYMBOL(ucs2_utf8size);
+
+/*
+ * copy at most maxlength bytes of whole utf8 characters to dest from the
+ * ucs2 string src.
+ *
+ * The return value is the number of characters copied, not including the
+ * final NUL character.
+ */
+unsigned long
+ucs2_as_utf8(u8 *dest, const ucs2_char_t *src, unsigned long maxlength)
+{
+	unsigned int i;
+	unsigned long j = 0;
+	unsigned long limit = ucs2_strnlen(src, maxlength);
+
+	for (i = 0; maxlength && i < limit; i++) {
+		u16 c = src[i];
+
+		if (c > 0x800) {
+			if (maxlength < 3)
+				break;
+			maxlength -= 3;
+			dest[j++] = 0xe0 | (c & 0xf000) >> 12;
+			dest[j++] = 0x80 | (c & 0x0fc0) >> 8;
+			dest[j++] = 0x80 | (c & 0x003f);
+		} else if (c > 0x80) {
+			if (maxlength < 2)
+				break;
+			maxlength -= 2;
+			dest[j++] = 0xc0 | (c & 0xfe0) >> 5;
+			dest[j++] = 0x80 | (c & 0x01f);
+		} else {
+			maxlength -= 1;
+			dest[j++] = c & 0x7f;
+		}
+	}
+	if (maxlength)
+		dest[j] = '\0';
+	return j;
+}
+EXPORT_SYMBOL(ucs2_as_utf8);
-- 
2.5.0

[PATCH 2/5] efi: use ucs2_as_utf8 in efivarfs instead of open coding a bad version

[Peter Jones]

Translate EFI's UCS-2 variable names to UTF-8 instead of just assuming
all variable names fit in ASCII.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/firmware/efi/efivars.c | 13 ++++---------
 fs/efivarfs/super.c            |  7 +++----
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index 756eca8..eef6331 100644
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -540,7 +540,7 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
 static int
 efivar_create_sysfs_entry(struct efivar_entry *new_var)
 {
-	int i, short_name_size;
+	int short_name_size;
 	char *short_name;
 	unsigned long variable_name_size;
 	efi_char16_t *variable_name;
@@ -553,22 +553,17 @@ efivar_create_sysfs_entry(struct efivar_entry *new_var)
 	 * Length of the variable bytes in ASCII, plus the '-' separator,
 	 * plus the GUID, plus trailing NUL
 	 */
-	short_name_size = variable_name_size / sizeof(efi_char16_t)
-				+ 1 + EFI_VARIABLE_GUID_LEN + 1;
+	short_name_size = ucs2_utf8size(new_var->var.VariableName) + 1;
 
 	short_name = kzalloc(short_name_size, GFP_KERNEL);
 
 	if (!short_name)
 		return -ENOMEM;
 
-	/* Convert Unicode to normal chars (assume top bits are 0),
-	   ala UTF-8 */
-	for (i=0; i < (int)(variable_name_size / sizeof(efi_char16_t)); i++) {
-		short_name[i] = variable_name[i] & 0xFF;
-	}
+	ucs2_as_utf8(short_name, variable_name, short_name_size);
+
 	/* This is ugly, but necessary to separate one vendor's
 	   private variables from another's.         */
-
 	*(short_name + strlen(short_name)) = '-';
 	efi_guid_to_str(&new_var->var.VendorGuid,
 			 short_name + strlen(short_name));
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index b8a564f..8651ac2 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -118,7 +118,7 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 	struct dentry *dentry, *root = sb->s_root;
 	unsigned long size = 0;
 	char *name;
-	int len, i;
+	int len;
 	int err = -ENOMEM;
 
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
@@ -128,15 +128,14 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 	memcpy(entry->var.VariableName, name16, name_size);
 	memcpy(&(entry->var.VendorGuid), &vendor, sizeof(efi_guid_t));
 
-	len = ucs2_strlen(entry->var.VariableName);
+	len = ucs2_utf8size(entry->var.VariableName);
 
 	/* name, plus '-', plus GUID, plus NUL*/
 	name = kmalloc(len + 1 + EFI_VARIABLE_GUID_LEN + 1, GFP_KERNEL);
 	if (!name)
 		goto fail;
 
-	for (i = 0; i < len; i++)
-		name[i] = entry->var.VariableName[i] & 0xFF;
+	ucs2_as_utf8(name, entry->var.VariableName, len);
 
 	name[len] = '-';
 
-- 
2.5.0

Re: [PATCH 2/5] efi: use ucs2_as_utf8 in efivarfs instead of open coding a bad version

[Matt Fleming]

On Wed, 03 Feb, at 08:02:44AM, Peter Jones wrote:
> Translate EFI's UCS-2 variable names to UTF-8 instead of just assuming
> all variable names fit in ASCII.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/firmware/efi/efivars.c | 13 ++++---------
>  fs/efivarfs/super.c            |  7 +++----
>  2 files changed, 7 insertions(+), 13 deletions(-)

This patch causes the following Oops on my test grid,

[    1.331926] EFI Variables Facility v0.08 2004-May-17
[    1.341570] hidraw: raw HID events driver (C) Jiri Kosina
[    1.343291] general protection fault: 0000 [#1] SMP 
[    1.343400] Modules linked in:
[    1.343550] CPU: 1 PID: 181 Comm: kworker/u4:4 Not tainted 4.4.0-rc2+ #1
[    1.343726] Workqueue: events_unbound call_usermodehelper_exec_work
[    1.343821] task: ffff88003f84d080 ti: ffff88003df48000 task.ti: ffff88003df48000
[    1.343915] RIP: 0010:[<ffffffff8116399c>]  [<ffffffff8116399c>] __kmalloc_track_caller+0x8c/0x170
[    1.344039] RSP: 0018:ffff88003df4bbc8  EFLAGS: 00000286
[    1.344039] RAX: 0000000000000000 RBX: 0000000000000018 RCX: 0000000000000d46
[    1.344039] RDX: 0000000000000d45 RSI: 0000000000000000 RDI: 0000000000000002
[    1.344039] RBP: ffff88003df4bbf8 R08: 00000000000182e0 R09: 000000003fb0f401
[    1.344039] R10: 0000000000000003 R11: ffff88003df99480 R12: 00000000024000c0
[    1.344039] R13: 0000000000000018 R14: 3061612d32643131 R15: ffff88003dc01c00
[    1.344039] FS:  0000000000000000(0000) GS:ffff88003e100000(0000) knlGS:0000000000000000
[    1.344039] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[    1.344039] CR2: 0000000000000000 CR3: 0000000001e0b000 CR4: 00000000000006e0
[    1.344039] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    1.344039] DR3: 0000000000000000 DR6: 0000000000000000 DR7: 0000000000000000
[    1.344039] Stack:
[    1.344039]  ffffffff812adda6 0000000000000018 ffff88003df8b480 ffff88003dee0780
[    1.344039]  ffff88003fb0f480 ffffffff81065ed0 ffff88003df4bc18 ffffffff811304fb
[    1.344039]  ffff88003fb0f480 00000000024000c0 ffff88003df4bc30 ffffffff812adda6
[    1.344039] Call Trace:
[    1.344039]  [<ffffffff812adda6>] ? selinux_cred_prepare+0x16/0x30
[    1.344039]  [<ffffffff81065ed0>] ? call_usermodehelper_exec_work+0xb0/0xb0
[    1.344039]  [<ffffffff811304fb>] kmemdup+0x1b/0x40
[    1.344039]  [<ffffffff812adda6>] selinux_cred_prepare+0x16/0x30
[    1.344039]  [<ffffffff812a9c9e>] security_prepare_creds+0x3e/0x60
[    1.344039]  [<ffffffff8107077d>] prepare_creds+0xdd/0x180
[    1.344039]  [<ffffffff81070cc2>] copy_creds+0x22/0x110
[    1.344039]  [<ffffffff81051771>] copy_process+0x311/0x1dc0
[    1.344039]  [<ffffffff81035c22>] ? native_smp_send_reschedule+0x42/0x60
[    1.344039]  [<ffffffff8107722a>] ? resched_curr+0x8a/0xb0
[    1.344039]  [<ffffffff8105338d>] _do_fork+0x7d/0x2d0
[    1.344039]  [<ffffffff8108525e>] ? pick_next_task_fair+0x3fe/0x460
[    1.344039]  [<ffffffff81053604>] kernel_thread+0x24/0x30
[    1.344039]  [<ffffffff81065e46>] call_usermodehelper_exec_work+0x26/0xb0
[    1.344039]  [<ffffffff8186def3>] ? __schedule+0x313/0x870
[    1.344039]  [<ffffffff8106996e>] process_one_work+0x13e/0x3c0
[    1.344039]  [<ffffffff81069d05>] worker_thread+0x115/0x450
[    1.344039]  [<ffffffff8186def3>] ? __schedule+0x313/0x870
[    1.344039]  [<ffffffff81069bf0>] ? process_one_work+0x3c0/0x3c0
[    1.344039]  [<ffffffff8106ed64>] kthread+0xc4/0xe0
[    1.344039]  [<ffffffff8106eca0>] ? kthread_park+0x50/0x50
[    1.344039]  [<ffffffff81871adf>] ret_from_fork+0x3f/0x70
[    1.344039]  [<ffffffff8106eca0>] ? kthread_park+0x50/0x50
[    1.344039] Code: 4c 03 05 a0 67 ea 7e 4d 8b 30 49 8b 40 10 4d 85 f6 0f 84 8e 00 00 00 48 85 c0 0f 84 85 00 00 00 49 63 47 20 48 8d 4a 01 4d 8b 07 <49> 8b 1c 06 4c 89 f0 65 49 0f c7 08 0f 94 c0 84 c0 74 b9 49 63 
[    1.344039] RIP  [<ffffffff8116399c>] __kmalloc_track_caller+0x8c/0x170
[    1.344039]  RSP <ffff88003df4bbc8>
[    1.348190] ---[ end trace ed036c029f24ae69 ]---

I suspect the length calculations we're doing are now wrong and we're
overwriting kmalloc metadata, probably in the efivars code.

[PATCH] efi: use ucs2_as_utf8 in efivarfs instead of open coding a bad version (v2)

[Peter Jones]

Translate EFI's UCS-2 variable names to UTF-8 instead of just assuming
all variable names fit in ASCII.

v2: fix where I'd accidentally left some of the sizes off when
    converting the variable name strings.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/firmware/efi/efivars.c | 14 +++++---------
 fs/efivarfs/super.c            |  7 +++----
 2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index 756eca8..cd53c4d 100644
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -540,7 +540,7 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
 static int
 efivar_create_sysfs_entry(struct efivar_entry *new_var)
 {
-	int i, short_name_size;
+	int short_name_size;
 	char *short_name;
 	unsigned long variable_name_size;
 	efi_char16_t *variable_name;
@@ -553,22 +553,18 @@ efivar_create_sysfs_entry(struct efivar_entry *new_var)
 	 * Length of the variable bytes in ASCII, plus the '-' separator,
 	 * plus the GUID, plus trailing NUL
 	 */
-	short_name_size = variable_name_size / sizeof(efi_char16_t)
-				+ 1 + EFI_VARIABLE_GUID_LEN + 1;
+	short_name_size = ucs2_utf8size(new_var->var.VariableName)
+					+ 1 + EFI_VARIABLE_GUID_LEN + 1;
 
 	short_name = kzalloc(short_name_size, GFP_KERNEL);
 
 	if (!short_name)
 		return -ENOMEM;
 
-	/* Convert Unicode to normal chars (assume top bits are 0),
-	   ala UTF-8 */
-	for (i=0; i < (int)(variable_name_size / sizeof(efi_char16_t)); i++) {
-		short_name[i] = variable_name[i] & 0xFF;
-	}
+	ucs2_as_utf8(short_name, variable_name, short_name_size);
+
 	/* This is ugly, but necessary to separate one vendor's
 	   private variables from another's.         */
-
 	*(short_name + strlen(short_name)) = '-';
 	efi_guid_to_str(&new_var->var.VendorGuid,
 			 short_name + strlen(short_name));
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index b8a564f..8651ac2 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -118,7 +118,7 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 	struct dentry *dentry, *root = sb->s_root;
 	unsigned long size = 0;
 	char *name;
-	int len, i;
+	int len;
 	int err = -ENOMEM;
 
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
@@ -128,15 +128,14 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 	memcpy(entry->var.VariableName, name16, name_size);
 	memcpy(&(entry->var.VendorGuid), &vendor, sizeof(efi_guid_t));
 
-	len = ucs2_strlen(entry->var.VariableName);
+	len = ucs2_utf8size(entry->var.VariableName);
 
 	/* name, plus '-', plus GUID, plus NUL*/
 	name = kmalloc(len + 1 + EFI_VARIABLE_GUID_LEN + 1, GFP_KERNEL);
 	if (!name)
 		goto fail;
 
-	for (i = 0; i < len; i++)
-		name[i] = entry->var.VariableName[i] & 0xFF;
+	ucs2_as_utf8(name, entry->var.VariableName, len);
 
 	name[len] = '-';
 
-- 
2.5.0

[PATCH 3/5] efi: do variable name validation tests in utf8

[Peter Jones]

Actually translate from ucs2 to utf8 before doing the test, and then
test against our other utf8 data, instead of fudging it.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/firmware/efi/vars.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 70a0fb1..36c5a6e 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -192,7 +192,15 @@ bool
 efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long len)
 {
 	int i;
-	u16 *unicode_name = var_name;
+	unsigned long utf8_size;
+	u8 *utf8_name;
+
+	utf8_size = ucs2_utf8size(var_name);
+	utf8_name = kmalloc(utf8_size + 1, GFP_KERNEL);
+	if (!utf8_name)
+		return false;
+
+	ucs2_as_utf8(utf8_name, var_name, len);
 
 	for (i = 0; variable_validate[i].validate != NULL; i++) {
 		const char *name = variable_validate[i].name;
@@ -200,28 +208,29 @@ efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long len)
 
 		for (match = 0; ; match++) {
 			char c = name[match];
-			u16 u = unicode_name[match];
-
-			/* All special variables are plain ascii */
-			if (u > 127)
-				return true;
+			char u = utf8_name[match];
 
 			/* Wildcard in the matching name means we've matched */
-			if (c == '*')
+			if (c == '*') {
+				kfree(utf8_name);
 				return variable_validate[i].validate(var_name,
 							     match, data, len);
+			}
 
 			/* Case sensitive match */
 			if (c != u)
 				break;
 
 			/* Reached the end of the string while matching */
-			if (!c)
+			if (!c) {
+				kfree(utf8_name);
 				return variable_validate[i].validate(var_name,
 							     match, data, len);
+			}
 		}
 	}
 
+	kfree(utf8_name);
 	return true;
 }
 EXPORT_SYMBOL_GPL(efivar_validate);
-- 
2.5.0

[PATCH 4/5] efi: make our variable validation list include the guid

[Peter Jones]

We're not actually checking the guid in the validation code, but it
makes it easier to use the list for other things later.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/firmware/efi/vars.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 36c5a6e..e084d08 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -165,27 +165,28 @@ validate_ascii_string(efi_char16_t *var_name, int match, u8 *buffer,
 }
 
 struct variable_validate {
+	efi_guid_t guid;
 	char *name;
 	bool (*validate)(efi_char16_t *var_name, int match, u8 *data,
 			 unsigned long len);
 };
 
 static const struct variable_validate variable_validate[] = {
-	{ "BootNext", validate_uint16 },
-	{ "BootOrder", validate_boot_order },
-	{ "DriverOrder", validate_boot_order },
-	{ "Boot*", validate_load_option },
-	{ "Driver*", validate_load_option },
-	{ "ConIn", validate_device_path },
-	{ "ConInDev", validate_device_path },
-	{ "ConOut", validate_device_path },
-	{ "ConOutDev", validate_device_path },
-	{ "ErrOut", validate_device_path },
-	{ "ErrOutDev", validate_device_path },
-	{ "Timeout", validate_uint16 },
-	{ "Lang", validate_ascii_string },
-	{ "PlatformLang", validate_ascii_string },
-	{ "", NULL },
+	{ EFI_GLOBAL_VARIABLE_GUID, "BootNext", validate_uint16 },
+	{ EFI_GLOBAL_VARIABLE_GUID, "BootOrder", validate_boot_order },
+	{ EFI_GLOBAL_VARIABLE_GUID, "DriverOrder", validate_boot_order },
+	{ EFI_GLOBAL_VARIABLE_GUID, "Boot*", validate_load_option },
+	{ EFI_GLOBAL_VARIABLE_GUID, "Driver*", validate_load_option },
+	{ EFI_GLOBAL_VARIABLE_GUID, "ConIn", validate_device_path },
+	{ EFI_GLOBAL_VARIABLE_GUID, "ConInDev", validate_device_path },
+	{ EFI_GLOBAL_VARIABLE_GUID, "ConOut", validate_device_path },
+	{ EFI_GLOBAL_VARIABLE_GUID, "ConOutDev", validate_device_path },
+	{ EFI_GLOBAL_VARIABLE_GUID, "ErrOut", validate_device_path },
+	{ EFI_GLOBAL_VARIABLE_GUID, "ErrOutDev", validate_device_path },
+	{ EFI_GLOBAL_VARIABLE_GUID, "Timeout", validate_uint16 },
+	{ EFI_GLOBAL_VARIABLE_GUID, "Lang", validate_ascii_string },
+	{ EFI_GLOBAL_VARIABLE_GUID, "PlatformLang", validate_ascii_string },
+	{ NULL_GUID, "", NULL },
 };
 
 bool
@@ -202,7 +203,7 @@ efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long len)
 
 	ucs2_as_utf8(utf8_name, var_name, len);
 
-	for (i = 0; variable_validate[i].validate != NULL; i++) {
+	for (i = 0; variable_validate[i].name[0] != '\0'; i++) {
 		const char *name = variable_validate[i].name;
 		int match;
 
-- 
2.5.0

[PATCH 5/5] efi: Make efivarfs entries immutable by default.

[Peter Jones]

"rm -rf" is bricking some peoples' laptops because of variables being
used to store non-reinitializable firmware driver data that's required
to POST the hardware.

These are 100% bugs, and they need to be fixed, but in the mean time it
shouldn't be easy to *accidentally* brick machines.

We have to have delete working, and picking which variables do and don't
work for deletion is quite intractable, so instead make everything
immutable by default (except for a whitelist), and make tools that
aren't quite so broad-spectrum unset the immutable flag.

v2: adds Timeout to our whitelist.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/firmware/efi/vars.c | 83 +++++++++++++++++++++++++++++++++------------
 fs/efivarfs/file.c          | 69 +++++++++++++++++++++++++++++++++++++
 fs/efivarfs/inode.c         | 32 +++++++++++------
 fs/efivarfs/internal.h      |  3 +-
 fs/efivarfs/super.c         |  9 +++--
 include/linux/efi.h         |  2 ++
 6 files changed, 163 insertions(+), 35 deletions(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index e084d08..2cd32ae 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -186,9 +186,33 @@ static const struct variable_validate variable_validate[] = {
 	{ EFI_GLOBAL_VARIABLE_GUID, "Timeout", validate_uint16 },
 	{ EFI_GLOBAL_VARIABLE_GUID, "Lang", validate_ascii_string },
 	{ EFI_GLOBAL_VARIABLE_GUID, "PlatformLang", validate_ascii_string },
+	{ EFI_GLOBAL_VARIABLE_GUID, "OsIndications", NULL },
+	{ EFI_GLOBAL_VARIABLE_GUID, "Timeout", NULL },
 	{ NULL_GUID, "", NULL },
 };
 
+static bool
+variable_matches(const char *var_name, size_t len, const char *match_name,
+		 int *match)
+{
+	for (*match = 0; match_name[*match] && *match < len; (*match)++) {
+		char c = match_name[*match];
+		char u = var_name[*match];
+
+		/* Wildcard in the matching name means we've matched */
+		if (c == '*')
+			return true;
+
+		/* Case sensitive match */
+		if (c != u)
+			break;
+
+		if (!c)
+			return true;
+	}
+	return false;
+}
+
 bool
 efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long len)
 {
@@ -205,36 +229,53 @@ efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long len)
 
 	for (i = 0; variable_validate[i].name[0] != '\0'; i++) {
 		const char *name = variable_validate[i].name;
-		int match;
+		int match = 0;
 
-		for (match = 0; ; match++) {
-			char c = name[match];
-			char u = utf8_name[match];
+		if (variable_matches(utf8_name, utf8_size+1, name, &match)) {
+			kfree(utf8_name);
+			if (variable_validate[i].validate == NULL)
+				break;
+			return variable_validate[i].validate(var_name, match,
+							     data, len);
+		}
 
-			/* Wildcard in the matching name means we've matched */
-			if (c == '*') {
-				kfree(utf8_name);
-				return variable_validate[i].validate(var_name,
-							     match, data, len);
-			}
+	}
+	kfree(utf8_name);
+	return true;
+}
+EXPORT_SYMBOL_GPL(efivar_validate);
 
-			/* Case sensitive match */
-			if (c != u)
-				break;
+bool
+efivar_variable_is_protected(efi_guid_t vendor, const char *var_name,
+			     size_t len)
+{
+	int i;
+	bool found = false;
+	int match = 0;
 
-			/* Reached the end of the string while matching */
-			if (!c) {
-				kfree(utf8_name);
-				return variable_validate[i].validate(var_name,
-							     match, data, len);
-			}
+	/*
+	 * Now check the validated variables list and then the whitelist -
+	 * both are whitelists
+	 */
+	for (i = 0; variable_validate[i].name[0] != '\0'; i++) {
+		if (efi_guidcmp(variable_validate[i].guid, vendor))
+			continue;
+
+		if (variable_matches(var_name, len,
+				     variable_validate[i].name, &match)) {
+			found = true;
+			break;
 		}
 	}
 
-	kfree(utf8_name);
+	/*
+	 * If we found it in our list, it /isn't/ one of our protected names.
+	 */
+	if (found)
+		return false;
 	return true;
 }
-EXPORT_SYMBOL_GPL(efivar_validate);
+EXPORT_SYMBOL_GPL(efivar_variable_is_protected);
 
 static efi_status_t
 check_var_size(u32 attributes, unsigned long size)
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index c424e48..b793c85 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -10,6 +10,7 @@
 #include <linux/efi.h>
 #include <linux/fs.h>
 #include <linux/slab.h>
+#include <linux/mount.h>
 
 #include "internal.h"
 
@@ -103,9 +104,77 @@ out_free:
 	return size;
 }
 
+static int
+efivarfs_ioc_getxflags(struct file *file,
+		       void __user *arg)
+{
+	struct inode *inode = file->f_mapping->host;
+	unsigned int iflags;
+	unsigned int flags = 0;
+
+	iflags = inode->i_flags;
+	if (iflags & S_IMMUTABLE)
+		flags |= FS_IMMUTABLE_FL;
+
+	if (copy_to_user(arg, &flags, sizeof(flags)))
+		return -EFAULT;
+	return 0;
+}
+
+static int
+efivarfs_ioc_setxflags(struct file *file,
+		       void __user *arg)
+{
+	struct inode *inode = file->f_mapping->host;
+	unsigned int flags;
+	int error;
+
+	if (!inode_owner_or_capable(inode))
+		return -EACCES;
+
+	if (copy_from_user(&flags, arg, sizeof(flags)))
+		return -EFAULT;
+
+	if (flags & ~FS_IMMUTABLE_FL)
+		return -EOPNOTSUPP;
+
+	if (!capable(CAP_LINUX_IMMUTABLE))
+		return -EPERM;
+
+	error = mnt_want_write_file(file);
+	if (error)
+		return error;
+
+	if (flags & FS_IMMUTABLE_FL)
+		inode->i_flags |= S_IMMUTABLE;
+	else
+		inode->i_flags &= ~S_IMMUTABLE;
+
+	return 0;
+}
+
+long
+efivarfs_file_ioctl(
+		    struct file *file,
+		    unsigned int cmd,
+		    unsigned long p)
+{
+	void __user *arg = (void __user *)p;
+
+	switch (cmd) {
+	case FS_IOC_GETFLAGS:
+		return efivarfs_ioc_getxflags(file, arg);
+	case FS_IOC_SETFLAGS:
+		return efivarfs_ioc_setxflags(file, arg);
+	}
+
+	return -ENOTTY;
+}
+
 const struct file_operations efivarfs_file_operations = {
 	.open	= simple_open,
 	.read	= efivarfs_file_read,
 	.write	= efivarfs_file_write,
 	.llseek	= no_llseek,
+	.unlocked_ioctl = efivarfs_file_ioctl,
 };
diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c
index 3381b9d..00735e0 100644
--- a/fs/efivarfs/inode.c
+++ b/fs/efivarfs/inode.c
@@ -15,7 +15,8 @@
 #include "internal.h"
 
 struct inode *efivarfs_get_inode(struct super_block *sb,
-				const struct inode *dir, int mode, dev_t dev)
+				const struct inode *dir, int mode,
+				dev_t dev, bool is_protected)
 {
 	struct inode *inode = new_inode(sb);
 
@@ -23,6 +24,7 @@ struct inode *efivarfs_get_inode(struct super_block *sb,
 		inode->i_ino = get_next_ino();
 		inode->i_mode = mode;
 		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+		inode->i_flags = is_protected ? S_IMMUTABLE : 0;
 		switch (mode & S_IFMT) {
 		case S_IFREG:
 			inode->i_fop = &efivarfs_file_operations;
@@ -102,22 +104,17 @@ static void efivarfs_hex_to_guid(const char *str, efi_guid_t *guid)
 static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 			  umode_t mode, bool excl)
 {
-	struct inode *inode;
+	struct inode *inode = NULL;
 	struct efivar_entry *var;
 	int namelen, i = 0, err = 0;
+	bool is_protected = false;
 
 	if (!efivarfs_valid_name(dentry->d_name.name, dentry->d_name.len))
 		return -EINVAL;
 
-	inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
-	if (!inode)
-		return -ENOMEM;
-
 	var = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
-	if (!var) {
-		err = -ENOMEM;
-		goto out;
-	}
+	if (!var)
+		return -ENOMEM;
 
 	/* length of the variable name itself: remove GUID and separator */
 	namelen = dentry->d_name.len - EFI_VARIABLE_GUID_LEN - 1;
@@ -125,6 +122,18 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 	efivarfs_hex_to_guid(dentry->d_name.name + namelen + 1,
 			&var->var.VendorGuid);
 
+	if (efivar_variable_is_protected(var->var.VendorGuid,
+					 dentry->d_name.name,
+					 dentry->d_name.len))
+		is_protected = true;
+
+	inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0, is_protected);
+	if (!inode) {
+		err = -ENOMEM;
+		goto out;
+	}
+	inode->i_flags = 0;
+
 	for (i = 0; i < namelen; i++)
 		var->var.VariableName[i] = dentry->d_name.name[i];
 
@@ -138,7 +147,8 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 out:
 	if (err) {
 		kfree(var);
-		iput(inode);
+		if (inode)
+			iput(inode);
 	}
 	return err;
 }
diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
index b5ff16a..f435832 100644
--- a/fs/efivarfs/internal.h
+++ b/fs/efivarfs/internal.h
@@ -15,7 +15,8 @@ extern const struct file_operations efivarfs_file_operations;
 extern const struct inode_operations efivarfs_dir_inode_operations;
 extern bool efivarfs_valid_name(const char *str, int len);
 extern struct inode *efivarfs_get_inode(struct super_block *sb,
-			const struct inode *dir, int mode, dev_t dev);
+			const struct inode *dir, int mode, dev_t dev,
+			bool is_protected);
 
 extern struct list_head efivarfs_list;
 
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 8651ac2..6d5dd56 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -120,6 +120,7 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 	char *name;
 	int len;
 	int err = -ENOMEM;
+	bool is_protected = false;
 
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
 	if (!entry)
@@ -137,13 +138,17 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 
 	ucs2_as_utf8(name, entry->var.VariableName, len);
 
+	if (efivar_variable_is_protected(entry->var.VendorGuid, name, len))
+		is_protected = true;
+
 	name[len] = '-';
 
 	efi_guid_to_str(&entry->var.VendorGuid, name + len + 1);
 
 	name[len + EFI_VARIABLE_GUID_LEN+1] = '\0';
 
-	inode = efivarfs_get_inode(sb, d_inode(root), S_IFREG | 0644, 0);
+	inode = efivarfs_get_inode(sb, d_inode(root), S_IFREG | 0644, 0,
+				   is_protected);
 	if (!inode)
 		goto fail_name;
 
@@ -199,7 +204,7 @@ static int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_d_op		= &efivarfs_d_ops;
 	sb->s_time_gran         = 1;
 
-	inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0);
+	inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0, false);
 	if (!inode)
 		return -ENOMEM;
 	inode->i_op = &efivarfs_dir_inode_operations;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 569b5a8..afc841b 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1200,6 +1200,8 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
 				       struct list_head *head, bool remove);
 
 bool efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long len);
+bool efivar_variable_is_protected(efi_guid_t vendor, const char *name,
+				  size_t len);
 
 extern struct work_struct efivar_work;
 void efivar_run_worker(void);
-- 
2.5.0

Re: [PATCH 5/5] efi: Make efivarfs entries immutable by default.

[Matt Fleming]

On Wed, 03 Feb, at 08:02:47AM, Peter Jones wrote:
> "rm -rf" is bricking some peoples' laptops because of variables being
> used to store non-reinitializable firmware driver data that's required
> to POST the hardware.
> 
> These are 100% bugs, and they need to be fixed, but in the mean time it
> shouldn't be easy to *accidentally* brick machines.
> 
> We have to have delete working, and picking which variables do and don't
> work for deletion is quite intractable, so instead make everything
> immutable by default (except for a whitelist), and make tools that
> aren't quite so broad-spectrum unset the immutable flag.
> 
> v2: adds Timeout to our whitelist.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/firmware/efi/vars.c | 83 +++++++++++++++++++++++++++++++++------------
>  fs/efivarfs/file.c          | 69 +++++++++++++++++++++++++++++++++++++
>  fs/efivarfs/inode.c         | 32 +++++++++++------
>  fs/efivarfs/internal.h      |  3 +-
>  fs/efivarfs/super.c         |  9 +++--
>  include/linux/efi.h         |  2 ++
>  6 files changed, 163 insertions(+), 35 deletions(-)

I see no mention of the benefit of using the immutable flag versus
making all protected files read-only.

Is it not possible to just make everything that needs protecting 444?
That way users can use standard tools if they really, really want to
delete/write to a variable. It has the added benefit of protecting
users from trashing variables that are important for POST too (as
opposed to deleting them altogether).

Re: [PATCH 5/5] efi: Make efivarfs entries immutable by default.

[Steve McIntyre]

On Wed, Feb 03, 2016 at 02:13:54PM +0000, Matt Fleming wrote:
>On Wed, 03 Feb, at 08:02:47AM, Peter Jones wrote:
>> "rm -rf" is bricking some peoples' laptops because of variables being
>> used to store non-reinitializable firmware driver data that's required
>> to POST the hardware.
>> 
>> These are 100% bugs, and they need to be fixed, but in the mean time it
>> shouldn't be easy to *accidentally* brick machines.
>> 
>> We have to have delete working, and picking which variables do and don't
>> work for deletion is quite intractable, so instead make everything
>> immutable by default (except for a whitelist), and make tools that
>> aren't quite so broad-spectrum unset the immutable flag.
>> 
>> v2: adds Timeout to our whitelist.
>> 
>> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/firmware/efi/vars.c | 83 +++++++++++++++++++++++++++++++++------------
>>  fs/efivarfs/file.c          | 69 +++++++++++++++++++++++++++++++++++++
>>  fs/efivarfs/inode.c         | 32 +++++++++++------
>>  fs/efivarfs/internal.h      |  3 +-
>>  fs/efivarfs/super.c         |  9 +++--
>>  include/linux/efi.h         |  2 ++
>>  6 files changed, 163 insertions(+), 35 deletions(-)
>
>I see no mention of the benefit of using the immutable flag versus
>making all protected files read-only.
>
>Is it not possible to just make everything that needs protecting 444?
>That way users can use standard tools if they really, really want to
>delete/write to a variable. It has the added benefit of protecting
>users from trashing variables that are important for POST too (as
>opposed to deleting them altogether).

Just making them read-only won't stop people trashing things as
root. They're already owned by root anyway aren't they??

Although if we're at the stage of doing things this wat then is there
much to be gained by having a filesystem interface in the first place?

-- 
Steve McIntyre, Cambridge, UK.                                steve-nt0JYOx6u4DQT0dZR+AlfA@public.gmane.org
< liw> everything I know about UK hotels I learned from "Fawlty Towers"

Re: [PATCH 5/5] efi: Make efivarfs entries immutable by default.

[Leif Lindholm]

On Wed, Feb 03, 2016 at 02:20:04PM +0000, Steve McIntyre wrote:
> On Wed, Feb 03, 2016 at 02:13:54PM +0000, Matt Fleming wrote:
> >On Wed, 03 Feb, at 08:02:47AM, Peter Jones wrote:
> >I see no mention of the benefit of using the immutable flag versus
> >making all protected files read-only.
> >
> >Is it not possible to just make everything that needs protecting 444?
> >That way users can use standard tools if they really, really want to
> >delete/write to a variable. It has the added benefit of protecting
> >users from trashing variables that are important for POST too (as
> >opposed to deleting them altogether).
> 
> Just making them read-only won't stop people trashing things as
> root. They're already owned by root anyway aren't they??

The point is to stop people _accidentally_ triggering
brickness-inducing bugs in completely broken firmware.

This set achieves that.

> Although if we're at the stage of doing things this wat then is there
> much to be gained by having a filesystem interface in the first place?

These systems would be manually brickable regardless of what interface
you implemented, or under which operating system. Probably even from
the UEFI Shell.

/
    Leif

Re: [PATCH 5/5] efi: Make efivarfs entries immutable by default.

[Matt Fleming]

On Wed, 03 Feb, at 02:50:05PM, Leif Lindholm wrote:
> On Wed, Feb 03, 2016 at 02:20:04PM +0000, Steve McIntyre wrote:
> > Although if we're at the stage of doing things this wat then is there
> > much to be gained by having a filesystem interface in the first place?
> 
> These systems would be manually brickable regardless of what interface
> you implemented, or under which operating system. Probably even from
> the UEFI Shell.

Right, and the old efivar sysfs code has a 1024-byte limit on the size
of data, which is impractical.

Re: [PATCH 5/5] efi: Make efivarfs entries immutable by default.

[Steve McIntyre]

On Wed, Feb 03, 2016 at 02:56:21PM +0000, Matt Fleming wrote:
>On Wed, 03 Feb, at 02:50:05PM, Leif Lindholm wrote:
>> On Wed, Feb 03, 2016 at 02:20:04PM +0000, Steve McIntyre wrote:
>> > Although if we're at the stage of doing things this wat then is there
>> > much to be gained by having a filesystem interface in the first place?
>> 
>> These systems would be manually brickable regardless of what interface
>> you implemented, or under which operating system. Probably even from
>> the UEFI Shell.
>
>Right, and the old efivar sysfs code has a 1024-byte limit on the size
>of data, which is impractical.

*nod* - as discussed on irc. I hadn't realised that limitation.

-- 
Steve McIntyre, Cambridge, UK.                                steve-nt0JYOx6u4DQT0dZR+AlfA@public.gmane.org
"Since phone messaging became popular, the young generation has lost the
 ability to read or write anything that is longer than one hundred and sixty
 characters."  -- Ignatios Souvatzis

[GIT PULL 0/5] EFI urgent fixes

[Matt Fleming]

Folks,

Please pull the following EFI patches from Peter that prevent
accidental deletion of EFI variables through efivarfs which can lead
to bricked machines.

These obviously need backporting to stable, so I'll take care of
sending the backports separately because we don't need to send the
entire 5 patch series.

The following changes since commit 59fd1214561921343305a0e9dc218bf3d40068f3:

  x86/mm/numa: Fix 32-bit memblock range truncation bug on 32-bit NUMA kernels (2016-02-08 12:10:03 +0100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git tags/efi-urgent

for you to fetch changes up to ed8b0de5a33d2a2557dce7f9429dca8cb5bc5879:

  efi: Make efivarfs entries immutable by default (2016-02-10 16:25:52 +0000)

----------------------------------------------------------------
 * Prevent accidental deletion of EFI variables through efivarfs that
   may brick machines. We use a whitelist of known-safe variables to
   allow things like installing distributions to work out of the box, and
   instead restrict vendor-specific variable deletion by making
   non-whitelist variables immutable - Peter Jones

----------------------------------------------------------------
Peter Jones (5):
      lib/ucs2_string: Add ucs2 -> utf8 helper functions
      efi: Use ucs2_as_utf8 in efivarfs instead of open coding a bad version
      efi: Do variable name validation tests in utf8
      efi: Make our variable validation list include the guid
      efi: Make efivarfs entries immutable by default

 Documentation/filesystems/efivarfs.txt         |   7 ++
 drivers/firmware/efi/efivars.c                 |  35 +++---
 drivers/firmware/efi/vars.c                    | 143 ++++++++++++++++++-------
 fs/efivarfs/file.c                             |  70 ++++++++++++
 fs/efivarfs/inode.c                            |  30 ++++--
 fs/efivarfs/internal.h                         |   3 +-
 fs/efivarfs/super.c                            |  16 +--
 include/linux/efi.h                            |   5 +-
 include/linux/ucs2_string.h                    |   4 +
 lib/ucs2_string.c                              |  62 +++++++++++
 tools/testing/selftests/efivarfs/efivarfs.sh   |  19 +++-
 tools/testing/selftests/efivarfs/open-unlink.c |  72 ++++++++++++-
 12 files changed, 383 insertions(+), 83 deletions(-)

[PATCH 5/5] efi: Make efivarfs entries immutable by default

[Matt Fleming]

From: Peter Jones <pjones@redhat.com>

"rm -rf" is bricking some peoples' laptops because of variables being
used to store non-reinitializable firmware driver data that's required
to POST the hardware.

These are 100% bugs, and they need to be fixed, but in the mean time it
shouldn't be easy to *accidentally* brick machines.

We have to have delete working, and picking which variables do and don't
work for deletion is quite intractable, so instead make everything
immutable by default (except for a whitelist), and make tools that
aren't quite so broad-spectrum unset the immutable flag.

Signed-off-by: Peter Jones <pjones@redhat.com>
Tested-by: "Lee, Chun-Yi" <jlee@suse.com>
Acked-by: Matthew Garrett <mjg59@coreos.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 Documentation/filesystems/efivarfs.txt         |  7 +++
 drivers/firmware/efi/vars.c                    | 87 +++++++++++++++++++-------
 fs/efivarfs/file.c                             | 70 +++++++++++++++++++++
 fs/efivarfs/inode.c                            | 30 +++++----
 fs/efivarfs/internal.h                         |  3 +-
 fs/efivarfs/super.c                            |  9 ++-
 include/linux/efi.h                            |  2 +
 tools/testing/selftests/efivarfs/efivarfs.sh   | 19 +++++-
 tools/testing/selftests/efivarfs/open-unlink.c | 72 ++++++++++++++++++++-
 9 files changed, 258 insertions(+), 41 deletions(-)

diff --git a/Documentation/filesystems/efivarfs.txt b/Documentation/filesystems/efivarfs.txt
index c477af086e65..686a64bba775 100644
--- a/Documentation/filesystems/efivarfs.txt
+++ b/Documentation/filesystems/efivarfs.txt
@@ -14,3 +14,10 @@ filesystem.
 efivarfs is typically mounted like this,
 
 	mount -t efivarfs none /sys/firmware/efi/efivars
+
+Due to the presence of numerous firmware bugs where removing non-standard
+UEFI variables causes the system firmware to fail to POST, efivarfs
+files that are not well-known standardized variables are created
+as immutable files.  This doesn't prevent removal - "chattr -i" will work -
+but it does prevent this kind of failure from being accomplished
+accidentally.
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 9a53da21e7b6..50f10bad2604 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -172,10 +172,12 @@ struct variable_validate {
 };
 
 /*
- * This is the list of variables we need to validate.
+ * This is the list of variables we need to validate, as well as the
+ * whitelist for what we think is safe not to default to immutable.
  *
  * If it has a validate() method that's not NULL, it'll go into the
- * validation routine.  If not, it is assumed valid.
+ * validation routine.  If not, it is assumed valid, but still used for
+ * whitelisting.
  *
  * Note that it's sorted by {vendor,name}, but globbed names must come after
  * any other name with the same prefix.
@@ -193,11 +195,37 @@ static const struct variable_validate variable_validate[] = {
 	{ EFI_GLOBAL_VARIABLE_GUID, "ErrOut", validate_device_path },
 	{ EFI_GLOBAL_VARIABLE_GUID, "ErrOutDev", validate_device_path },
 	{ EFI_GLOBAL_VARIABLE_GUID, "Lang", validate_ascii_string },
+	{ EFI_GLOBAL_VARIABLE_GUID, "OsIndications", NULL },
 	{ EFI_GLOBAL_VARIABLE_GUID, "PlatformLang", validate_ascii_string },
 	{ EFI_GLOBAL_VARIABLE_GUID, "Timeout", validate_uint16 },
 	{ NULL_GUID, "", NULL },
 };
 
+static bool
+variable_matches(const char *var_name, size_t len, const char *match_name,
+		 int *match)
+{
+	for (*match = 0; ; (*match)++) {
+		char c = match_name[*match];
+		char u = var_name[*match];
+
+		/* Wildcard in the matching name means we've matched */
+		if (c == '*')
+			return true;
+
+		/* Case sensitive match */
+		if (!c && *match == len)
+			return true;
+
+		if (c != u)
+			return false;
+
+		if (!c)
+			return true;
+	}
+	return true;
+}
+
 bool
 efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
 		unsigned long data_size)
@@ -221,35 +249,48 @@ efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
 		if (efi_guidcmp(vendor, variable_validate[i].vendor))
 			continue;
 
-		for (match = 0; ; match++) {
-			char c = name[match];
-			char u = utf8_name[match];
-
-			/* Wildcard in the matching name means we've matched */
-			if (c == '*') {
-				kfree(utf8_name);
-				return variable_validate[i].validate(var_name,
-							match, data, data_size);
-			}
-
-			/* Case sensitive match */
-			if (c != u)
+		if (variable_matches(utf8_name, utf8_size+1, name, &match)) {
+			if (variable_validate[i].validate == NULL)
 				break;
-
-			/* Reached the end of the string while matching */
-			if (!c) {
-				kfree(utf8_name);
-				return variable_validate[i].validate(var_name,
-							match, data, data_size);
-			}
+			kfree(utf8_name);
+			return variable_validate[i].validate(var_name, match,
+							     data, data_size);
 		}
 	}
-
 	kfree(utf8_name);
 	return true;
 }
 EXPORT_SYMBOL_GPL(efivar_validate);
 
+bool
+efivar_variable_is_removable(efi_guid_t vendor, const char *var_name,
+			     size_t len)
+{
+	int i;
+	bool found = false;
+	int match = 0;
+
+	/*
+	 * Check if our variable is in the validated variables list
+	 */
+	for (i = 0; variable_validate[i].name[0] != '\0'; i++) {
+		if (efi_guidcmp(variable_validate[i].vendor, vendor))
+			continue;
+
+		if (variable_matches(var_name, len,
+				     variable_validate[i].name, &match)) {
+			found = true;
+			break;
+		}
+	}
+
+	/*
+	 * If it's in our list, it is removable.
+	 */
+	return found;
+}
+EXPORT_SYMBOL_GPL(efivar_variable_is_removable);
+
 static efi_status_t
 check_var_size(u32 attributes, unsigned long size)
 {
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index c424e4813ec8..d48e0d261d78 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -10,6 +10,7 @@
 #include <linux/efi.h>
 #include <linux/fs.h>
 #include <linux/slab.h>
+#include <linux/mount.h>
 
 #include "internal.h"
 
@@ -103,9 +104,78 @@ out_free:
 	return size;
 }
 
+static int
+efivarfs_ioc_getxflags(struct file *file, void __user *arg)
+{
+	struct inode *inode = file->f_mapping->host;
+	unsigned int i_flags;
+	unsigned int flags = 0;
+
+	i_flags = inode->i_flags;
+	if (i_flags & S_IMMUTABLE)
+		flags |= FS_IMMUTABLE_FL;
+
+	if (copy_to_user(arg, &flags, sizeof(flags)))
+		return -EFAULT;
+	return 0;
+}
+
+static int
+efivarfs_ioc_setxflags(struct file *file, void __user *arg)
+{
+	struct inode *inode = file->f_mapping->host;
+	unsigned int flags;
+	unsigned int i_flags = 0;
+	int error;
+
+	if (!inode_owner_or_capable(inode))
+		return -EACCES;
+
+	if (copy_from_user(&flags, arg, sizeof(flags)))
+		return -EFAULT;
+
+	if (flags & ~FS_IMMUTABLE_FL)
+		return -EOPNOTSUPP;
+
+	if (!capable(CAP_LINUX_IMMUTABLE))
+		return -EPERM;
+
+	if (flags & FS_IMMUTABLE_FL)
+		i_flags |= S_IMMUTABLE;
+
+
+	error = mnt_want_write_file(file);
+	if (error)
+		return error;
+
+	inode_lock(inode);
+	inode_set_flags(inode, i_flags, S_IMMUTABLE);
+	inode_unlock(inode);
+
+	mnt_drop_write_file(file);
+
+	return 0;
+}
+
+long
+efivarfs_file_ioctl(struct file *file, unsigned int cmd, unsigned long p)
+{
+	void __user *arg = (void __user *)p;
+
+	switch (cmd) {
+	case FS_IOC_GETFLAGS:
+		return efivarfs_ioc_getxflags(file, arg);
+	case FS_IOC_SETFLAGS:
+		return efivarfs_ioc_setxflags(file, arg);
+	}
+
+	return -ENOTTY;
+}
+
 const struct file_operations efivarfs_file_operations = {
 	.open	= simple_open,
 	.read	= efivarfs_file_read,
 	.write	= efivarfs_file_write,
 	.llseek	= no_llseek,
+	.unlocked_ioctl = efivarfs_file_ioctl,
 };
diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c
index 3381b9da9ee6..e2ab6d0497f2 100644
--- a/fs/efivarfs/inode.c
+++ b/fs/efivarfs/inode.c
@@ -15,7 +15,8 @@
 #include "internal.h"
 
 struct inode *efivarfs_get_inode(struct super_block *sb,
-				const struct inode *dir, int mode, dev_t dev)
+				const struct inode *dir, int mode,
+				dev_t dev, bool is_removable)
 {
 	struct inode *inode = new_inode(sb);
 
@@ -23,6 +24,7 @@ struct inode *efivarfs_get_inode(struct super_block *sb,
 		inode->i_ino = get_next_ino();
 		inode->i_mode = mode;
 		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+		inode->i_flags = is_removable ? 0 : S_IMMUTABLE;
 		switch (mode & S_IFMT) {
 		case S_IFREG:
 			inode->i_fop = &efivarfs_file_operations;
@@ -102,22 +104,17 @@ static void efivarfs_hex_to_guid(const char *str, efi_guid_t *guid)
 static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 			  umode_t mode, bool excl)
 {
-	struct inode *inode;
+	struct inode *inode = NULL;
 	struct efivar_entry *var;
 	int namelen, i = 0, err = 0;
+	bool is_removable = false;
 
 	if (!efivarfs_valid_name(dentry->d_name.name, dentry->d_name.len))
 		return -EINVAL;
 
-	inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
-	if (!inode)
-		return -ENOMEM;
-
 	var = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
-	if (!var) {
-		err = -ENOMEM;
-		goto out;
-	}
+	if (!var)
+		return -ENOMEM;
 
 	/* length of the variable name itself: remove GUID and separator */
 	namelen = dentry->d_name.len - EFI_VARIABLE_GUID_LEN - 1;
@@ -125,6 +122,16 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 	efivarfs_hex_to_guid(dentry->d_name.name + namelen + 1,
 			&var->var.VendorGuid);
 
+	if (efivar_variable_is_removable(var->var.VendorGuid,
+					 dentry->d_name.name, namelen))
+		is_removable = true;
+
+	inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0, is_removable);
+	if (!inode) {
+		err = -ENOMEM;
+		goto out;
+	}
+
 	for (i = 0; i < namelen; i++)
 		var->var.VariableName[i] = dentry->d_name.name[i];
 
@@ -138,7 +145,8 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 out:
 	if (err) {
 		kfree(var);
-		iput(inode);
+		if (inode)
+			iput(inode);
 	}
 	return err;
 }
diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
index b5ff16addb7c..b4505188e799 100644
--- a/fs/efivarfs/internal.h
+++ b/fs/efivarfs/internal.h
@@ -15,7 +15,8 @@ extern const struct file_operations efivarfs_file_operations;
 extern const struct inode_operations efivarfs_dir_inode_operations;
 extern bool efivarfs_valid_name(const char *str, int len);
 extern struct inode *efivarfs_get_inode(struct super_block *sb,
-			const struct inode *dir, int mode, dev_t dev);
+			const struct inode *dir, int mode, dev_t dev,
+			bool is_removable);
 
 extern struct list_head efivarfs_list;
 
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 8651ac28ec0d..dd029d13ea61 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -120,6 +120,7 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 	char *name;
 	int len;
 	int err = -ENOMEM;
+	bool is_removable = false;
 
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
 	if (!entry)
@@ -137,13 +138,17 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 
 	ucs2_as_utf8(name, entry->var.VariableName, len);
 
+	if (efivar_variable_is_removable(entry->var.VendorGuid, name, len))
+		is_removable = true;
+
 	name[len] = '-';
 
 	efi_guid_to_str(&entry->var.VendorGuid, name + len + 1);
 
 	name[len + EFI_VARIABLE_GUID_LEN+1] = '\0';
 
-	inode = efivarfs_get_inode(sb, d_inode(root), S_IFREG | 0644, 0);
+	inode = efivarfs_get_inode(sb, d_inode(root), S_IFREG | 0644, 0,
+				   is_removable);
 	if (!inode)
 		goto fail_name;
 
@@ -199,7 +204,7 @@ static int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_d_op		= &efivarfs_d_ops;
 	sb->s_time_gran         = 1;
 
-	inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0);
+	inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0, true);
 	if (!inode)
 		return -ENOMEM;
 	inode->i_op = &efivarfs_dir_inode_operations;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 16ca611aabc8..47be3ad7d3e5 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1201,6 +1201,8 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
 
 bool efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
 		     unsigned long data_size);
+bool efivar_variable_is_removable(efi_guid_t vendor, const char *name,
+				  size_t len);
 
 extern struct work_struct efivar_work;
 void efivar_run_worker(void);
diff --git a/tools/testing/selftests/efivarfs/efivarfs.sh b/tools/testing/selftests/efivarfs/efivarfs.sh
index 77edcdcc016b..057278448515 100755
--- a/tools/testing/selftests/efivarfs/efivarfs.sh
+++ b/tools/testing/selftests/efivarfs/efivarfs.sh
@@ -88,7 +88,11 @@ test_delete()
 		exit 1
 	fi
 
-	rm $file
+	rm $file 2>/dev/null
+	if [ $? -ne 0 ]; then
+		chattr -i $file
+		rm $file
+	fi
 
 	if [ -e $file ]; then
 		echo "$file couldn't be deleted" >&2
@@ -111,6 +115,7 @@ test_zero_size_delete()
 		exit 1
 	fi
 
+	chattr -i $file
 	printf "$attrs" > $file
 
 	if [ -e $file ]; then
@@ -141,7 +146,11 @@ test_valid_filenames()
 			echo "$file could not be created" >&2
 			ret=1
 		else
-			rm $file
+			rm $file 2>/dev/null
+			if [ $? -ne 0 ]; then
+				chattr -i $file
+				rm $file
+			fi
 		fi
 	done
 
@@ -174,7 +183,11 @@ test_invalid_filenames()
 
 		if [ -e $file ]; then
 			echo "Creating $file should have failed" >&2
-			rm $file
+			rm $file 2>/dev/null
+			if [ $? -ne 0 ]; then
+				chattr -i $file
+				rm $file
+			fi
 			ret=1
 		fi
 	done
diff --git a/tools/testing/selftests/efivarfs/open-unlink.c b/tools/testing/selftests/efivarfs/open-unlink.c
index 8c0764407b3c..4af74f733036 100644
--- a/tools/testing/selftests/efivarfs/open-unlink.c
+++ b/tools/testing/selftests/efivarfs/open-unlink.c
@@ -1,10 +1,68 @@
+#include <errno.h>
 #include <stdio.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
+#include <sys/ioctl.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
+#include <linux/fs.h>
+
+static int set_immutable(const char *path, int immutable)
+{
+	unsigned int flags;
+	int fd;
+	int rc;
+	int error;
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		return fd;
+
+	rc = ioctl(fd, FS_IOC_GETFLAGS, &flags);
+	if (rc < 0) {
+		error = errno;
+		close(fd);
+		errno = error;
+		return rc;
+	}
+
+	if (immutable)
+		flags |= FS_IMMUTABLE_FL;
+	else
+		flags &= ~FS_IMMUTABLE_FL;
+
+	rc = ioctl(fd, FS_IOC_SETFLAGS, &flags);
+	error = errno;
+	close(fd);
+	errno = error;
+	return rc;
+}
+
+static int get_immutable(const char *path)
+{
+	unsigned int flags;
+	int fd;
+	int rc;
+	int error;
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		return fd;
+
+	rc = ioctl(fd, FS_IOC_GETFLAGS, &flags);
+	if (rc < 0) {
+		error = errno;
+		close(fd);
+		errno = error;
+		return rc;
+	}
+	close(fd);
+	if (flags & FS_IMMUTABLE_FL)
+		return 1;
+	return 0;
+}
 
 int main(int argc, char **argv)
 {
@@ -27,7 +85,7 @@ int main(int argc, char **argv)
 	buf[4] = 0;
 
 	/* create a test variable */
-	fd = open(path, O_WRONLY | O_CREAT);
+	fd = open(path, O_WRONLY | O_CREAT, 0600);
 	if (fd < 0) {
 		perror("open(O_WRONLY)");
 		return EXIT_FAILURE;
@@ -41,6 +99,18 @@ int main(int argc, char **argv)
 
 	close(fd);
 
+	rc = get_immutable(path);
+	if (rc < 0) {
+		perror("ioctl(FS_IOC_GETFLAGS)");
+		return EXIT_FAILURE;
+	} else if (rc) {
+		rc = set_immutable(path, 0);
+		if (rc < 0) {
+			perror("ioctl(FS_IOC_SETFLAGS)");
+			return EXIT_FAILURE;
+		}
+	}
+
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		perror("open");
-- 
2.6.2

Re: [PATCH 5/5] efi: Make efivarfs entries immutable by default

[Matt Fleming]

On Fri, 12 Feb, at 11:27:12AM, Matt Fleming wrote:
> From: Peter Jones <pjones@redhat.com>
> 
> "rm -rf" is bricking some peoples' laptops because of variables being
> used to store non-reinitializable firmware driver data that's required
> to POST the hardware.
> 
> These are 100% bugs, and they need to be fixed, but in the mean time it
> shouldn't be easy to *accidentally* brick machines.
> 
> We have to have delete working, and picking which variables do and don't
> work for deletion is quite intractable, so instead make everything
> immutable by default (except for a whitelist), and make tools that
> aren't quite so broad-spectrum unset the immutable flag.
> 
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Tested-by: "Lee, Chun-Yi" <jlee@suse.com>
> Acked-by: Matthew Garrett <mjg59@coreos.com>
> Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> ---
>  Documentation/filesystems/efivarfs.txt         |  7 +++
>  drivers/firmware/efi/vars.c                    | 87 +++++++++++++++++++-------
>  fs/efivarfs/file.c                             | 70 +++++++++++++++++++++
>  fs/efivarfs/inode.c                            | 30 +++++----
>  fs/efivarfs/internal.h                         |  3 +-
>  fs/efivarfs/super.c                            |  9 ++-
>  include/linux/efi.h                            |  2 +
>  tools/testing/selftests/efivarfs/efivarfs.sh   | 19 +++++-
>  tools/testing/selftests/efivarfs/open-unlink.c | 72 ++++++++++++++++++++-
>  9 files changed, 258 insertions(+), 41 deletions(-)

Folks, please hold off on merging this patch as Laszlo has raised a
good point about including the efi-pstore variables in the whitelist.

Re: [PATCH 5/5] efi: Make efivarfs entries immutable by default

[Matt Fleming]

On Fri, 12 Feb, at 11:27:12AM, Matt Fleming wrote:
> From: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> "rm -rf" is bricking some peoples' laptops because of variables being
> used to store non-reinitializable firmware driver data that's required
> to POST the hardware.
> 
> These are 100% bugs, and they need to be fixed, but in the mean time it
> shouldn't be easy to *accidentally* brick machines.
> 
> We have to have delete working, and picking which variables do and don't
> work for deletion is quite intractable, so instead make everything
> immutable by default (except for a whitelist), and make tools that
> aren't quite so broad-spectrum unset the immutable flag.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Tested-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>
> Acked-by: Matthew Garrett <mjg59-JW9irJGTvgXQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> ---
>  Documentation/filesystems/efivarfs.txt         |  7 +++
>  drivers/firmware/efi/vars.c                    | 87 +++++++++++++++++++-------
>  fs/efivarfs/file.c                             | 70 +++++++++++++++++++++
>  fs/efivarfs/inode.c                            | 30 +++++----
>  fs/efivarfs/internal.h                         |  3 +-
>  fs/efivarfs/super.c                            |  9 ++-
>  include/linux/efi.h                            |  2 +
>  tools/testing/selftests/efivarfs/efivarfs.sh   | 19 +++++-
>  tools/testing/selftests/efivarfs/open-unlink.c | 72 ++++++++++++++++++++-
>  9 files changed, 258 insertions(+), 41 deletions(-)

Folks, please hold off on merging this patch as Laszlo has raised a
good point about including the efi-pstore variables in the whitelist.

Preventing "rm -rf /sys/firmware/efi/efivars/" from damage

[Peter Jones]

Hi everybody,
Here's a patchset to make all the variables in efivarfs that aren't well
known to be reasonably safe to delete be immutable by default.

This should alleviate the danger of somebody accidentally using "rm" to
remove some proprietary file that turns out to be important to the
platform, which for some reason it also can't regenerate during POST.

In all cases this is just preventing the user from accidentally
triggering a major security problem with their underlying firmware, but
stopping accidents isn't a bad thing.  These firmwares still need CVEs
and updates to fix them.  Maybe using ESRT and fwupd :)

Thanks.

[PATCH 5/5] efi: Make efivarfs entries immutable by default.

[Peter Jones]

"rm -rf" is bricking some peoples' laptops because of variables being
used to store non-reinitializable firmware driver data that's required
to POST the hardware.

These are 100% bugs, and they need to be fixed, but in the mean time it
shouldn't be easy to *accidentally* brick machines.

We have to have delete working, and picking which variables do and don't
work for deletion is quite intractable, so instead make everything
immutable by default, and make tools that aren't quite so broad-spectrum
unset the immutable flag.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/firmware/efi/vars.c | 82 +++++++++++++++++++++++++++++++++------------
 fs/efivarfs/file.c          | 69 ++++++++++++++++++++++++++++++++++++++
 fs/efivarfs/inode.c         | 32 ++++++++++++------
 fs/efivarfs/internal.h      |  3 +-
 fs/efivarfs/super.c         |  9 +++--
 include/linux/efi.h         |  2 ++
 6 files changed, 162 insertions(+), 35 deletions(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index e084d08..3a16a57 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -186,9 +186,32 @@ static const struct variable_validate variable_validate[] = {
 	{ EFI_GLOBAL_VARIABLE_GUID, "Timeout", validate_uint16 },
 	{ EFI_GLOBAL_VARIABLE_GUID, "Lang", validate_ascii_string },
 	{ EFI_GLOBAL_VARIABLE_GUID, "PlatformLang", validate_ascii_string },
+	{ EFI_GLOBAL_VARIABLE_GUID, "OsIndications", NULL },
 	{ NULL_GUID, "", NULL },
 };
 
+static bool
+variable_matches(const char *var_name, size_t len, const char *match_name,
+		 int *match)
+{
+	for (*match = 0; match_name[*match] && *match < len; (*match)++) {
+		char c = match_name[*match];
+		char u = var_name[*match];
+
+		/* Wildcard in the matching name means we've matched */
+		if (c == '*')
+			return true;
+
+		/* Case sensitive match */
+		if (c != u)
+			break;
+
+		if (!c)
+			return true;
+	}
+	return false;
+}
+
 bool
 efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long len)
 {
@@ -205,36 +228,53 @@ efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long len)
 
 	for (i = 0; variable_validate[i].name[0] != '\0'; i++) {
 		const char *name = variable_validate[i].name;
-		int match;
+		int match = 0;
 
-		for (match = 0; ; match++) {
-			char c = name[match];
-			char u = utf8_name[match];
+		if (variable_matches(utf8_name, utf8_size+1, name, &match)) {
+			kfree(utf8_name);
+			if (variable_validate[i].validate == NULL)
+				break;
+			return variable_validate[i].validate(var_name, match,
+							     data, len);
+		}
 
-			/* Wildcard in the matching name means we've matched */
-			if (c == '*') {
-				kfree(utf8_name);
-				return variable_validate[i].validate(var_name,
-							     match, data, len);
-			}
+	}
+	kfree(utf8_name);
+	return true;
+}
+EXPORT_SYMBOL_GPL(efivar_validate);
 
-			/* Case sensitive match */
-			if (c != u)
-				break;
+bool
+efivar_variable_is_protected(efi_guid_t vendor, const char *var_name,
+			     size_t len)
+{
+	int i;
+	bool found = false;
+	int match = 0;
 
-			/* Reached the end of the string while matching */
-			if (!c) {
-				kfree(utf8_name);
-				return variable_validate[i].validate(var_name,
-							     match, data, len);
-			}
+	/*
+	 * Now check the validated variables list and then the whitelist -
+	 * both are whitelists
+	 */
+	for (i = 0; variable_validate[i].name[0] != '\0'; i++) {
+		if (efi_guidcmp(variable_validate[i].guid, vendor))
+			continue;
+
+		if (variable_matches(var_name, len,
+				     variable_validate[i].name, &match)) {
+			found = true;
+			break;
 		}
 	}
 
-	kfree(utf8_name);
+	/*
+	 * If we found it in our list, it /isn't/ one of our protected names.
+	 */
+	if (found)
+		return false;
 	return true;
 }
-EXPORT_SYMBOL_GPL(efivar_validate);
+EXPORT_SYMBOL_GPL(efivar_variable_is_protected);
 
 static efi_status_t
 check_var_size(u32 attributes, unsigned long size)
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index c424e48..b793c85 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -10,6 +10,7 @@
 #include <linux/efi.h>
 #include <linux/fs.h>
 #include <linux/slab.h>
+#include <linux/mount.h>
 
 #include "internal.h"
 
@@ -103,9 +104,77 @@ out_free:
 	return size;
 }
 
+static int
+efivarfs_ioc_getxflags(struct file *file,
+		       void __user *arg)
+{
+	struct inode *inode = file->f_mapping->host;
+	unsigned int iflags;
+	unsigned int flags = 0;
+
+	iflags = inode->i_flags;
+	if (iflags & S_IMMUTABLE)
+		flags |= FS_IMMUTABLE_FL;
+
+	if (copy_to_user(arg, &flags, sizeof(flags)))
+		return -EFAULT;
+	return 0;
+}
+
+static int
+efivarfs_ioc_setxflags(struct file *file,
+		       void __user *arg)
+{
+	struct inode *inode = file->f_mapping->host;
+	unsigned int flags;
+	int error;
+
+	if (!inode_owner_or_capable(inode))
+		return -EACCES;
+
+	if (copy_from_user(&flags, arg, sizeof(flags)))
+		return -EFAULT;
+
+	if (flags & ~FS_IMMUTABLE_FL)
+		return -EOPNOTSUPP;
+
+	if (!capable(CAP_LINUX_IMMUTABLE))
+		return -EPERM;
+
+	error = mnt_want_write_file(file);
+	if (error)
+		return error;
+
+	if (flags & FS_IMMUTABLE_FL)
+		inode->i_flags |= S_IMMUTABLE;
+	else
+		inode->i_flags &= ~S_IMMUTABLE;
+
+	return 0;
+}
+
+long
+efivarfs_file_ioctl(
+		    struct file *file,
+		    unsigned int cmd,
+		    unsigned long p)
+{
+	void __user *arg = (void __user *)p;
+
+	switch (cmd) {
+	case FS_IOC_GETFLAGS:
+		return efivarfs_ioc_getxflags(file, arg);
+	case FS_IOC_SETFLAGS:
+		return efivarfs_ioc_setxflags(file, arg);
+	}
+
+	return -ENOTTY;
+}
+
 const struct file_operations efivarfs_file_operations = {
 	.open	= simple_open,
 	.read	= efivarfs_file_read,
 	.write	= efivarfs_file_write,
 	.llseek	= no_llseek,
+	.unlocked_ioctl = efivarfs_file_ioctl,
 };
diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c
index 3381b9d..00735e0 100644
--- a/fs/efivarfs/inode.c
+++ b/fs/efivarfs/inode.c
@@ -15,7 +15,8 @@
 #include "internal.h"
 
 struct inode *efivarfs_get_inode(struct super_block *sb,
-				const struct inode *dir, int mode, dev_t dev)
+				const struct inode *dir, int mode,
+				dev_t dev, bool is_protected)
 {
 	struct inode *inode = new_inode(sb);
 
@@ -23,6 +24,7 @@ struct inode *efivarfs_get_inode(struct super_block *sb,
 		inode->i_ino = get_next_ino();
 		inode->i_mode = mode;
 		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+		inode->i_flags = is_protected ? S_IMMUTABLE : 0;
 		switch (mode & S_IFMT) {
 		case S_IFREG:
 			inode->i_fop = &efivarfs_file_operations;
@@ -102,22 +104,17 @@ static void efivarfs_hex_to_guid(const char *str, efi_guid_t *guid)
 static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 			  umode_t mode, bool excl)
 {
-	struct inode *inode;
+	struct inode *inode = NULL;
 	struct efivar_entry *var;
 	int namelen, i = 0, err = 0;
+	bool is_protected = false;
 
 	if (!efivarfs_valid_name(dentry->d_name.name, dentry->d_name.len))
 		return -EINVAL;
 
-	inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
-	if (!inode)
-		return -ENOMEM;
-
 	var = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
-	if (!var) {
-		err = -ENOMEM;
-		goto out;
-	}
+	if (!var)
+		return -ENOMEM;
 
 	/* length of the variable name itself: remove GUID and separator */
 	namelen = dentry->d_name.len - EFI_VARIABLE_GUID_LEN - 1;
@@ -125,6 +122,18 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 	efivarfs_hex_to_guid(dentry->d_name.name + namelen + 1,
 			&var->var.VendorGuid);
 
+	if (efivar_variable_is_protected(var->var.VendorGuid,
+					 dentry->d_name.name,
+					 dentry->d_name.len))
+		is_protected = true;
+
+	inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0, is_protected);
+	if (!inode) {
+		err = -ENOMEM;
+		goto out;
+	}
+	inode->i_flags = 0;
+
 	for (i = 0; i < namelen; i++)
 		var->var.VariableName[i] = dentry->d_name.name[i];
 
@@ -138,7 +147,8 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 out:
 	if (err) {
 		kfree(var);
-		iput(inode);
+		if (inode)
+			iput(inode);
 	}
 	return err;
 }
diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
index b5ff16a..f435832 100644
--- a/fs/efivarfs/internal.h
+++ b/fs/efivarfs/internal.h
@@ -15,7 +15,8 @@ extern const struct file_operations efivarfs_file_operations;
 extern const struct inode_operations efivarfs_dir_inode_operations;
 extern bool efivarfs_valid_name(const char *str, int len);
 extern struct inode *efivarfs_get_inode(struct super_block *sb,
-			const struct inode *dir, int mode, dev_t dev);
+			const struct inode *dir, int mode, dev_t dev,
+			bool is_protected);
 
 extern struct list_head efivarfs_list;
 
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 8651ac2..6d5dd56 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -120,6 +120,7 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 	char *name;
 	int len;
 	int err = -ENOMEM;
+	bool is_protected = false;
 
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
 	if (!entry)
@@ -137,13 +138,17 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 
 	ucs2_as_utf8(name, entry->var.VariableName, len);
 
+	if (efivar_variable_is_protected(entry->var.VendorGuid, name, len))
+		is_protected = true;
+
 	name[len] = '-';
 
 	efi_guid_to_str(&entry->var.VendorGuid, name + len + 1);
 
 	name[len + EFI_VARIABLE_GUID_LEN+1] = '\0';
 
-	inode = efivarfs_get_inode(sb, d_inode(root), S_IFREG | 0644, 0);
+	inode = efivarfs_get_inode(sb, d_inode(root), S_IFREG | 0644, 0,
+				   is_protected);
 	if (!inode)
 		goto fail_name;
 
@@ -199,7 +204,7 @@ static int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_d_op		= &efivarfs_d_ops;
 	sb->s_time_gran         = 1;
 
-	inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0);
+	inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0, false);
 	if (!inode)
 		return -ENOMEM;
 	inode->i_op = &efivarfs_dir_inode_operations;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 569b5a8..afc841b 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1200,6 +1200,8 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
 				       struct list_head *head, bool remove);
 
 bool efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long len);
+bool efivar_variable_is_protected(efi_guid_t vendor, const char *name,
+				  size_t len);
 
 extern struct work_struct efivar_work;
 void efivar_run_worker(void);
-- 
2.5.0

Re: [PATCH 5/5] efi: Make efivarfs entries immutable by default.

[Matt Fleming]

On Tue, 02 Feb, at 05:33:06PM, Peter Jones wrote:
> "rm -rf" is bricking some peoples' laptops because of variables being
> used to store non-reinitializable firmware driver data that's required
> to POST the hardware.
> 
> These are 100% bugs, and they need to be fixed, but in the mean time it
> shouldn't be easy to *accidentally* brick machines.
> 
> We have to have delete working, and picking which variables do and don't
> work for deletion is quite intractable, so instead make everything
> immutable by default, and make tools that aren't quite so broad-spectrum
> unset the immutable flag.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/firmware/efi/vars.c | 82 +++++++++++++++++++++++++++++++++------------
>  fs/efivarfs/file.c          | 69 ++++++++++++++++++++++++++++++++++++++
>  fs/efivarfs/inode.c         | 32 ++++++++++++------
>  fs/efivarfs/internal.h      |  3 +-
>  fs/efivarfs/super.c         |  9 +++--
>  include/linux/efi.h         |  2 ++
>  6 files changed, 162 insertions(+), 35 deletions(-)
 
Only the last two patches (4 and 5) of this series are required to fix
this bricking issue, right? The fix needs backporting to stable
kernels and if the first three patches are just improving the
robustness of UCS-2 handling, they don't qualify for backport.

> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index e084d08..3a16a57 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -186,9 +186,32 @@ static const struct variable_validate variable_validate[] = {
>  	{ EFI_GLOBAL_VARIABLE_GUID, "Timeout", validate_uint16 },
>  	{ EFI_GLOBAL_VARIABLE_GUID, "Lang", validate_ascii_string },
>  	{ EFI_GLOBAL_VARIABLE_GUID, "PlatformLang", validate_ascii_string },
> +	{ EFI_GLOBAL_VARIABLE_GUID, "OsIndications", NULL },
>  	{ NULL_GUID, "", NULL },
>  };

It would be good to include a comment above this table that documents
what it is used for since we're adding one more use.
  
> +bool
> +efivar_variable_is_protected(efi_guid_t vendor, const char *var_name,
> +			     size_t len)
> +{
> +	int i;
> +	bool found = false;
> +	int match = 0;
>  
> -			/* Reached the end of the string while matching */
> -			if (!c) {
> -				kfree(utf8_name);
> -				return variable_validate[i].validate(var_name,
> -							     match, data, len);
> -			}
> +	/*
> +	 * Now check the validated variables list and then the whitelist -
> +	 * both are whitelists
> +	 */
> +	for (i = 0; variable_validate[i].name[0] != '\0'; i++) {
> +		if (efi_guidcmp(variable_validate[i].guid, vendor))
> +			continue;
> +
> +		if (variable_matches(var_name, len,
> +				     variable_validate[i].name, &match)) {
> +			found = true;
> +			break;
>  		}
>  	}
>  
> -	kfree(utf8_name);
> +	/*
> +	 * If we found it in our list, it /isn't/ one of our protected names.
> +	 */
> +	if (found)
> +		return false;
>  	return true;
>  }
> -EXPORT_SYMBOL_GPL(efivar_validate);
> +EXPORT_SYMBOL_GPL(efivar_variable_is_protected);

"protected" is a bit of a vague word in this context.

How about inverting the return values and naming it,

efivar_variable_is_removable() ? Or efivar_variable_is_deletable() ?
  
> @@ -102,22 +104,17 @@ static void efivarfs_hex_to_guid(const char *str, efi_guid_t *guid)
>  static int efivarfs_create(struct inode *dir, struct dentry *dentry,
>  			  umode_t mode, bool excl)
>  {
> -	struct inode *inode;
> +	struct inode *inode = NULL;
>  	struct efivar_entry *var;
>  	int namelen, i = 0, err = 0;
> +	bool is_protected = false;
>  
>  	if (!efivarfs_valid_name(dentry->d_name.name, dentry->d_name.len))
>  		return -EINVAL;
>  
> -	inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
> -	if (!inode)
> -		return -ENOMEM;
> -
>  	var = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
> -	if (!var) {
> -		err = -ENOMEM;
> -		goto out;
> -	}
> +	if (!var)
> +		return -ENOMEM;
>  
>  	/* length of the variable name itself: remove GUID and separator */
>  	namelen = dentry->d_name.len - EFI_VARIABLE_GUID_LEN - 1;
> @@ -125,6 +122,18 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
>  	efivarfs_hex_to_guid(dentry->d_name.name + namelen + 1,
>  			&var->var.VendorGuid);
>  
> +	if (efivar_variable_is_protected(var->var.VendorGuid,
> +					 dentry->d_name.name,
> +					 dentry->d_name.len))
> +		is_protected = true;
> +
> +	inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0, is_protected);
> +	if (!inode) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +	inode->i_flags = 0;
> +

What's the point looking up efivar_variable_is_protected() if you
trash ->i_flags?