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

[Ard Biesheuvel]

On 16 February 2018 at 19:22, Peter Jones <pjones@xxxxxxxxxx> 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@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.

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.