[PATCH 0/2] efivars: reading variables can generate SMIs

[PATCH 0/2] efivars: reading variables can generate SMIs

[Joe Konno]

From: Joe Konno <joe.konno@intel.com>

It was pointed out that normal, unprivileged users reading certain EFI
variables (through efivarfs) can generate SMIs. Given these nodes are created
with 0644 permissions, normal users could generate a lot of SMIs. By
restricting permissions a bit (patch 1), we can make it harder for normal users
to generate spurious SMIs.

A normal user could generate lots of SMIs by reading the efivarfs in a trivial
loop:

```
while true; do
    cat /sys/firmware/efi/evivars/* > /dev/null
done
```

Patch 1 in this series limits read and write permissions on efivarfs to the
owner/superuser. Group and world cannot access.

Patch 2 is for consistency and hygiene. If we restrict permissions for either
efivarfs or efi/vars, the other interface should get the same treatment.

Joe Konno (2):
  fs/efivarfs: restrict inode permissions
  efi: restrict top-level attribute permissions

 drivers/firmware/efi/efi.c | 10 ++++++----
 fs/efivarfs/super.c        |  4 ++--
 2 files changed, 8 insertions(+), 6 deletions(-)

-- 
2.14.1

[PATCH 1/2] fs/efivarfs: restrict inode permissions

[Joe Konno]

From: Joe Konno <joe.konno@intel.com>

Efivarfs nodes are created with group and world readable permissions.
Reading certain EFI variables trigger SMIs. So, this is a potential DoS
surface.

Make permissions more restrictive-- only the owner may read or write to
created inodes.

Suggested-by: Andi Kleen <ak@linux.intel.com>
Reported-by: Tony Luck <tony.luck@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Matthew Garrett <matthew.garrett@nebula.com>
Cc: Jeremy Kerr <jk@ozlabs.org>
Cc: linux-efi@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Joe Konno <joe.konno@intel.com>
---
 fs/efivarfs/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 5b68e4294faa..ca98c4e31eb7 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -145,7 +145,7 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 
 	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 | 0600, 0,
 				   is_removable);
 	if (!inode)
 		goto fail_name;
@@ -207,7 +207,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, true);
+	inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0700, 0, true);
 	if (!inode)
 		return -ENOMEM;
 	inode->i_op = &efivarfs_dir_inode_operations;
-- 
2.14.1

[PATCH 2/2] efi: restrict top-level attribute permissions

[Joe Konno]

From: Joe Konno <joe.konno-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Restrict four top-level (/sys/firmware/efi) attributes to match systab.
This is for consistency's sake, as well as hygiene.

Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Joe Konno <joe.konno-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/efi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index cd42f66a7c85..9a1f6c85c8c7 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -167,11 +167,13 @@ static ssize_t fw_platform_size_show(struct kobject *kobj,
 	return sprintf(buf, "%d\n", efi_enabled(EFI_64BIT) ? 64 : 32);
 }
 
-static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
-static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
-static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
+static struct kobj_attribute efi_attr_fw_vendor =
+	__ATTR_RO_MODE(fw_vendor, 0400);
+static struct kobj_attribute efi_attr_runtime = __ATTR_RO_MODE(runtime, 0400);
+static struct kobj_attribute efi_attr_config_table =
+	__ATTR_RO_MODE(config_table, 0400);
 static struct kobj_attribute efi_attr_fw_platform_size =
-	__ATTR_RO(fw_platform_size);
+	__ATTR_RO_MODE(fw_platform_size, 0400);
 
 static struct attribute *efi_subsys_attrs[] = {
 	&efi_attr_systab.attr,
-- 
2.14.1

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Ard Biesheuvel]

On 15 February 2018 at 18:22, Joe Konno <joe.konno@linux.intel.com> wrote:
> From: Joe Konno <joe.konno@intel.com>
>
> It was pointed out that normal, unprivileged users reading certain EFI
> variables (through efivarfs) can generate SMIs. Given these nodes are created
> with 0644 permissions, normal users could generate a lot of SMIs. By
> restricting permissions a bit (patch 1), we can make it harder for normal users
> to generate spurious SMIs.
>
> A normal user could generate lots of SMIs by reading the efivarfs in a trivial
> loop:
>
> ```
> while true; do
>     cat /sys/firmware/efi/evivars/* > /dev/null
> done
> ```
>
> Patch 1 in this series limits read and write permissions on efivarfs to the
> owner/superuser. Group and world cannot access.
>
> Patch 2 is for consistency and hygiene. If we restrict permissions for either
> efivarfs or efi/vars, the other interface should get the same treatment.
>

I am inclined to apply this as a fix, but I will give the x86 guys a
chance to respond as well.


> Joe Konno (2):
>   fs/efivarfs: restrict inode permissions
>   efi: restrict top-level attribute permissions
>
>  drivers/firmware/efi/efi.c | 10 ++++++----
>  fs/efivarfs/super.c        |  4 ++--
>  2 files changed, 8 insertions(+), 6 deletions(-)
>
> --
> 2.14.1
>

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Borislav Petkov]

On Fri, Feb 16, 2018 at 10:41:45AM +0000, Ard Biesheuvel wrote:
> On 15 February 2018 at 18:22, Joe Konno <joe.konno@linux.intel.com> wrote:
> > From: Joe Konno <joe.konno@intel.com>
> >
> > It was pointed out that normal, unprivileged users reading certain EFI
> > variables (through efivarfs) can generate SMIs. Given these nodes are created
> > with 0644 permissions, normal users could generate a lot of SMIs. By
> > restricting permissions a bit (patch 1), we can make it harder for normal users
> > to generate spurious SMIs.
> >
> > A normal user could generate lots of SMIs by reading the efivarfs in a trivial
> > loop:
> >
> > ```
> > while true; do
> >     cat /sys/firmware/efi/evivars/* > /dev/null
> > done
> > ```
> >
> > Patch 1 in this series limits read and write permissions on efivarfs to the
> > owner/superuser. Group and world cannot access.
> >
> > Patch 2 is for consistency and hygiene. If we restrict permissions for either
> > efivarfs or efi/vars, the other interface should get the same treatment.
> >
> 
> I am inclined to apply this as a fix, but I will give the x86 guys a
> chance to respond as well.

That stinking pile EFI never ceases to amaze me...

Just one question: by narrowing permissions this way, aren't you
breaking some userspace which reads those?

And if you do, then that's a no-no.

Which then would mean that you'd have to come up with some caching
scheme to protect the firmware from itself.

Or we could simply admit that EFI is a piece of crap, kill it and
start anew, this time much more conservatively and not add a whole OS
underneath the actual OS.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Ard Biesheuvel]

On 16 February 2018 at 10:55, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Feb 16, 2018 at 10:41:45AM +0000, Ard Biesheuvel wrote:
>> On 15 February 2018 at 18:22, Joe Konno <joe.konno@linux.intel.com> wrote:
>> > From: Joe Konno <joe.konno@intel.com>
>> >
>> > It was pointed out that normal, unprivileged users reading certain EFI
>> > variables (through efivarfs) can generate SMIs. Given these nodes are created
>> > with 0644 permissions, normal users could generate a lot of SMIs. By
>> > restricting permissions a bit (patch 1), we can make it harder for normal users
>> > to generate spurious SMIs.
>> >
>> > A normal user could generate lots of SMIs by reading the efivarfs in a trivial
>> > loop:
>> >
>> > ```
>> > while true; do
>> >     cat /sys/firmware/efi/evivars/* > /dev/null
>> > done
>> > ```
>> >
>> > Patch 1 in this series limits read and write permissions on efivarfs to the
>> > owner/superuser. Group and world cannot access.
>> >
>> > Patch 2 is for consistency and hygiene. If we restrict permissions for either
>> > efivarfs or efi/vars, the other interface should get the same treatment.
>> >
>>
>> I am inclined to apply this as a fix, but I will give the x86 guys a
>> chance to respond as well.
>
> That stinking pile EFI never ceases to amaze me...
>
> Just one question: by narrowing permissions this way, aren't you
> breaking some userspace which reads those?
>
> And if you do, then that's a no-no.
>
> Which then would mean that you'd have to come up with some caching
> scheme to protect the firmware from itself.
>
> Or we could simply admit that EFI is a piece of crap, kill it and
> start anew, this time much more conservatively and not add a whole OS
> underneath the actual OS.
>

By your own reasoning above, that's a no-no as well.

But thanks for your input. Anyone else got something constructive to contribute?

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Borislav Petkov]

On Fri, Feb 16, 2018 at 10:58:47AM +0000, Ard Biesheuvel wrote:
> By your own reasoning above, that's a no-no as well.

I'm sure we can come up with some emulation - the same way we did the
BIOS emulation.

> But thanks for your input. Anyone else got something constructive to contribute?

The not-breaking userspace is constructive contribution. The last
paragraph is my usual rant.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Ard Biesheuvel]

On 16 February 2018 at 11:08, Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> wrote:
> On Fri, Feb 16, 2018 at 10:58:47AM +0000, Ard Biesheuvel wrote:
>> By your own reasoning above, that's a no-no as well.
>
> I'm sure we can come up with some emulation - the same way we did the
> BIOS emulation.
>
>> But thanks for your input. Anyone else got something constructive to contribute?
>
> The not-breaking userspace is constructive contribution. The last
> paragraph is my usual rant.
>

Fair enough. And I am not disagreeing with you either.

So question to Joe: is it well defined which variables may exhibit
this behavior? Given that UEFI variables are GUID scoped, would
whitelisting certain GUIDs (the ones userland currently relies on to
be readable my non-privileged users) and making everything else
user-only solve this problem as well?

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Joe Konno]

[-- Attachment #1: Type: text/plain, Size: 2004 bytes --]

On Fri, Feb 16, 2018 at 11:18:12AM +0000, Ard Biesheuvel wrote:
> On 16 February 2018 at 11:08, Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> wrote:
> > On Fri, Feb 16, 2018 at 10:58:47AM +0000, Ard Biesheuvel wrote:
> >> By your own reasoning above, that's a no-no as well.
> >
> > I'm sure we can come up with some emulation - the same way we did the
> > BIOS emulation.
> >
> >> But thanks for your input. Anyone else got something constructive to contribute?
> >
> > The not-breaking userspace is constructive contribution. The last
> > paragraph is my usual rant.
> >
> 
> Fair enough. And I am not disagreeing with you either.
> 
> So question to Joe: is it well defined which variables may exhibit
> this behavior?

For brevity's sake, "not yet." Folks-- e.g. firmware writers and
equipment makers-- may use SMIs more (or less) than others. So, how many
SMIs generated by the reference shell script can, and will, vary
platform to platform. I and my colleagues are digging into this.

> Given that UEFI variables are GUID scoped, would whitelisting certain
> GUIDs (the ones userland currently relies on to be readable my
> non-privileged users) and making everything else user-only solve this
> problem as well?

Perhaps. I don't want us chasing white rabbits just yet, but the
whitelist is but one approach under consideration. We may see some other
patches or RFCs about caching and/or shadowing variable values in
efivarfs to reduce the number of direct EFI reads, with the goal of
reducing how many SMIs are generated.

Any obvious EFI variables that userspace tools have come to depend on--
those which normal, unprivileged users need to read-- are helpful inputs
to this discussion.

The discussion is double-ended: asking the "which variables almost
always cause SMIs?" at the front and "which variables do normal,
unprivileged tools need for standard operation?" at the back.


Cheers, thanks for the feedback and consideration

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Borislav Petkov]

On Fri, Feb 16, 2018 at 10:48:32AM -0800, Joe Konno wrote:
>  We may see some other patches or RFCs about caching and/or shadowing
> variable values in efivarfs to reduce the number of direct EFI reads,
> with the goal of reducing how many SMIs are generated.

So if you do the caching scheme, the question about narrowing
permissions becomes moot...

> Any obvious EFI variables that userspace tools have come to depend on--
> those which normal, unprivileged users need to read-- are helpful inputs
> to this discussion.

... which solves the aspect of not breaking userspace nicely.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Peter Jones]

On Fri, Feb 16, 2018 at 10:48:32AM -0800, Joe Konno wrote:
> On Fri, Feb 16, 2018 at 11:18:12AM +0000, Ard Biesheuvel wrote:
> > On 16 February 2018 at 11:08, Borislav Petkov <bp@alien8.de> wrote:
> > > On Fri, Feb 16, 2018 at 10:58:47AM +0000, Ard Biesheuvel wrote:
> > >> By your own reasoning above, that's a no-no as well.
> > >
> > > I'm sure we can come up with some emulation - the same way we did the
> > > BIOS emulation.
> > >
> > >> But thanks for your input. Anyone else got something constructive to contribute?
> > >
> > > The not-breaking userspace is constructive contribution. The last
> > > paragraph is my usual rant.
> > >
> > 
> > Fair enough. And I am not disagreeing with you either.
> > 
> > So question to Joe: is it well defined which variables may exhibit
> > this behavior?
> 
> For brevity's sake, "not yet." Folks-- e.g. firmware writers and
> equipment makers-- may use SMIs more (or less) than others. So, how many
> SMIs generated by the reference shell script can, and will, vary
> platform to platform. I and my colleagues are digging into this.

As a first guess: anything generated during boot is probably not an
SMI.  Everything else is probably an SMI.  In fact, I would expect that
for most systems, the entire list of things that *don't* generate an SMI
(aside from a few IBV specific variables) is all the variables in Table
10 of the UEFI spec that don't have the NV flag.

> > Given that UEFI variables are GUID scoped, would whitelisting certain
> > GUIDs (the ones userland currently relies on to be readable my
> > non-privileged users) and making everything else user-only solve this
> > problem as well?
> 
> Perhaps. I don't want us chasing white rabbits just yet, but the
> whitelist is but one approach under consideration. We may see some other
> patches or RFCs about caching and/or shadowing variable values in
> efivarfs to reduce the number of direct EFI reads, with the goal of
> reducing how many SMIs are generated.
> 
> Any obvious EFI variables that userspace tools have come to depend on--
> those which normal, unprivileged users need to read-- are helpful inputs
> to this discussion.

So, our big userland consumers are efibootmgr, fwupdate, and
"systemctl reboot --firmware-setup".  efibootmgr and fwupdate can do the
"show the current status" half of their job as a user right now, but
they rely on root for the other half anyway.  I don't think we normally
use libfwup as non-root even for reading status.  So basically, the use
case from userland that this will effect looks like:

efibootmgr -v
*scratch head*
sudo su -
efibootmgr -b 0000 -B
efibootmgr -b 0000 -c -L "fixed entry" ...
exit

I don't feel really bad about people having to move the third line up to
the top of that.  It's also not a use case you can have very much of: it
means you manually booted without any valid boot entries.  fallback.efi
would have created a valid boot entry if you didn't do it manually.

"systemctl reboot --firmware-setup" effectively runs as root in all
cases.

The only thing we really must ensure to preserve the other workflows
is making sure creat() and open() with 0644 doesn't return an error (it
obviously won't be preserved across a reboot), because that would break
the existing tools.  But I don't see anything in this patchset which
will cause that.

tl;dr: I think changing everything to 0600 is probably completely fine,
and whitelisting is probably pointless.  
-- 
  Peter

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Ard Biesheuvel]

On 16 February 2018 at 19:22, Peter Jones <pjones@redhat.com> wrote:
> On Fri, Feb 16, 2018 at 10:48:32AM -0800, Joe Konno wrote:
>> On Fri, Feb 16, 2018 at 11:18:12AM +0000, Ard Biesheuvel wrote:
>> > On 16 February 2018 at 11:08, Borislav Petkov <bp@alien8.de> wrote:
>> > > On Fri, Feb 16, 2018 at 10:58:47AM +0000, Ard Biesheuvel wrote:
>> > >> By your own reasoning above, that's a no-no as well.
>> > >
>> > > I'm sure we can come up with some emulation - the same way we did the
>> > > BIOS emulation.
>> > >
>> > >> But thanks for your input. Anyone else got something constructive to contribute?
>> > >
>> > > The not-breaking userspace is constructive contribution. The last
>> > > paragraph is my usual rant.
>> > >
>> >
>> > Fair enough. And I am not disagreeing with you either.
>> >
>> > So question to Joe: is it well defined which variables may exhibit
>> > this behavior?
>>
>> For brevity's sake, "not yet." Folks-- e.g. firmware writers and
>> equipment makers-- may use SMIs more (or less) than others. So, how many
>> SMIs generated by the reference shell script can, and will, vary
>> platform to platform. I and my colleagues are digging into this.
>
> As a first guess: anything generated during boot is probably not an
> SMI.  Everything else is probably an SMI.  In fact, I would expect that
> for most systems, the entire list of things that *don't* generate an SMI
> (aside from a few IBV specific variables) is all the variables in Table
> 10 of the UEFI spec that don't have the NV flag.
>
>> > Given that UEFI variables are GUID scoped, would whitelisting certain
>> > GUIDs (the ones userland currently relies on to be readable my
>> > non-privileged users) and making everything else user-only solve this
>> > problem as well?
>>
>> Perhaps. I don't want us chasing white rabbits just yet, but the
>> whitelist is but one approach under consideration. We may see some other
>> patches or RFCs about caching and/or shadowing variable values in
>> efivarfs to reduce the number of direct EFI reads, with the goal of
>> reducing how many SMIs are generated.
>>
>> Any obvious EFI variables that userspace tools have come to depend on--
>> those which normal, unprivileged users need to read-- are helpful inputs
>> to this discussion.
>
> So, our big userland consumers are efibootmgr, fwupdate, and
> "systemctl reboot --firmware-setup".  efibootmgr and fwupdate can do the
> "show the current status" half of their job as a user right now, but
> they rely on root for the other half anyway.  I don't think we normally
> use libfwup as non-root even for reading status.  So basically, the use
> case from userland that this will effect looks like:
>
> efibootmgr -v
> *scratch head*
> sudo su -
> efibootmgr -b 0000 -B
> efibootmgr -b 0000 -c -L "fixed entry" ...
> exit
>
> I don't feel really bad about people having to move the third line up to
> the top of that.  It's also not a use case you can have very much of: it
> means you manually booted without any valid boot entries.  fallback.efi
> would have created a valid boot entry if you didn't do it manually.
>
> "systemctl reboot --firmware-setup" effectively runs as root in all
> cases.
>
> The only thing we really must ensure to preserve the other workflows
> is making sure creat() and open() with 0644 doesn't return an error (it
> obviously won't be preserved across a reboot), because that would break
> the existing tools.  But I don't see anything in this patchset which
> will cause that.
>
> tl;dr: I think changing everything to 0600 is probably completely fine,
> and whitelisting is probably pointless.

This is why I was leaning towards applying these patches: not breaking
userland is an important rule, but it does not imply every aspect of
behavior observable by userland is set in stone. In other words, I
agree with Peter that making this change does not *break* userland in
a way anyone is likely to care deeply about.

Adding a caching layer to efivarfs for this purpose is really overkill
IMHO. Also, we need this only on x86, so I'd like to see it proposed
in a way that allows it to be compiled out for architectures that have
no need for it.

RE: [PATCH 0/2] efivars: reading variables can generate SMIs

[Luck, Tony]

> tl;dr: I think changing everything to 0600 is probably completely fine,
> and whitelisting is probably pointless.  

But do you speak for all users? It will just take one person complaining
that efibootmgr no longer shows them what it used to show to bring down
the wrath of Linus on our (specifically Joe's) head for breaking user space.

I've got someone about to start looking at making efivarfs read and save
the values during mount, so all the read-only options can continue to work
without making EFI calls.

This will cost some memory (say 20-30 variables at up to 1K each).

-Tony

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Matthew Garrett]

On Fri, Feb 16, 2018 at 11:31 AM Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
wrote:
> This is why I was leaning towards applying these patches: not breaking
> userland is an important rule, but it does not imply every aspect of
> behavior observable by userland is set in stone. In other words, I
> agree with Peter that making this change does not *break* userland in
> a way anyone is likely to care deeply about.

In some modes tpmtotp will run as non-root and expect to be able to read an
EFI variable.

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Peter Jones]

On Fri, Feb 16, 2018 at 07:32:17PM +0000, Luck, Tony wrote:
> > tl;dr: I think changing everything to 0600 is probably completely fine,
> > and whitelisting is probably pointless.  
> 
> But do you speak for all users?

No, I just write their tools :)

> It will just take one person complaining that efibootmgr no longer
> shows them what it used to show to bring down the wrath of Linus on
> our (specifically Joe's) head for breaking user space.

The userland use case is gazing idly at the values without intending to
do anything about them.  And most of this is firmware config and
firmware/OS interaction.  Honestly it should never have been user
readable to begin with.

But also, we had a bug for quite some time where efibootmgr created
everything as 0600, and as a result non-root users couldn't do e.g.
"efibootmgr -v" and see the paths of new entries until a reboot
occurred.  Nobody ever reported it in bugzilla.redhat.com or efibootmgr
or efivar's github issues pages.  One person noticed it while commenting
about another issue, but didn't see it as related to his actual issue or
being a bug so much as "weird" that listing worked as non-root before
changing something but not after.

Another user asked about getting permission denied while setting the
boot order on askubuntu here:
https://askubuntu.com/questions/688317/getting-permission-denied-errors-from-efibootmgr
The response was exactly that you have to run it as root.  I think it's
telling that nobody said anything about reading vs writing.

> I've got someone about to start looking at making efivarfs read and save
> the values during mount, so all the read-only options can continue to work
> without making EFI calls.
> 
> This will cost some memory (say 20-30 variables at up to 1K each).

71 variables on my laptop, and the 1K restriction went away a *loooong*
time ago.  It was fully excised from the userland tools in 2013.  On my
laptop, 4 of those 71 variables are >5000 bytes.  The total storage of
all of the data in the variables is 38kB.

I still think changing it to 0600 and calling this done is the right
thing to do.

-- 
  Peter

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[James Bottomley]

On Fri, 2018-02-16 at 10:41 +0000, Ard Biesheuvel wrote:
> On 15 February 2018 at 18:22, Joe Konno <joe.konno-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> wrote:
> > 
> > From: Joe Konno <joe.konno-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > 
> > It was pointed out that normal, unprivileged users reading certain
> > EFI
> > variables (through efivarfs) can generate SMIs. Given these nodes
> > are created
> > with 0644 permissions, normal users could generate a lot of SMIs.
> > By
> > restricting permissions a bit (patch 1), we can make it harder for
> > normal users
> > to generate spurious SMIs.
> > 
> > A normal user could generate lots of SMIs by reading the efivarfs
> > in a trivial
> > loop:
> > 
> > ```
> > while true; do
> >     cat /sys/firmware/efi/evivars/* > /dev/null
> > done
> > ```
> > 
> > Patch 1 in this series limits read and write permissions on
> > efivarfs to the
> > owner/superuser. Group and world cannot access.
> > 
> > Patch 2 is for consistency and hygiene. If we restrict permissions
> > for either
> > efivarfs or efi/vars, the other interface should get the same
> > treatment.
> > 
> 
> I am inclined to apply this as a fix, but I will give the x86 guys a
> chance to respond as well.

It would break my current efi certificate tools because right at the
moment you can read the EFI secure boot variables as an unprivileged
user.

That said, I'm not sure how many non-root users run the toolkit to
extract their EFI certificates or check on the secure boot status of
the system, but I suspect it might be non-zero: I can see the tinfoil
hat people wanting at least to check the secure boot status when they
log in.

James

RE: [PATCH 0/2] efivars: reading variables can generate SMIs

[Luck, Tony]

> That said, I'm not sure how many non-root users run the toolkit to
> extract their EFI certificates or check on the secure boot status of
> the system, but I suspect it might be non-zero: I can see the tinfoil
> hat people wanting at least to check the secure boot status when they
> log in.

Another fix option might be to rate limit EFI calls for non-root users (on X86
since only we have the SMI problem). That would:

1) Avoid using memory to cache all the variables
2) Catch any other places where non-root users can call EFI

-Tony

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Andy Lutomirski]

On Fri, Feb 16, 2018 at 9:09 PM, Luck, Tony <tony.luck@intel.com> wrote:
>> That said, I'm not sure how many non-root users run the toolkit to
>> extract their EFI certificates or check on the secure boot status of
>> the system, but I suspect it might be non-zero: I can see the tinfoil
>> hat people wanting at least to check the secure boot status when they
>> log in.
>
> Another fix option might be to rate limit EFI calls for non-root users (on X86
> since only we have the SMI problem). That would:
>
> 1) Avoid using memory to cache all the variables
> 2) Catch any other places where non-root users can call EFI

I'm going to go out on a limb and suggest that the fact that
unprivileged users can read efi variables at all is a mistake
regardless of SMI issues.

Also, chmod() just shouldn't work on efi variables, and the mode
passed to creat() should be ignored.  After all, there's no backing
store for the mode.

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Matthew Garrett]

On Fri, Feb 16, 2018 at 1:45 PM Andy Lutomirski <luto@kernel.org> wrote:
> I'm going to go out on a limb and suggest that the fact that
> unprivileged users can read efi variables at all is a mistake
> regardless of SMI issues.

Why? They should never contain sensitive material.

> Also, chmod() just shouldn't work on efi variables, and the mode
> passed to creat() should be ignored.  After all, there's no backing
> store for the mode.

If the default is 600 then it makes sense to allow a privileged service to
selectively make certain variables world readable at runtime.

RE: [PATCH 0/2] efivars: reading variables can generate SMIs

[Luck, Tony]

> If the default is 600 then it makes sense to allow a privileged service to
> selectively make certain variables world readable at runtime.

As soon as you make one variable world readable you are vulnerable to
a local user launching a DoS attack by reading that variable over and over
generating a flood of SMIs.

-Tony

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Matthew Garrett]

On Fri, Feb 16, 2018 at 2:02 PM Luck, Tony <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

> > If the default is 600 then it makes sense to allow a privileged service
to
> > selectively make certain variables world readable at runtime.

> As soon as you make one variable world readable you are vulnerable to
> a local user launching a DoS attack by reading that variable over and over
> generating a flood of SMIs.

I'm not terribly worried about untrusted users on my laptop, but I would
prefer to run as little code as root as possible.

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Peter Jones]

On Fri, Feb 16, 2018 at 09:09:30PM +0000, Luck, Tony wrote:
> > That said, I'm not sure how many non-root users run the toolkit to
> > extract their EFI certificates or check on the secure boot status of
> > the system, but I suspect it might be non-zero: I can see the tinfoil
> > hat people wanting at least to check the secure boot status when they
> > log in.
> 
> Another fix option might be to rate limit EFI calls for non-root users (on X86
> since only we have the SMI problem). That would:
> 
> 1) Avoid using memory to cache all the variables
> 2) Catch any other places where non-root users can call EFI

I could get behind that as well.  Currently the things I maintain do
approximately this many normal accesses with invocations you can do as a
user:

"efibootmgr -v" - six files we always try to read, plus one per Boot####
		  entry.
"fwupdate --info" - one file it always tries to read, one file for each
                    ESRT entry.
"dbxtool -l" - one file it always reads.
"mokutil --sb-state" - reads the same file twice.  I don't maintain
                       this, but I'll send a patch to Gary to make it
		       only read it once.  AFAICS all of the other
		       invocations you can currently do as a user
		       /legitimately/ read two files, though.

Some systems seem to *love* making a pile of Boot#### entries; I think
the most I've seen is something like 16.  So on that machine, one
"efibootmgr -v" invocation is ~22 efivars files read.  I've never seen a
machine that advertised more than 2 ESRT entries, but maybe we'll get
there some day.

-- 
  Peter

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Ard Biesheuvel]

On 16 February 2018 at 22:05, Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, Feb 16, 2018 at 09:09:30PM +0000, Luck, Tony wrote:
>> > That said, I'm not sure how many non-root users run the toolkit to
>> > extract their EFI certificates or check on the secure boot status of
>> > the system, but I suspect it might be non-zero: I can see the tinfoil
>> > hat people wanting at least to check the secure boot status when they
>> > log in.
>>
>> Another fix option might be to rate limit EFI calls for non-root users (on X86
>> since only we have the SMI problem). That would:
>>
>> 1) Avoid using memory to cache all the variables
>> 2) Catch any other places where non-root users can call EFI
>
> I could get behind that as well.  Currently the things I maintain do
> approximately this many normal accesses with invocations you can do as a
> user:
>
> "efibootmgr -v" - six files we always try to read, plus one per Boot####
>                   entry.
> "fwupdate --info" - one file it always tries to read, one file for each
>                     ESRT entry.
> "dbxtool -l" - one file it always reads.
> "mokutil --sb-state" - reads the same file twice.  I don't maintain
>                        this, but I'll send a patch to Gary to make it
>                        only read it once.  AFAICS all of the other
>                        invocations you can currently do as a user
>                        /legitimately/ read two files, though.
>
> Some systems seem to *love* making a pile of Boot#### entries; I think
> the most I've seen is something like 16.  So on that machine, one
> "efibootmgr -v" invocation is ~22 efivars files read.  I've never seen a
> machine that advertised more than 2 ESRT entries, but maybe we'll get
> there some day.
>

Would rate limiting (but not only for non-root) help mitigate Spectre
v1 issues in UEFI runtime services code as well? I have been looking
into unmapping the entire kernel while such calls are in progress,
because firmware is likely to remain vulnerable long after the OSes
have been fixed, and we may be able to kill two birds with one stone
here (and not break userland in the process)

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Andi Kleen]

> Would rate limiting (but not only for non-root) help mitigate Spectre
> v1 issues in UEFI runtime services code as well? I have been looking
> into unmapping the entire kernel while such calls are in progress,
> because firmware is likely to remain vulnerable long after the OSes
> have been fixed, and we may be able to kill two birds with one stone
> here (and not break userland in the process)

Yes a global rate limit would seem like a good compromise.

-Andi

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Andy Lutomirski]

On Fri, Feb 16, 2018 at 10:03 PM, Matthew Garrett <mjg59-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, Feb 16, 2018 at 2:02 PM Luck, Tony <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>
>> > If the default is 600 then it makes sense to allow a privileged service
> to
>> > selectively make certain variables world readable at runtime.
>
>> As soon as you make one variable world readable you are vulnerable to
>> a local user launching a DoS attack by reading that variable over and over
>> generating a flood of SMIs.
>
> I'm not terribly worried about untrusted users on my laptop, but I would
> prefer to run as little code as root as possible.

I think that, for the most part, systemwide configuration should not
be accessible to non-root.  Unprivileged users, in general, have no
legitimate reason to know that my default boot is Boot0000* Fedora
HD(1,GPT,ee...,0x800,0x64000)/File(\EFI\fedora\shim.efi).  Even more
so if I'm network booting.

Alternatively, we could call this a distro issue.  Distros could
easily change the permissions on /sys/firmware/efi/efivars to disallow
unprivileged access.