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

[Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Thu, Feb 18, 2016 at 02:56:50PM +0000, Matt Fleming wrote:
> On Tue, 16 Feb, at 11:09:43AM, 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.
> > 
> > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  Documentation/filesystems/efivarfs.txt         |   7 ++
> >  drivers/firmware/efi/vars.c                    | 115 ++++++++++++++++++++-----
> >  fs/efivarfs/file.c                             |  70 +++++++++++++++
> >  fs/efivarfs/inode.c                            |  30 ++++---
> >  fs/efivarfs/internal.h                         |   3 +-
> >  fs/efivarfs/super.c                            |   9 +-
> >  include/linux/efi.h                            |   4 +-
> >  tools/testing/selftests/efivarfs/efivarfs.sh   |  19 +++-
> >  tools/testing/selftests/efivarfs/open-unlink.c |  72 +++++++++++++++-
> >  9 files changed, 287 insertions(+), 42 deletions(-)
>  
> [...]
> 
> > @@ -193,17 +195,59 @@ 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 },
> > +	{ LINUX_EFI_CRASH_GUID, "*", NULL },
> >  	{ NULL_GUID, "", NULL },
> >  };
> >  
> 
> You don't need to fold in patches that can be applied verbatim from
> upstream. It makes validation of backports harder and the git history
> bizarre.

Fair enough.

> > +static bool
> > +variable_matches(const efi_char16_t *ucs2_name, 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;
> > +
> > +		/* All special variables are plain ascii unless they were
> > +		 * wildcards. */
> > +		if (ucs2_name[*match] > 127)
> > +			return false;
> > +
> 
> This caught my eye. You've inverted the check from here,
> 
> [...]
>   
> > -			/* All special variables are plain ascii */
> > -			if (u > 127)
> > -				return true;
> 
> Was that intentional?

Yes - the original one (the bottom here) the return is "is the variable
valid", and we're declaring that anything outside of our list does not
/need/ to be validated, and is thus valid.  

In the new one (top), the function is "does the variable match this
entry in the table", and we're saying any case that isn't ASCII does not
match, except by globbing.  The calling function, efivar_validate(),
then never runs a ->validate() method on the variable data, eventually
exits the loop having matched nothing, and returns true.

Do you want me to resend with the crash guid one line fix as its own
separate patch?

-- 
  Peter