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