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

[Linus Torvalds]

On Tue, Feb 20, 2018 at 5:05 PM, Luck, Tony  wrote:
>>> EFI[1] stinks. Reading any file in /sys/firmware/efi/efivars/ generates
>>> 4 (yes FOUR!) SMIs.
>
>> Is that actualkly the normal implementation?
>
> I don't know if there are other implementations. This is what I see on my
> lab system.

Ok. I'm not a huge fan of EFI. Over-designed to the hilt. Happily at
least the loadable drivers are a thing of the past.

Do we have a list of things normal users care about? Because one thing
that would solve it is caching of the values. We don't want to do that
in general, but maybe we could just do it for the subset that we think
are "user accessible".

Although maybe just that "rate limit" thing would be simplest.

I don't want to break existing users, although it's not entirely clear
to me if there are any real use cases that matter to users. If tpmtotp
is the main case, maybe that can be changed to work around it and just
cache a value or something?

So I could imagine just applying Joe's / Andy's patch to see if
anybody even notices. But if somebody does, we'd have to go to the
alternatives anyway.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Luck, Tony]

>> EFI[1] stinks. Reading any file in /sys/firmware/efi/efivars/ generates
>> 4 (yes FOUR!) SMIs.

> Is that actualkly the normal implementation?

I don't know if there are other implementations. This is what I see on my
lab system.

> Also, if these are just synchronous SMI's, then don't we just end up
> correctly assigning the CPU load to the reader, and it doesn't
> actually matter? Where's the DoS?

SMI are broadcast to all logical CPUs. The bigger the system the
more the pain from an SMI as code tries to rendezvous them all
(4 socket * 24 core * 2 HyperThreads = 192 CPUs on my system).

Looping reading files from efivarfs doesn't stop the system, but it
does slow to a crawl while it is happening.  Not a full blown DoS,
but fairly unpleasant.

-Tony

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

[Linus Torvalds]

On Tue, Feb 20, 2018 at 3:30 PM, Luck, Tony  wrote:
>
> Too much weasel there.  Should say:
>
> EFI[1] stinks. Reading any file in /sys/firmware/efi/efivars/ generates
> 4 (yes FOUR!) SMIs.

Is that actualkly the normal implementation?

Also, if these are just synchronous SMI's, then don't we just end up
correctly assigning the CPU load to the reader, and it doesn't
actually matter? Where's the DoS?

 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Peter Jones]

On Tue, Feb 20, 2018 at 02:01:51PM -0800, Linus Torvalds wrote:

> Which variables are actually used by user space tools? It sounds like
> it may be exactly those boot order things that we already have flags
> and special behavior for.

Not that many are really part of a non-root workflow.  The biggest
consumer that there's a real /user/ workflow for is Matthew's tpmtotp,
which lets you pick your own when you set it up as root and merely read
from it as a user in perpetuity.  Most workflows are of the same form as
"I run efibootmgr as a user to list things and then use sudo when I
actually need to make some changes."

> And no, I don't believe in the "SMI can trigger a DoS" argument. Those
> garbage efivars that *do* trigger SMI's should me marked and shamed,
> and maybe _they_ need to be added to the list of things that you
> should look out for. Root or not.

It's all of them except the very few that are generated during bootup.
It's worth noting, though, that EFI does not actually require this
implementation behavior in any way.  This is all about Intel's
choice to use SMI to protect the variable store.

> And just on general principlies, I don't want to see weasel-wordy
> commit messages like
> 
>  "Reading certain EFI variables trigger SMIs"
> 
> I understand *writing* them causing SMI's due to some flash protection
> scheme. What's the reading thing? And why aren't we calling that
> garbage out?

Assuming most Intel CPUs work more or less the same as Galileo in this
particular regard, what's going on is the SPI part is connected to the
North Cluster, which presents an MMIO interface to program it, and SMM
is configured so that touching those addresses triggers an SMI.  This
way they can protect the the "authenticated" variables by checking
signatures inside SMM.

So basically it's not "Reading certain variables trigger SMIs", it's
"reading any variable that's not completely fake causes SMI".  The
result is any user can not merely trigger an SMI, but really more like
they can hold it asserted.

> So even if we do need to limit reading, I want out comments (in core
> or commits) to actually explain *Why*., instead of this kind of
> non-specific fear-mongering that nobody can really say yay or nay
> about.

Yeah, makes sense.

There are other options, but they're not great either.  For example, On
most systems the total data is <100kB, so we could make a default-on
config option to just cache it all during boot by default.  Like the
options suggested so far, it has downsides, but it's not the end of the
world for most systems.

-- 
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Luck, Tony]

> read() will make two calls - one to obtain the size of the variable, the
> other to read it. It looks like cat will also trigger an fstat(), so we're
> probably also making a call for that. There's presumably some optimisation
> that could be made there if we trust the firmware not to change the size
> behind our back.

Hmmm. "ls -l" reports the size of each of the files without causing any SMIs,
so Linux has that cached someplace and "read" is being silly making a call
to get something that we already know.  Unless we think the size may be
changing asynchronously and are OK with reporting a stale value for "stat"
but want the actual value for "read".

-Tony

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

[Matthew Garrett]

On Tue, Feb 20, 2018 at 3:30 PM Luck, Tony  wrote:
> [1] I didn't dig through the Linux code to check whether we manage to
> get those four SMIs from a single EFI call, or whether we make multiple
> EFI calls to open/read/close one file. It is possible that we stink a
> bit too if we are doing more EFI calls than required.

read() will make two calls - one to obtain the size of the variable, the
other to read it. It looks like cat will also trigger an fstat(), so we're
probably also making a call for that. There's presumably some optimisation
that could be made there if we trust the firmware not to change the size
behind our back.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Luck, Tony]

On Tue, Feb 20, 2018 at 02:01:51PM -0800, Linus Torvalds wrote:
> And just on general principlies, I don't want to see weasel-wordy
> commit messages like
> 
>  "Reading certain EFI variables trigger SMIs"
> 
> I understand *writing* them causing SMI's due to some flash protection
> scheme. What's the reading thing? And why aren't we calling that
> garbage out?

Too much weasel there.  Should say:

EFI[1] stinks. Reading any file in /sys/firmware/efi/efivars/ generates
4 (yes FOUR!) SMIs.

# rdmsr 0x34
14e2
# cat /sys/firmware/efi/efivars/ConIn-8be4df61-93ca-11d2-aa0d-00e098032b8c > 
/dev/null
# rdmsr 0x34
14e6

-Tony

[1] I didn't dig through the Linux code to check whether we manage to
get those four SMIs from a single EFI call, or whether we make multiple
EFI calls to open/read/close one file. It is possible that we stink a
bit too if we are doing more EFI calls than required.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andy Lutomirski]

On Tue, Feb 20, 2018 at 7:18 PM, Andy Lutomirski  wrote:
> On 02/15/2018 10:22 AM, Joe Konno wrote:
>>
>> From: Joe Konno 
>>
>> 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 
>> Reported-by: Tony Luck 
>> Cc: Ard Biesheuvel 
>> Cc: Matthew Garrett 
>> Cc: Jeremy Kerr 
>> Cc: linux-efi@vger.kernel.org
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Joe Konno 
>
>
> The discussion in this thread has gone on too long, so:
>
> Acked-by: Andy Lutomirski 
>
> And yes, this patch will break a couple of minor usecases, but IMO those
> usecases deserve to break.

Alternatively, a patch like this (untested but straightforward) might
be a little more effective and easier to undo in userspace for anyone
who may be adversely affected:

diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 5b68e4294faa..88c7163c0ac1 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -207,7 +207,7 @@ static int efivarfs_fill_super(struct super_block
*sb, void *data, int silent)
sb->s_d_op  = _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 = _dir_inode_operations;

If you prefer that, I'd be happy to re-send it for real.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Luck, Tony]

On Tue, Feb 20, 2018 at 09:22:29PM +, Matthew Garrett wrote:
> On Tue, Feb 20, 2018 at 1:18 PM Luck, Tony  wrote:
> 
> > Does this rate an exception to the "don't break userspace" for a security
> issue?
> 
> To be clear, when you say "security" is this in reference to it being a
> denial of service, or are you worried about other interactions that may
> cause wider security issues?

The immediate problem is the denial of service attack.  I have
a nagging worry that allowing a user to cause an SMI at a precise
time might also be a problem. But I don't know how that could be
leveraged in some other attack.

Making the efivar files 0600 would stop the user from causing the
SMIs. The rate limit solution could include a random delay to make
it tricky to use any attack that relies on an SMI during some specific
code sequence.

-Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Matthew Garrett]

On Tue, Feb 20, 2018 at 1:18 PM Luck, Tony  wrote:

> Does this rate an exception to the "don't break userspace" for a security
issue?

To be clear, when you say "security" is this in reference to it being a
denial of service, or are you worried about other interactions that may
cause wider security issues?
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Luck, Tony]

On Tue, Feb 20, 2018 at 11:18:57AM -0800, Andy Lutomirski wrote:
> On 02/15/2018 10:22 AM, Joe Konno wrote:
> > From: Joe Konno 
> > 
> > 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.
...
> The discussion in this thread has gone on too long, so:
> 
> Acked-by: Andy Lutomirski 
> 
> And yes, this patch will break a couple of minor usecases, but IMO those
> usecases deserve to break.
...
> > -   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);

Linus,

Does this rate an exception to the "don't break userspace" for a security issue?

What breaks:
User can't run efibootmgr(8) to see things like BootOrder. Also
"fwupdate", "dbxtool", "mokutil", and "tpmtotp" have some modes
where ordinary users need read access to some EFI variables.

We looked at some other options.

1) When mounting efivarfs have it read all the variables and
   cache the values. Then user can read without making an EFI
   call because we just copyout the cached copy.
   Rejected as there can be a lot of variables (70 on Peter Jones
   system) and EFI dropped the 1KB per variable limit. So this
   pins a bunch of memory for a few obscure use cases.

2) Rate limit EFI calls for non-root
   This solution still has some cheer-leaders. Obviously a bit
   more code than just changing the permissions. But would also
   preemptively fix any other places where an ordinary user can
   trigger an EFI call.

-Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andy Lutomirski]

On 02/15/2018 10:22 AM, Joe Konno wrote:

From: Joe Konno 

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 
Reported-by: Tony Luck 
Cc: Ard Biesheuvel 
Cc: Matthew Garrett 
Cc: Jeremy Kerr 
Cc: linux-efi@vger.kernel.org
Cc: sta...@vger.kernel.org
Signed-off-by: Joe Konno 


The discussion in this thread has gone on too long, so:

Acked-by: Andy Lutomirski 

And yes, this patch will break a couple of minor usecases, but IMO those 
usecases deserve to break.



---
  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   = _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 = _dir_inode_operations;



--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html