* [PATCH 1/5] Add ucs2 -> utf8 helper functions
@ 2016-02-03 16:43 Peter Jones
[not found] ` <1454517834-13736-1-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Peter Jones @ 2016-02-03 16:43 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/5] efi: use ucs2_as_utf8 in efivarfs instead of open coding a bad version
[not found] ` <1454517834-13736-1-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2016-02-03 16:43 ` Peter Jones
2016-02-03 16:43 ` [PATCH 3/5] efi: do variable name validation tests in utf8 Peter Jones
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Peter Jones @ 2016-02-03 16:43 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/5] efi: do variable name validation tests in utf8
[not found] ` <1454517834-13736-1-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-03 16:43 ` [PATCH 2/5] efi: use ucs2_as_utf8 in efivarfs instead of open coding a bad version Peter Jones
@ 2016-02-03 16:43 ` Peter Jones
2016-02-03 16:43 ` [PATCH 4/5] efi: make our variable validation list include the guid (v2) Peter Jones
2016-02-03 16:43 ` [PATCH 5/5] efi: Make efivarfs entries immutable by default. (v3) Peter Jones
3 siblings, 0 replies; 17+ messages in thread
From: Peter Jones @ 2016-02-03 16:43 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/5] efi: make our variable validation list include the guid (v2)
[not found] ` <1454517834-13736-1-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-03 16:43 ` [PATCH 2/5] efi: use ucs2_as_utf8 in efivarfs instead of open coding a bad version Peter Jones
2016-02-03 16:43 ` [PATCH 3/5] efi: do variable name validation tests in utf8 Peter Jones
@ 2016-02-03 16:43 ` Peter Jones
[not found] ` <1454517834-13736-4-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-03 16:43 ` [PATCH 5/5] efi: Make efivarfs entries immutable by default. (v3) Peter Jones
3 siblings, 1 reply; 17+ messages in thread
From: Peter Jones @ 2016-02-03 16:43 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, Peter Jones
v2 correctly checks the guid for validation *as well as* attribute
whitelisting.
Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
drivers/firmware/efi/efivars.c | 5 +++--
drivers/firmware/efi/vars.c | 43 +++++++++++++++++++++++-------------------
include/linux/efi.h | 3 ++-
3 files changed, 29 insertions(+), 22 deletions(-)
diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index eef6331..707b0fd 100644
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -221,7 +221,7 @@ sanity_check(struct efi_variable *var, efi_char16_t *name, efi_guid_t vendor,
}
if ((attributes & ~EFI_VARIABLE_MASK) != 0 ||
- efivar_validate(name, data, size) == false) {
+ efivar_validate(vendor, name, data, size) == false) {
printk(KERN_ERR "efivars: Malformed variable content\n");
return -EINVAL;
}
@@ -447,7 +447,8 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
}
if ((attributes & ~EFI_VARIABLE_MASK) != 0 ||
- efivar_validate(name, data, size) == false) {
+ efivar_validate(new_var->var.VendorGuid, name, data,
+ size) == false) {
printk(KERN_ERR "efivars: Malformed variable content\n");
return -EINVAL;
}
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 36c5a6e..5a88ce0 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -165,31 +165,33 @@ validate_ascii_string(efi_char16_t *var_name, int match, u8 *buffer,
}
struct variable_validate {
+ efi_guid_t vendor;
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
-efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long len)
+efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
+ unsigned long len)
{
int i;
unsigned long utf8_size;
@@ -202,9 +204,12 @@ 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;
+ int match = 0;
+
+ if (efi_guidcmp(vendor, variable_validate[i].vendor))
+ continue;
for (match = 0; ; match++) {
char c = name[match];
@@ -861,7 +866,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
*set = false;
- if (efivar_validate(name, data, *size) == false)
+ if (efivar_validate(*vendor, name, data, *size) == false)
return -EINVAL;
/*
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 569b5a8..52444fd 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1199,7 +1199,8 @@ int efivar_entry_iter(int (*func)(struct efivar_entry *, void *),
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_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
+ unsigned long len);
extern struct work_struct efivar_work;
void efivar_run_worker(void);
--
2.5.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 5/5] efi: Make efivarfs entries immutable by default. (v3)
[not found] ` <1454517834-13736-1-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
` (2 preceding siblings ...)
2016-02-03 16:43 ` [PATCH 4/5] efi: make our variable validation list include the guid (v2) Peter Jones
@ 2016-02-03 16:43 ` Peter Jones
[not found] ` <1454517834-13736-5-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
3 siblings, 1 reply; 17+ messages in thread
From: Peter Jones @ 2016-02-03 16:43 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, 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.
v3:
- takes the extra Timeout out of the whitelist
- fixes whitelist matching to actually work
- inverts the flag on efivarfs_get_inode() and calls it is_removable
- adds documentation and test cases
Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
Documentation/filesystems/efivarfs.txt | 7 ++
drivers/firmware/efi/vars.c | 97 ++++++++++++++++++++------
fs/efivarfs/file.c | 69 ++++++++++++++++++
fs/efivarfs/inode.c | 31 +++++---
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, 268 insertions(+), 41 deletions(-)
diff --git a/Documentation/filesystems/efivarfs.txt b/Documentation/filesystems/efivarfs.txt
index c477af0..686a64b 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 5a88ce0..490cd60 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -171,11 +171,21 @@ struct variable_validate {
unsigned long len);
};
+/*
+ * This list is an 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'll just be used for whitelisting.
+ *
+ * Note that it's sorted by {vendor,name}, but globbed names must come after
+ * any other name with the same prefix.
+ */
static const struct variable_validate variable_validate[] = {
{ 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, "DriverOrder", validate_boot_order },
{ EFI_GLOBAL_VARIABLE_GUID, "Driver*", validate_load_option },
{ EFI_GLOBAL_VARIABLE_GUID, "ConIn", validate_device_path },
{ EFI_GLOBAL_VARIABLE_GUID, "ConInDev", validate_device_path },
@@ -183,12 +193,38 @@ static const struct variable_validate variable_validate[] = {
{ 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, "Timeout", validate_uint16 },
{ 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)++) {
+ 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 len)
@@ -211,35 +247,50 @@ 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, len);
- }
-
- /* Case sensitive match */
- if (c != u)
+ if (variable_matches(utf8_name, utf8_size+1, name, &match)) {
+ kfree(utf8_name);
+ 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, len);
- }
+ return variable_validate[i].validate(var_name, match,
+ data, len);
}
- }
+ }
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;
+
+ /*
+ * 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].vendor, vendor))
+ continue;
+
+ if (variable_matches(var_name, len,
+ variable_validate[i].name, &match)) {
+ found = true;
+ break;
+ }
+ }
+
+ /*
+ * If we found it in our list, it /isn't/ one of our protected names.
+ */
+ 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 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..88242a3 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,17 @@ 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,
+ dentry->d_name.len))
+ 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 +146,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..b450518 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 8651ac2..dd029d1 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 52444fd..1869d5c 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 len);
+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 77edcdc..0572784 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 8c07644..4af74f7 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.5.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] efi: make our variable validation list include the guid (v2)
[not found] ` <1454517834-13736-4-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2016-02-03 17:51 ` joeyli
[not found] ` <20160203175128.GP26698-empE8CJ7fzk2xCFIczX1Fw@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: joeyli @ 2016-02-03 17:51 UTC (permalink / raw)
To: Peter Jones; +Cc: Matt Fleming, linux-efi-u79uwXL29TY76Z2rM5mHXA
Hi Peter,
On Wed, Feb 03, 2016 at 11:43:53AM -0500, Peter Jones wrote:
> v2 correctly checks the guid for validation *as well as* attribute
> whitelisting.
>
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/firmware/efi/efivars.c | 5 +++--
> drivers/firmware/efi/vars.c | 43 +++++++++++++++++++++++-------------------
> include/linux/efi.h | 3 ++-
> 3 files changed, 29 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
> index eef6331..707b0fd 100644
> --- a/drivers/firmware/efi/efivars.c
> +++ b/drivers/firmware/efi/efivars.c
> @@ -221,7 +221,7 @@ sanity_check(struct efi_variable *var, efi_char16_t *name, efi_guid_t vendor,
> }
>
> if ((attributes & ~EFI_VARIABLE_MASK) != 0 ||
> - efivar_validate(name, data, size) == false) {
> + efivar_validate(vendor, name, data, size) == false) {
> printk(KERN_ERR "efivars: Malformed variable content\n");
> return -EINVAL;
> }
> @@ -447,7 +447,8 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
> }
>
> if ((attributes & ~EFI_VARIABLE_MASK) != 0 ||
> - efivar_validate(name, data, size) == false) {
> + efivar_validate(new_var->var.VendorGuid, name, data,
> + size) == false) {
I just built fail here:
CC drivers/firmware/efi/efivars.o
drivers/firmware/efi/efivars.c: In function ‘efivar_create’:
drivers/firmware/efi/efivars.c:450:29: error: ‘struct efi_variable’ has no member named ‘var’
efivar_validate(new_var->var.VendorGuid, name, data,
^
scripts/Makefile.build:258: recipe for target 'drivers/firmware/efi/efivars.o' failed
Regards
Joey Lee
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] efi: make our variable validation list include the guid (v2)
[not found] ` <20160203175128.GP26698-empE8CJ7fzk2xCFIczX1Fw@public.gmane.org>
@ 2016-02-03 17:55 ` Peter Jones
0 siblings, 0 replies; 17+ messages in thread
From: Peter Jones @ 2016-02-03 17:55 UTC (permalink / raw)
To: joeyli; +Cc: Matt Fleming, linux-efi-u79uwXL29TY76Z2rM5mHXA
On Thu, Feb 04, 2016 at 01:51:28AM +0800, joeyli wrote:
> Hi Peter,
>
> On Wed, Feb 03, 2016 at 11:43:53AM -0500, Peter Jones wrote:
> > v2 correctly checks the guid for validation *as well as* attribute
> > whitelisting.
> >
> > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> > drivers/firmware/efi/efivars.c | 5 +++--
> > drivers/firmware/efi/vars.c | 43 +++++++++++++++++++++++-------------------
> > include/linux/efi.h | 3 ++-
> > 3 files changed, 29 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
> > index eef6331..707b0fd 100644
> > --- a/drivers/firmware/efi/efivars.c
> > +++ b/drivers/firmware/efi/efivars.c
> > @@ -221,7 +221,7 @@ sanity_check(struct efi_variable *var, efi_char16_t *name, efi_guid_t vendor,
> > }
> >
> > if ((attributes & ~EFI_VARIABLE_MASK) != 0 ||
> > - efivar_validate(name, data, size) == false) {
> > + efivar_validate(vendor, name, data, size) == false) {
> > printk(KERN_ERR "efivars: Malformed variable content\n");
> > return -EINVAL;
> > }
> > @@ -447,7 +447,8 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
> > }
> >
> > if ((attributes & ~EFI_VARIABLE_MASK) != 0 ||
> > - efivar_validate(name, data, size) == false) {
> > + efivar_validate(new_var->var.VendorGuid, name, data,
> > + size) == false) {
>
> I just built fail here:
>
> CC drivers/firmware/efi/efivars.o
> drivers/firmware/efi/efivars.c: In function ‘efivar_create’:
> drivers/firmware/efi/efivars.c:450:29: error: ‘struct efi_variable’ has no member named ‘var’
> efivar_validate(new_var->var.VendorGuid, name, data,
> ^
> scripts/Makefile.build:258: recipe for target 'drivers/firmware/efi/efivars.o' failed
Thanks for this - I've got a couple more fixes mfleming wanted and then
I'll send out a new version once I've built and tested the whole
patchset again.
--
Peter
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] efi: Make efivarfs entries immutable by default. (v3)
[not found] ` <1454517834-13736-5-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2016-02-03 18:00 ` joeyli
[not found] ` <20160203180016.GQ26698-empE8CJ7fzk2xCFIczX1Fw@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: joeyli @ 2016-02-03 18:00 UTC (permalink / raw)
To: Peter Jones; +Cc: Matt Fleming, linux-efi-u79uwXL29TY76Z2rM5mHXA
On Wed, Feb 03, 2016 at 11:43:54AM -0500, 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.
> v3:
> - takes the extra Timeout out of the whitelist
> - fixes whitelist matching to actually work
> - inverts the flag on efivarfs_get_inode() and calls it is_removable
> - adds documentation and test cases
>
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Tested-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Regards
Joey Lee
> ---
> Documentation/filesystems/efivarfs.txt | 7 ++
> drivers/firmware/efi/vars.c | 97 ++++++++++++++++++++------
> fs/efivarfs/file.c | 69 ++++++++++++++++++
> fs/efivarfs/inode.c | 31 +++++---
> 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, 268 insertions(+), 41 deletions(-)
>
> diff --git a/Documentation/filesystems/efivarfs.txt b/Documentation/filesystems/efivarfs.txt
> index c477af0..686a64b 100644
> --- a/Documentation/filesystems/efivarfs.txt
> +++ b/Documentation/filesystems/efivarfs.txt
> @@ -14,3 +14,10 @@ filesystem.
> efivarfs is typically mounted like this,
>
[...snip]
> +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;
> +}
> +
Yes, this change works on my testing.
Regards
Joey Lee
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] efi: Make efivarfs entries immutable by default. (v3)
[not found] ` <20160203180016.GQ26698-empE8CJ7fzk2xCFIczX1Fw@public.gmane.org>
@ 2016-02-03 18:18 ` Peter Jones
[not found] ` <20160203181759.GB19297-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Peter Jones @ 2016-02-03 18:18 UTC (permalink / raw)
To: joeyli; +Cc: Matt Fleming, linux-efi-u79uwXL29TY76Z2rM5mHXA
On Thu, Feb 04, 2016 at 02:00:16AM +0800, joeyli wrote:
> On Wed, Feb 03, 2016 at 11:43:54AM -0500, 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.
> > v3:
> > - takes the extra Timeout out of the whitelist
> > - fixes whitelist matching to actually work
> > - inverts the flag on efivarfs_get_inode() and calls it is_removable
> > - adds documentation and test cases
> >
> > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> Tested-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Is this to say on 4/5 you did s/new_var->var./new_var->/ and then tested
the whole set?
--
Peter
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] efi: Make efivarfs entries immutable by default. (v3)
[not found] ` <20160203181759.GB19297-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2016-02-04 3:40 ` joeyli
0 siblings, 0 replies; 17+ messages in thread
From: joeyli @ 2016-02-04 3:40 UTC (permalink / raw)
To: Peter Jones; +Cc: Matt Fleming, linux-efi-u79uwXL29TY76Z2rM5mHXA
On Wed, Feb 03, 2016 at 01:18:00PM -0500, Peter Jones wrote:
> On Thu, Feb 04, 2016 at 02:00:16AM +0800, joeyli wrote:
> > On Wed, Feb 03, 2016 at 11:43:54AM -0500, 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.
> > > v3:
> > > - takes the extra Timeout out of the whitelist
> > > - fixes whitelist matching to actually work
> > > - inverts the flag on efivarfs_get_inode() and calls it is_removable
> > > - adds documentation and test cases
> > >
> > > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >
> > Tested-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
>
> Is this to say on 4/5 you did s/new_var->var./new_var->/ and then tested
> the whole set?
>
Yes, I changed the code then built whole patch set success. And, I tested
this set on OVMF to remove some variables in whitelist or not. It works to
me to avoid root removes non-whitelist variables.
Actually I tested your last version, it doesn't have compiler problem but
I found there have some whitelist variables that can not be removed because
variable_matches() has issue to compare name. Looks you fixed it in this
version.
So I put Tested-by tag to this set.
Thanks a lot!
Joey Lee
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] efi: Use ucs2_as_utf8 in efivarfs instead of open coding a bad version
[not found] ` <CAPeXnHuoQgrz1-_zkBKcskNE24jK2L5DSyWjbBoU+ceVzGZe0Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-02-18 9:36 ` H. Peter Anvin
0 siblings, 0 replies; 17+ messages in thread
From: H. Peter Anvin @ 2016-02-18 9:36 UTC (permalink / raw)
To: Matthew Garrett
Cc: Matt Fleming, Ingo Molnar, Thomas Gleixner, Ard Biesheuvel,
Peter Jones, linux-efi-u79uwXL29TY76Z2rM5mHXA, Lee, Chun-Yi
On February 17, 2016 10:09:29 PM PST, Matthew Garrett <mjg59-JW9irJGTvgXQT0dZR+AlfA@public.gmane.org> wrote:
>If we're worried about UTF-16, the appropriate thing for us to do is
>error on seeing a surrogate pair.
>
>On Wed, Feb 17, 2016 at 9:34 PM, H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> wrote:
>> On February 12, 2016 3:27:09 AM PST, Matt Fleming
><matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>>>From: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>
>>>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>
>>>Acked-by: Matthew Garrett <mjg59-JW9irJGTvgXQT0dZR+AlfA@public.gmane.org>
>>>Tested-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>
>>>Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
>>>---
>>> drivers/firmware/efi/efivars.c | 30 +++++++++++-------------------
>>> fs/efivarfs/super.c | 7 +++----
>>> 2 files changed, 14 insertions(+), 23 deletions(-)
>>>
>>>diff --git a/drivers/firmware/efi/efivars.c
>>>b/drivers/firmware/efi/efivars.c
>>>index 756eca8c4cf8..f4ff8abc5f3e 100644
>>>--- a/drivers/firmware/efi/efivars.c
>>>+++ b/drivers/firmware/efi/efivars.c
>>>@@ -540,38 +540,30 @@ 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;
>>>+ unsigned long utf8_name_size;
>>>+ efi_char16_t *variable_name = new_var->var.VariableName;
>>> int ret;
>>>
>>>- variable_name = new_var->var.VariableName;
>>>- variable_name_size = ucs2_strlen(variable_name) *
>>>sizeof(efi_char16_t);
>>>-
>>> /*
>>>- * Length of the variable bytes in ASCII, plus the '-'
>separator,
>>>+ * Length of the variable bytes in UTF8, 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 = kzalloc(short_name_size, GFP_KERNEL);
>>>+ utf8_name_size = ucs2_utf8size(variable_name);
>>>+ short_name_size = utf8_name_size + 1 + EFI_VARIABLE_GUID_LEN +
>1;
>>>
>>>+ short_name = kmalloc(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)) = '-';
>>>+ short_name[utf8_name_size] = '-';
>>> efi_guid_to_str(&new_var->var.VendorGuid,
>>>- short_name + strlen(short_name));
>>>+ short_name + utf8_name_size + 1);
>>>
>>> new_var->kobj.kset = efivars_kset;
>>>
>>>diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
>>>index b8a564f29107..8651ac28ec0d 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] = '-';
>>>
>>
>> However, I think we should treat this "ucs2" as utf16, because sooner
>or later someone will enter utf16 characters.
>> --
>> Sent from my Android device with K-9 Mail. Please excuse brevity and
>formatting.
Error how? Now you're making something in EFI memory inaccessible for no good reason. Most likely utf16 works just fine except when being displayed on the EFI console.
--
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] efi: Use ucs2_as_utf8 in efivarfs instead of open coding a bad version
[not found] ` <12473B1F-5227-4E83-BAF9-06B69CF74D77-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
@ 2016-02-18 6:09 ` Matthew Garrett
[not found] ` <CAPeXnHuoQgrz1-_zkBKcskNE24jK2L5DSyWjbBoU+ceVzGZe0Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Matthew Garrett @ 2016-02-18 6:09 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Matt Fleming, Ingo Molnar, Thomas Gleixner, Ard Biesheuvel,
Peter Jones, linux-efi-u79uwXL29TY76Z2rM5mHXA, Lee, Chun-Yi
If we're worried about UTF-16, the appropriate thing for us to do is
error on seeing a surrogate pair.
On Wed, Feb 17, 2016 at 9:34 PM, H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> wrote:
> On February 12, 2016 3:27:09 AM PST, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>>From: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>
>>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>
>>Acked-by: Matthew Garrett <mjg59-JW9irJGTvgXQT0dZR+AlfA@public.gmane.org>
>>Tested-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>
>>Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
>>---
>> drivers/firmware/efi/efivars.c | 30 +++++++++++-------------------
>> fs/efivarfs/super.c | 7 +++----
>> 2 files changed, 14 insertions(+), 23 deletions(-)
>>
>>diff --git a/drivers/firmware/efi/efivars.c
>>b/drivers/firmware/efi/efivars.c
>>index 756eca8c4cf8..f4ff8abc5f3e 100644
>>--- a/drivers/firmware/efi/efivars.c
>>+++ b/drivers/firmware/efi/efivars.c
>>@@ -540,38 +540,30 @@ 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;
>>+ unsigned long utf8_name_size;
>>+ efi_char16_t *variable_name = new_var->var.VariableName;
>> int ret;
>>
>>- variable_name = new_var->var.VariableName;
>>- variable_name_size = ucs2_strlen(variable_name) *
>>sizeof(efi_char16_t);
>>-
>> /*
>>- * Length of the variable bytes in ASCII, plus the '-' separator,
>>+ * Length of the variable bytes in UTF8, 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 = kzalloc(short_name_size, GFP_KERNEL);
>>+ utf8_name_size = ucs2_utf8size(variable_name);
>>+ short_name_size = utf8_name_size + 1 + EFI_VARIABLE_GUID_LEN + 1;
>>
>>+ short_name = kmalloc(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)) = '-';
>>+ short_name[utf8_name_size] = '-';
>> efi_guid_to_str(&new_var->var.VendorGuid,
>>- short_name + strlen(short_name));
>>+ short_name + utf8_name_size + 1);
>>
>> new_var->kobj.kset = efivars_kset;
>>
>>diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
>>index b8a564f29107..8651ac28ec0d 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] = '-';
>>
>
> However, I think we should treat this "ucs2" as utf16, because sooner or later someone will enter utf16 characters.
> --
> Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] efi: Use ucs2_as_utf8 in efivarfs instead of open coding a bad version
[not found] ` <1455276432-9931-3-git-send-email-matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
@ 2016-02-18 5:34 ` H. Peter Anvin
[not found] ` <12473B1F-5227-4E83-BAF9-06B69CF74D77-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: H. Peter Anvin @ 2016-02-18 5:34 UTC (permalink / raw)
To: Matt Fleming, Ingo Molnar, Thomas Gleixner
Cc: Ard Biesheuvel, Peter Jones, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA, Lee, Chun-Yi, Matthew Garrett
On February 12, 2016 3:27:09 AM PST, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>From: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
>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>
>Acked-by: Matthew Garrett <mjg59-JW9irJGTvgXQT0dZR+AlfA@public.gmane.org>
>Tested-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>
>Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
>---
> drivers/firmware/efi/efivars.c | 30 +++++++++++-------------------
> fs/efivarfs/super.c | 7 +++----
> 2 files changed, 14 insertions(+), 23 deletions(-)
>
>diff --git a/drivers/firmware/efi/efivars.c
>b/drivers/firmware/efi/efivars.c
>index 756eca8c4cf8..f4ff8abc5f3e 100644
>--- a/drivers/firmware/efi/efivars.c
>+++ b/drivers/firmware/efi/efivars.c
>@@ -540,38 +540,30 @@ 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;
>+ unsigned long utf8_name_size;
>+ efi_char16_t *variable_name = new_var->var.VariableName;
> int ret;
>
>- variable_name = new_var->var.VariableName;
>- variable_name_size = ucs2_strlen(variable_name) *
>sizeof(efi_char16_t);
>-
> /*
>- * Length of the variable bytes in ASCII, plus the '-' separator,
>+ * Length of the variable bytes in UTF8, 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 = kzalloc(short_name_size, GFP_KERNEL);
>+ utf8_name_size = ucs2_utf8size(variable_name);
>+ short_name_size = utf8_name_size + 1 + EFI_VARIABLE_GUID_LEN + 1;
>
>+ short_name = kmalloc(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)) = '-';
>+ short_name[utf8_name_size] = '-';
> efi_guid_to_str(&new_var->var.VendorGuid,
>- short_name + strlen(short_name));
>+ short_name + utf8_name_size + 1);
>
> new_var->kobj.kset = efivars_kset;
>
>diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
>index b8a564f29107..8651ac28ec0d 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] = '-';
>
However, I think we should treat this "ucs2" as utf16, because sooner or later someone will enter utf16 characters.
--
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/5] efi: Use ucs2_as_utf8 in efivarfs instead of open coding a bad version
2016-02-12 11:27 [GIT PULL 0/5] EFI urgent fixes Matt Fleming
@ 2016-02-12 11:27 ` Matt Fleming
[not found] ` <1455276432-9931-3-git-send-email-matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Matt Fleming @ 2016-02-12 11:27 UTC (permalink / raw)
To: Ingo Molnar, Thomas Gleixner, H . Peter Anvin
Cc: Ard Biesheuvel, Peter Jones, linux-kernel, linux-efi,
Matt Fleming, Lee, Chun-Yi, Matthew Garrett
From: Peter Jones <pjones@redhat.com>
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@redhat.com>
Acked-by: Matthew Garrett <mjg59@coreos.com>
Tested-by: "Lee, Chun-Yi" <jlee@suse.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
drivers/firmware/efi/efivars.c | 30 +++++++++++-------------------
fs/efivarfs/super.c | 7 +++----
2 files changed, 14 insertions(+), 23 deletions(-)
diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index 756eca8c4cf8..f4ff8abc5f3e 100644
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -540,38 +540,30 @@ 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;
+ unsigned long utf8_name_size;
+ efi_char16_t *variable_name = new_var->var.VariableName;
int ret;
- variable_name = new_var->var.VariableName;
- variable_name_size = ucs2_strlen(variable_name) * sizeof(efi_char16_t);
-
/*
- * Length of the variable bytes in ASCII, plus the '-' separator,
+ * Length of the variable bytes in UTF8, 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 = kzalloc(short_name_size, GFP_KERNEL);
+ utf8_name_size = ucs2_utf8size(variable_name);
+ short_name_size = utf8_name_size + 1 + EFI_VARIABLE_GUID_LEN + 1;
+ short_name = kmalloc(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)) = '-';
+ short_name[utf8_name_size] = '-';
efi_guid_to_str(&new_var->var.VendorGuid,
- short_name + strlen(short_name));
+ short_name + utf8_name_size + 1);
new_var->kobj.kset = efivars_kset;
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index b8a564f29107..8651ac28ec0d 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.6.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] efi: use ucs2_as_utf8 in efivarfs instead of open coding a bad version
[not found] ` <1454504567-2826-2-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2016-02-03 16:42 ` Matt Fleming
0 siblings, 0 replies; 17+ messages in thread
From: Matt Fleming @ 2016-02-03 16:42 UTC (permalink / raw)
To: Peter Jones; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/5] efi: use ucs2_as_utf8 in efivarfs instead of open coding a bad version
[not found] ` <1454504567-2826-1-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2016-02-03 13:02 ` Peter Jones
[not found] ` <1454504567-2826-2-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Peter Jones @ 2016-02-03 13:02 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/5] efi: use ucs2_as_utf8 in efivarfs instead of open coding a bad version
[not found] ` <1454452386-27709-1-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2016-02-02 22:33 ` Peter Jones
0 siblings, 0 replies; 17+ messages in thread
From: Peter Jones @ 2016-02-02 22:33 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-02-18 9:36 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-03 16:43 [PATCH 1/5] Add ucs2 -> utf8 helper functions Peter Jones
[not found] ` <1454517834-13736-1-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-03 16:43 ` [PATCH 2/5] efi: use ucs2_as_utf8 in efivarfs instead of open coding a bad version Peter Jones
2016-02-03 16:43 ` [PATCH 3/5] efi: do variable name validation tests in utf8 Peter Jones
2016-02-03 16:43 ` [PATCH 4/5] efi: make our variable validation list include the guid (v2) Peter Jones
[not found] ` <1454517834-13736-4-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-03 17:51 ` joeyli
[not found] ` <20160203175128.GP26698-empE8CJ7fzk2xCFIczX1Fw@public.gmane.org>
2016-02-03 17:55 ` Peter Jones
2016-02-03 16:43 ` [PATCH 5/5] efi: Make efivarfs entries immutable by default. (v3) Peter Jones
[not found] ` <1454517834-13736-5-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-03 18:00 ` joeyli
[not found] ` <20160203180016.GQ26698-empE8CJ7fzk2xCFIczX1Fw@public.gmane.org>
2016-02-03 18:18 ` Peter Jones
[not found] ` <20160203181759.GB19297-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-04 3:40 ` joeyli
-- strict thread matches above, loose matches on Subject: below --
2016-02-12 11:27 [GIT PULL 0/5] EFI urgent fixes Matt Fleming
2016-02-12 11:27 ` [PATCH 2/5] efi: Use ucs2_as_utf8 in efivarfs instead of open coding a bad version Matt Fleming
[not found] ` <1455276432-9931-3-git-send-email-matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
2016-02-18 5:34 ` H. Peter Anvin
[not found] ` <12473B1F-5227-4E83-BAF9-06B69CF74D77-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
2016-02-18 6:09 ` Matthew Garrett
[not found] ` <CAPeXnHuoQgrz1-_zkBKcskNE24jK2L5DSyWjbBoU+ceVzGZe0Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-02-18 9:36 ` H. Peter Anvin
2016-02-03 13:02 [PATCH 1/5] Add ucs2 -> utf8 helper functions Peter Jones
[not found] ` <1454504567-2826-1-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-03 13:02 ` [PATCH 2/5] efi: use ucs2_as_utf8 in efivarfs instead of open coding a bad version Peter Jones
[not found] ` <1454504567-2826-2-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-03 16:42 ` Matt Fleming
2016-02-02 22:33 Preventing "rm -rf /sys/firmware/efi/efivars/" from damage Peter Jones
[not found] ` <1454452386-27709-1-git-send-email-pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-02 22:33 ` [PATCH 2/5] efi: use ucs2_as_utf8 in efivarfs instead of open coding a bad version Peter Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).