efivarfs immutable files patch set.

efivarfs immutable files patch set.

[Peter Jones]

Hi Matt,
The next few emails are the latest (and hopefully final) version of my
efivarfs immutability patch set, all shiny and chrome.  This version
has:

- everything you and I talked about fixed
- *probably* that oops you saw fixed - at least there was one oops in
  that patch that is fixed.
- everything fixed that Leif Lindholm noticed 
- everything fixed Joey Li noticed
- an inode locking error Mateusz Guzik noticed
- a bug where from _create() we were including the guid in the name, so
  the check for is_removable failed.

Also it seems to work for Joey, Leif, and I.  So please go ahead and
throw it on your test server and whatnot.  If all goes well, I'll send
you a version against Linus' v4.4 for stable, based on just the last two
patches.  (I'm testing that one now.)

Thanks!

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

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

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

[Laszlo Ersek]

(I imported these messages from the gmane archive, after reading about
this work on LWN. Sorry if I'm not looking at the latest patches.)

On 02/04/16 16:34, Peter Jones wrote:
> 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..
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> Tested-by: Lee, Chun-Yi <jlee-IBi9RG/b67k-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> Acked-by: Matthew Garrett <mjg59-JW9irJGTvgXQT0dZR+AlfA-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> ---
>  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);
> 

Since this code is being added to a generic library, I have two comments
/ questions:

(1) shouldn't we handle the endianness of ucs2_char_t explicitly?

https://en.wikipedia.org/wiki/UTF-16#Byte_order_encoding_schemes

(2) Code *units* that stand for low or high halves of surrogate pairs
(0xD800 to 0xDFFF) are not treated specially; meaning the unicode code
*point* they represent (from U+10000 to U+10FFFF) is not decoded, and
then separately encoded to UTF-8. Instead, the above will transcode the
surrogates individually to UTF-8, which looks invalid.

https://en.wikipedia.org/wiki/UTF-16#U.2B10000_to_U.2B10FFFF

https://en.wikipedia.org/wiki/UTF-8#Invalid_code_points

Has this been considered?

Again, the above two questions don't seem relevant for UEFI
specifically: first, UEFI is little-endian only; second, UEFI does not
support surrogate characters -- I found a hint about this in the HII
chapter of the spec, "31.2.6.2.2 Surrogate Area".

But since this code is being added to a generic library, the UEFI
assumptions may not hold for other (future) callers.

Or does "ucs2" -- as opposed to "utf16" -- imply that the caller is
responsible for not passing in code units from the surrogate area? If
so, I think this should be spelled out in a comment as well, and maybe
even WARN'd about.

Just my two cents; I'm by no means a unicode expert.

Thanks
Laszlo

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

[Peter Jones]

On Fri, Feb 12, 2016 at 02:22:29PM +0100, Laszlo Ersek wrote:
> (I imported these messages from the gmane archive, after reading about
> this work on LWN. Sorry if I'm not looking at the latest patches.)
> 
> On 02/04/16 16:34, Peter Jones wrote:
> > 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..
> > 
> > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> > Tested-by: Lee, Chun-Yi <jlee-IBi9RG/b67k-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> > Acked-by: Matthew Garrett <mjg59-JW9irJGTvgXQT0dZR+AlfA-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> > ---
> >  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);
> > 
> 
> Since this code is being added to a generic library, I have two comments
> / questions:
> 
> (1) shouldn't we handle the endianness of ucs2_char_t explicitly?
> 
> https://en.wikipedia.org/wiki/UTF-16#Byte_order_encoding_schemes

TBH I've explicitly ignored this because EFI is always LE.  If somebody
else decides they're using this code for something else, then either a)
we'll need to add some cpu_to_le16() and whatnot, or declare it a
prerequisite on the input.

> (2) Code *units* that stand for low or high halves of surrogate pairs
> (0xD800 to 0xDFFF) are not treated specially; meaning the unicode code
> *point* they represent (from U+10000 to U+10FFFF) is not decoded, and
> then separately encoded to UTF-8. Instead, the above will transcode the
> surrogates individually to UTF-8, which looks invalid.
> 
> https://en.wikipedia.org/wiki/UTF-16#U.2B10000_to_U.2B10FFFF
> 
> https://en.wikipedia.org/wiki/UTF-8#Invalid_code_points
> 
> Has this been considered?
> 
> Again, the above two questions don't seem relevant for UEFI
> specifically: first, UEFI is little-endian only; second, UEFI does not
> support surrogate characters -- I found a hint about this in the HII
> chapter of the spec, "31.2.6.2.2 Surrogate Area".
>
> But since this code is being added to a generic library, the UEFI
> assumptions may not hold for other (future) callers.
> 
> Or does "ucs2" -- as opposed to "utf16" -- imply that the caller is
> responsible for not passing in code units from the surrogate area? If
> so, I think this should be spelled out in a comment as well, and maybe
> even WARN'd about.

UCS-2 means /there are no surrogates/.  Everything directly maps to the
16-bit subset set of unicode characters.

-- 
        Peter

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

[Matt Fleming]

On Fri, 12 Feb, at 02:22:29PM, Laszlo Ersek wrote:
> 
> Again, the above two questions don't seem relevant for UEFI
> specifically: first, UEFI is little-endian only; second, UEFI does not
> support surrogate characters -- I found a hint about this in the HII
> chapter of the spec, "31.2.6.2.2 Surrogate Area".
> 
> But since this code is being added to a generic library, the UEFI
> assumptions may not hold for other (future) callers.

We're not generally in the habit of adding code in the kernel to cater
for users that don't exist yet (in this case, non-UEFI), because doing
so pretty much always leads to bit-rot and maintenance headaches down
the line.

If someone wants to use this library in the future for non-UEFI
things, they'll have to make the necessary modifications.

[PATCH 2/5] 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>
Tested-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Acked-by: Matthew Garrett <mjg59-JW9irJGTvgXQT0dZR+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

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

[Matt Fleming]

On Thu, 04 Feb, at 10:34:31AM, Peter Jones wrote:
> 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>
> Tested-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
> Acked-by: Matthew Garrett <mjg59-JW9irJGTvgXQT0dZR+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;
>  

'variable_name_size' is now unused, so you can delete it.

[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>
Tested-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Acked-by: Matthew Garrett <mjg59-JW9irJGTvgXQT0dZR+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

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

[Matt Fleming]

On Thu, 04 Feb, at 10:34:32AM, Peter Jones wrote:
> 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>
> Tested-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
> Acked-by: Matthew Garrett <mjg59-JW9irJGTvgXQT0dZR+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);

The use of 'len' here is incorrect. 'len' is actually ->DataSize if
you follow the call stack down from efivar_create(). Yeah, it's poorly
named.

What you actually want to be using here is 'utf8_size'. Furthermore,
ucs2_as_utf8() doesn't guarantee NUL termination, so you need to
handle that.

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

[Peter Jones]

v2 correctly checks the guid for validation *as well as* attribute
whitelisting.
v3 moves the sorting of variable_validate here and adds a comment about
it.

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>
---
 drivers/firmware/efi/efivars.c |  5 ++--
 drivers/firmware/efi/vars.c    | 52 +++++++++++++++++++++++++++---------------
 include/linux/efi.h            |  3 ++-
 3 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index cd53c4d..e0b1e37 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->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..0fc9194 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -165,31 +165,42 @@ 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);
 };
 
+/*
+ * This is the list of variables we need to validate.
+ *
+ * If it has a validate() method that's not NULL, it'll go into the
+ * validation routine.  If not, it is assumed valid.
+ *
+ * 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[] = {
-	{ "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, "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 },
+	{ 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, "Lang", validate_ascii_string },
+	{ EFI_GLOBAL_VARIABLE_GUID, "PlatformLang", validate_ascii_string },
+	{ EFI_GLOBAL_VARIABLE_GUID, "Timeout", validate_uint16 },
+	{ 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 +213,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 +875,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

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

[Matt Fleming]

On Thu, 04 Feb, at 10:34:33AM, Peter Jones wrote:
> v2 correctly checks the guid for validation *as well as* attribute
> whitelisting.
> v3 moves the sorting of variable_validate here and adds a comment about
> it.
> 
> 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>
> ---
>  drivers/firmware/efi/efivars.c |  5 ++--
>  drivers/firmware/efi/vars.c    | 52 +++++++++++++++++++++++++++---------------
>  include/linux/efi.h            |  3 ++-
>  3 files changed, 38 insertions(+), 22 deletions(-)

I've expanded on the commit message ever so slightly so that a casual
observer can figure out why this change was made.

Something like this?

---

  efi: Make our variable validation list include the guid
  
  This list of variables is defined to be part of the global namespace
  in the UEFI spec, so this just further ensures we're validating the
  variables we think we are.
  
  Including the guid for entries will become more important in future
  patches when we decide whether or not to allow deletion of variables
  based on presence in this list.

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

[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
v4: - fix a double-free on the end of list traversal
v5: - fix the inode locking in _setxflags()
    - use namelen not dentry->d_name.len when we're calling
      efivar_variable_is_removable() from efivarfs_create()

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>
---
 Documentation/filesystems/efivarfs.txt         |  7 ++
 drivers/firmware/efi/vars.c                    | 88 +++++++++++++++++++-------
 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, 259 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 0fc9194..721194e 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 len)
@@ -220,35 +248,49 @@ 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)) {
+			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);
-			}
+			kfree(utf8_name);
+			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..d48e0d2 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 3381b9d..e2ab6d0 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 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

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

[Matt Fleming]

On Thu, 04 Feb, at 10:34:34AM, 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
> v4: - fix a double-free on the end of list traversal
> v5: - fix the inode locking in _setxflags()
>     - use namelen not dentry->d_name.len when we're calling
>       efivar_variable_is_removable() from efivarfs_create()
> 
> 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>
> ---
>  Documentation/filesystems/efivarfs.txt         |  7 ++
>  drivers/firmware/efi/vars.c                    | 88 +++++++++++++++++++-------
>  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, 259 insertions(+), 41 deletions(-)

[...]

> +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
> +	 */

I don't understand this comment. There are two lists?

efi: make most efivarfs files immutable by default

[Peter Jones]

Hi,
Here's the latest version of my patch series to make efivarfs make files
be immutable by default unless they're on our whitelist.  As you might
expect, this version has a couple of differences from the previous one:

In patch 2/5:
- efivar_create_sysfs_entry() was reworked to use the length it computes
  instead of lots of calls to strlen(), and it doesn't have unused
  variables any more.
In 3/5:
- "len" was renamed to "data_size" for clarity, and we calculate the
  variable name's size correctly instead now.
In 4/5:
- The commit message was reworded based largely on mfleming's suggestions.
In 5/5:
- An old comment from a previous version was fixed to not be confusing.
- the inode lock is now taken when we're modifying inode metadata.

Thanks,
Peter

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

Signed-off-by: Peter Jones <pjones@redhat.com>
Tested-by: Lee, Chun-Yi <jlee@suse.com>
Acked-by: Matthew Garrett <mjg59@coreos.com>
---
 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 3/5] efi: do variable name validation tests in utf8 (v2)

[Peter Jones]

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

v2: - rename efivar_validate()'s "len" parameter to "data_size" for clarity
    - use the right limit on ucs2_as_utf8 and terminate it manually.

Signed-off-by: Peter Jones <pjones@redhat.com>
Acked-by: Matthew Garrett <mjg59@coreos.com>

An earlier version that's not hugely different was:
Tested-by: Lee, Chun-Yi <jlee@suse.com>
---
 drivers/firmware/efi/vars.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 70a0fb1..5c5fde3 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -189,10 +189,19 @@ static const struct variable_validate variable_validate[] = {
 };
 
 bool
-efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long len)
+efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long data_size)
 {
 	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, utf8_size);
+	utf8_name[utf8_size] = '\0';
 
 	for (i = 0; variable_validate[i].validate != NULL; i++) {
 		const char *name = variable_validate[i].name;
@@ -200,28 +209,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);
+							match, data, data_size);
+			}
 
 			/* 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);
+							match, data, data_size);
+			}
 		}
 	}
 
+	kfree(utf8_name);
 	return true;
 }
 EXPORT_SYMBOL_GPL(efivar_validate);
-- 
2.5.0

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

[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
v4: - fix a double-free on the end of list traversal
v5: - fix the inode locking in _setxflags()
    - use namelen not dentry->d_name.len when we're calling
      efivar_variable_is_removable() from efivarfs_create()

Signed-off-by: Peter Jones <pjones@redhat.com>
Tested-by: Lee, Chun-Yi <jlee@suse.com>
Acked-by: Matthew Garrett <mjg59@coreos.com>
---
 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 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 9a53da2..50f10ba 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 c424e48..d48e0d2 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 3381b9d..e2ab6d0 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 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

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

[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.
v3: rework efivar_create_sysfs_entry not to have extra variables and not
    to call strlen() over and over on the same string.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Matthew Garrett <mjg59-JW9irJGTvgXQT0dZR+AlfA@public.gmane.org>

An earlier version that's not hugely different was:
Tested-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@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 756eca8..f4ff8ab 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 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 4/5] efi: make our variable validation list include the guid (v3)

[Peter Jones]

All the variables in this list so far are defined to be in the global
namespace in the UEFI spec, so this just further ensures we're
validating the variables we think we are.

Including the guid for entries will become more important in future
patches when we decide whether or not to allow deletion of variables
based on presence in this list.

v2 correctly checks the guid for validation *as well as* attribute
whitelisting.
v3 moves the sorting of variable_validate here and adds a comment about
it.

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>
---
 drivers/firmware/efi/efivars.c |  5 ++--
 drivers/firmware/efi/vars.c    | 52 +++++++++++++++++++++++++++---------------
 include/linux/efi.h            |  3 ++-
 3 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index f4ff8ab..10e6774 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->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 5c5fde3..9a53da2 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -165,31 +165,42 @@ 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);
 };
 
+/*
+ * This is the list of variables we need to validate.
+ *
+ * If it has a validate() method that's not NULL, it'll go into the
+ * validation routine.  If not, it is assumed valid.
+ *
+ * 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[] = {
-	{ "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, "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 },
+	{ 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, "Lang", validate_ascii_string },
+	{ EFI_GLOBAL_VARIABLE_GUID, "PlatformLang", validate_ascii_string },
+	{ EFI_GLOBAL_VARIABLE_GUID, "Timeout", validate_uint16 },
+	{ NULL_GUID, "", NULL },
 };
 
 bool
-efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long data_size)
+efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
+		unsigned long data_size)
 {
 	int i;
 	unsigned long utf8_size;
@@ -203,9 +214,12 @@ efivar_validate(efi_char16_t *var_name, u8 *data, unsigned long data_size)
 	ucs2_as_utf8(utf8_name, var_name, utf8_size);
 	utf8_name[utf8_size] = '\0';
 
-	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];
@@ -862,7 +876,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

Re: efi: make most efivarfs files immutable by default

[Matt Fleming]

On Mon, 08 Feb, at 02:48:10PM, Peter Jones wrote:
> Hi,
> Here's the latest version of my patch series to make efivarfs make files
> be immutable by default unless they're on our whitelist.  As you might
> expect, this version has a couple of differences from the previous one:
> 
> In patch 2/5:
> - efivar_create_sysfs_entry() was reworked to use the length it computes
>   instead of lots of calls to strlen(), and it doesn't have unused
>   variables any more.
> In 3/5:
> - "len" was renamed to "data_size" for clarity, and we calculate the
>   variable name's size correctly instead now.
> In 4/5:
> - The commit message was reworded based largely on mfleming's suggestions.
> In 5/5:
> - An old comment from a previous version was fixed to not be confusing.
> - the inode lock is now taken when we're modifying inode metadata.

Thanks Peter. I've applied all of these patches to the EFI 'urgent'
branch.

I've got a little more testing to do, but assuming it all goes OK I'll
send a pull request to tip this Friday.

[PATCH] efi: minor fixup in efivar_validate() declaration

[Peter Jones]

When I switched the function to data_size as the parameter name, I
forgot to fix the header.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 include/linux/efi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 1869d5c..47be3ad 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1200,7 +1200,7 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
 				       struct list_head *head, bool remove);
 
 bool efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
-		     unsigned long len);
+		     unsigned long data_size);
 bool efivar_variable_is_removable(efi_guid_t vendor, const char *name,
 				  size_t len);
 
-- 
2.5.0

Re: [PATCH] efi: minor fixup in efivar_validate() declaration

[Matt Fleming]

On Wed, 10 Feb, at 09:51:02AM, Peter Jones wrote:
> When I switched the function to data_size as the parameter name, I
> forgot to fix the header.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  include/linux/efi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 1869d5c..47be3ad 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1200,7 +1200,7 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
>  				       struct list_head *head, bool remove);
>  
>  bool efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
> -		     unsigned long len);
> +		     unsigned long data_size);
>  bool efivar_variable_is_removable(efi_guid_t vendor, const char *name,
>  				  size_t len);

Thanks Peter, I folded this snippet into your patch that adds the
'vendor' argument to the efivar_validate() prototype. Let me know if
that's not OK.

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

[Laszlo Ersek]

On 02/04/16 16:34, 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.

I think that the whitelist should include the following pattern
(permitting deletion):
- GUID:                  LINUX_EFI_CRASH_GUID
- variable name pattern: "dump-type*"

This is because the pstore filesystem can be backed by UEFI variables,
and (for example) a crash might dump the last kilobytes of the dmesg
into a number of pstore entries, each entry backed by a separate UEFI
variable in the above GUID namespace, and with a variable name according
to the above pattern.

Please see "drivers/firmware/efi/efi-pstore.c".

While this patch series will not prevent the user from deleting those
UEFI variables via the pstore filesystem (i.e., deleting a pstore fs
entry will continue to delete the backing UEFI variable), I think it
would be nice to preserve the possibility for the sysadmin to delete
Linux-created UEFI variables that carry portions of the crash log,
*without* having to mount the pstore filesystem.

Thanks
Laszlo

> 
> 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
> v4: - fix a double-free on the end of list traversal
> v5: - fix the inode locking in _setxflags()
>     - use namelen not dentry->d_name.len when we're calling
>       efivar_variable_is_removable() from efivarfs_create()
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> Tested-by: Lee, Chun-Yi <jlee-IBi9RG/b67k-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> Acked-by: Matthew Garrett <mjg59-JW9irJGTvgXQT0dZR+AlfA-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> ---
>  Documentation/filesystems/efivarfs.txt         |  7 ++
>  drivers/firmware/efi/vars.c                    | 88 +++++++++++++++++++-------
>  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, 259 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 0fc9194..721194e 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 len)
> @@ -220,35 +248,49 @@ 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)) {
> +			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);
> -			}
> +			kfree(utf8_name);
> +			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..d48e0d2 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 3381b9d..e2ab6d0 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 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");
> 

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

[Peter Jones]

On Fri, Feb 12, 2016 at 02:36:28PM +0100, Laszlo Ersek wrote:
> On 02/04/16 16:34, 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.
> 
> I think that the whitelist should include the following pattern
> (permitting deletion):
> - GUID:                  LINUX_EFI_CRASH_GUID
> - variable name pattern: "dump-type*"
> 
> This is because the pstore filesystem can be backed by UEFI variables,
> and (for example) a crash might dump the last kilobytes of the dmesg
> into a number of pstore entries, each entry backed by a separate UEFI
> variable in the above GUID namespace, and with a variable name according
> to the above pattern.
> 
> Please see "drivers/firmware/efi/efi-pstore.c".
> 
> While this patch series will not prevent the user from deleting those
> UEFI variables via the pstore filesystem (i.e., deleting a pstore fs
> entry will continue to delete the backing UEFI variable), I think it
> would be nice to preserve the possibility for the sysadmin to delete
> Linux-created UEFI variables that carry portions of the crash log,
> *without* having to mount the pstore filesystem.

They still have that ability with this patch - they have to chattr -i it
manually, or use libefivar's efi_del_variable() with a suitably new
libefivar, but we aren't ever actually stopping deletion.

Which doesn't mean you're wrong, mind you, but it does remove urgency.

-- 
        Peter

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

[Matt Fleming]

On Fri, 12 Feb, at 10:09:49AM, Peter Jones wrote:
> On Fri, Feb 12, 2016 at 02:36:28PM +0100, Laszlo Ersek wrote:
> > On 02/04/16 16:34, 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.
> > 
> > I think that the whitelist should include the following pattern
> > (permitting deletion):
> > - GUID:                  LINUX_EFI_CRASH_GUID
> > - variable name pattern: "dump-type*"
> > 
> > This is because the pstore filesystem can be backed by UEFI variables,
> > and (for example) a crash might dump the last kilobytes of the dmesg
> > into a number of pstore entries, each entry backed by a separate UEFI
> > variable in the above GUID namespace, and with a variable name according
> > to the above pattern.
> > 
> > Please see "drivers/firmware/efi/efi-pstore.c".
> > 
> > While this patch series will not prevent the user from deleting those
> > UEFI variables via the pstore filesystem (i.e., deleting a pstore fs
> > entry will continue to delete the backing UEFI variable), I think it
> > would be nice to preserve the possibility for the sysadmin to delete
> > Linux-created UEFI variables that carry portions of the crash log,
> > *without* having to mount the pstore filesystem.
> 
> They still have that ability with this patch - they have to chattr -i it
> manually, or use libefivar's efi_del_variable() with a suitably new
> libefivar, but we aren't ever actually stopping deletion.
> 
> Which doesn't mean you're wrong, mind you, but it does remove urgency.

Well, we should avoid having different whitelists in different kernel
versions, so I think there is some urgency in fixing the efi-pstore
case now.

Remember, we're breaking userspace ABI with these changes, which is a
major downside that is only offset by the risk of bricking the
machine. There's no chance of bricking by deleting the efi-pstore
variables, so we'd be breaking userspace ABI for no reason.

Would something like this allow efi-pstore variables to be deleted
(and anything else that uses LINUX_EFI_CRASH_GUID in future)?

---

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 50f10bad2604..7f2ea21c730d 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -198,6 +198,7 @@ static const struct variable_validate variable_validate[] = {
 	{ EFI_GLOBAL_VARIABLE_GUID, "OsIndications", NULL },
 	{ EFI_GLOBAL_VARIABLE_GUID, "PlatformLang", validate_ascii_string },
 	{ EFI_GLOBAL_VARIABLE_GUID, "Timeout", validate_uint16 },
+	{ LINUX_EFI_CRASH_GUID, "*", NULL },
 	{ NULL_GUID, "", NULL },
 };

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

[Peter Jones]

On Mon, Feb 15, 2016 at 10:48:01AM +0000, Matt Fleming wrote:
> On Fri, 12 Feb, at 10:09:49AM, Peter Jones wrote:
> > On Fri, Feb 12, 2016 at 02:36:28PM +0100, Laszlo Ersek wrote:
> > > On 02/04/16 16:34, 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.
> > > 
> > > I think that the whitelist should include the following pattern
> > > (permitting deletion):
> > > - GUID:                  LINUX_EFI_CRASH_GUID
> > > - variable name pattern: "dump-type*"
> > > 
> > > This is because the pstore filesystem can be backed by UEFI variables,
> > > and (for example) a crash might dump the last kilobytes of the dmesg
> > > into a number of pstore entries, each entry backed by a separate UEFI
> > > variable in the above GUID namespace, and with a variable name according
> > > to the above pattern.
> > > 
> > > Please see "drivers/firmware/efi/efi-pstore.c".
> > > 
> > > While this patch series will not prevent the user from deleting those
> > > UEFI variables via the pstore filesystem (i.e., deleting a pstore fs
> > > entry will continue to delete the backing UEFI variable), I think it
> > > would be nice to preserve the possibility for the sysadmin to delete
> > > Linux-created UEFI variables that carry portions of the crash log,
> > > *without* having to mount the pstore filesystem.
> > 
> > They still have that ability with this patch - they have to chattr -i it
> > manually, or use libefivar's efi_del_variable() with a suitably new
> > libefivar, but we aren't ever actually stopping deletion.
> > 
> > Which doesn't mean you're wrong, mind you, but it does remove urgency.
> 
> Well, we should avoid having different whitelists in different kernel
> versions, so I think there is some urgency in fixing the efi-pstore
> case now.
> 
> Remember, we're breaking userspace ABI with these changes, which is a
> major downside that is only offset by the risk of bricking the
> machine. There's no chance of bricking by deleting the efi-pstore
> variables, so we'd be breaking userspace ABI for no reason.
> 
> Would something like this allow efi-pstore variables to be deleted
> (and anything else that uses LINUX_EFI_CRASH_GUID in future)?

Yes, this patch works as expected:

Acked-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Tested-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

> 
> ---
> 
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 50f10bad2604..7f2ea21c730d 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -198,6 +198,7 @@ static const struct variable_validate variable_validate[] = {
>  	{ EFI_GLOBAL_VARIABLE_GUID, "OsIndications", NULL },
>  	{ EFI_GLOBAL_VARIABLE_GUID, "PlatformLang", validate_ascii_string },
>  	{ EFI_GLOBAL_VARIABLE_GUID, "Timeout", validate_uint16 },
> +	{ LINUX_EFI_CRASH_GUID, "*", NULL },
>  	{ NULL_GUID, "", NULL },
>  };

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

[Matt Fleming]

On Mon, 15 Feb, at 12:02:16PM, Peter Jones wrote:
> On Mon, Feb 15, 2016 at 10:48:01AM +0000, Matt Fleming wrote:
> > On Fri, 12 Feb, at 10:09:49AM, Peter Jones wrote:
> > > On Fri, Feb 12, 2016 at 02:36:28PM +0100, Laszlo Ersek wrote:
> > > > On 02/04/16 16:34, 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.
> > > > 
> > > > I think that the whitelist should include the following pattern
> > > > (permitting deletion):
> > > > - GUID:                  LINUX_EFI_CRASH_GUID
> > > > - variable name pattern: "dump-type*"
> > > > 
> > > > This is because the pstore filesystem can be backed by UEFI variables,
> > > > and (for example) a crash might dump the last kilobytes of the dmesg
> > > > into a number of pstore entries, each entry backed by a separate UEFI
> > > > variable in the above GUID namespace, and with a variable name according
> > > > to the above pattern.
> > > > 
> > > > Please see "drivers/firmware/efi/efi-pstore.c".
> > > > 
> > > > While this patch series will not prevent the user from deleting those
> > > > UEFI variables via the pstore filesystem (i.e., deleting a pstore fs
> > > > entry will continue to delete the backing UEFI variable), I think it
> > > > would be nice to preserve the possibility for the sysadmin to delete
> > > > Linux-created UEFI variables that carry portions of the crash log,
> > > > *without* having to mount the pstore filesystem.
> > > 
> > > They still have that ability with this patch - they have to chattr -i it
> > > manually, or use libefivar's efi_del_variable() with a suitably new
> > > libefivar, but we aren't ever actually stopping deletion.
> > > 
> > > Which doesn't mean you're wrong, mind you, but it does remove urgency.
> > 
> > Well, we should avoid having different whitelists in different kernel
> > versions, so I think there is some urgency in fixing the efi-pstore
> > case now.
> > 
> > Remember, we're breaking userspace ABI with these changes, which is a
> > major downside that is only offset by the risk of bricking the
> > machine. There's no chance of bricking by deleting the efi-pstore
> > variables, so we'd be breaking userspace ABI for no reason.
> > 
> > Would something like this allow efi-pstore variables to be deleted
> > (and anything else that uses LINUX_EFI_CRASH_GUID in future)?
> 
> Yes, this patch works as expected:
> 
> Acked-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Tested-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Thanks Peter. This is what I've got queued up,

---

>From e246eb568bc4cbbdd8a30a3c11151ff9b7ca7312 Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Date: Mon, 15 Feb 2016 10:34:05 +0000
Subject: [PATCH] efi: Add pstore variables to the deletion whitelist

Laszlo explains why this is a good idea,

 'This is because the pstore filesystem can be backed by UEFI variables,
  and (for example) a crash might dump the last kilobytes of the dmesg
  into a number of pstore entries, each entry backed by a separate UEFI
  variable in the above GUID namespace, and with a variable name
  according to the above pattern.

  Please see "drivers/firmware/efi/efi-pstore.c".

  While this patch series will not prevent the user from deleting those
  UEFI variables via the pstore filesystem (i.e., deleting a pstore fs
  entry will continue to delete the backing UEFI variable), I think it
  would be nice to preserve the possibility for the sysadmin to delete
  Linux-created UEFI variables that carry portions of the crash log,
  *without* having to mount the pstore filesystem.'

There's also no chance of causing machines to become bricked by
deleting these variables, which is the whole purpose of excluding
things from the whitelist.

Use the LINUX_EFI_CRASH_GUID guid and a wildcard '*' for the match so
that we don't have to update the string in the future if new variable
name formats are created for crash dump variables.

Reported-by: Laszlo Ersek <lersek-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Tested-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>
Cc: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>
Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
---
 drivers/firmware/efi/vars.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 50f10bad2604..7f2ea21c730d 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -198,6 +198,7 @@ static const struct variable_validate variable_validate[] = {
 	{ EFI_GLOBAL_VARIABLE_GUID, "OsIndications", NULL },
 	{ EFI_GLOBAL_VARIABLE_GUID, "PlatformLang", validate_ascii_string },
 	{ EFI_GLOBAL_VARIABLE_GUID, "Timeout", validate_uint16 },
+	{ LINUX_EFI_CRASH_GUID, "*", NULL },
 	{ NULL_GUID, "", NULL },
 };
 
-- 
2.6.2