From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matt Fleming Subject: Re: [PATCH 5/5] efi: Make efivarfs entries immutable by default. (v5) Date: Mon, 15 Feb 2016 10:48:01 +0000 Message-ID: <20160215104801.GB2591@codeblueprint.co.uk> References: <1454600074-14854-1-git-send-email-pjones@redhat.com> <1454600074-14854-6-git-send-email-pjones@redhat.com> <56BDDFDC.406@redhat.com> <20160212150948.GC31573@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20160212150948.GC31573-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Peter Jones Cc: Laszlo Ersek , efi kernel list , Matthew Garrett List-Id: linux-efi@vger.kernel.org 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 }, };