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

On 24 February 2018 at 20:06, Alan Cox wrote: > On Wed, 21 Feb 2018 09:03:00 + >> The thing I like about rate limiting (for everyone including root) is >> that it does not break anybody's workflow (unless EFI variable access >> occurs on a hot path, in which case

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

On Wed, 21 Feb 2018 09:03:00 + > The thing I like about rate limiting (for everyone including root) is > that it does not break anybody's workflow (unless EFI variable access > occurs on a hot path, in which case you're either a) asking for it, or > b) the guy trying to DoS us), and that it

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

On Wed, Feb 21, 2018 at 11:47 AM, Luck, Tony wrote: > > The EFI calls are all about checking system configuration. A thing > that only a handful of users do on a very occasional basis. I don't > see much harm if my "efibootmgr -v" call is slowed down a bit (or even > a lot)

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

On Wed, Feb 21, 2018 at 11:58 AM, Luck, Tony wrote: > > How are you envisioning this rate-limiting to be implemented? Are > you going to fail an EFI call if the rate is too high? I'm thinking that > we just add a delay to each call so that we can't exceed the limit.

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

On Wed, Feb 21, 2018 at 10:21:04AM -0800, Andi Kleen wrote: > > But it should be fairly easy to just add a 'struct ratelimit_state' to > > 'struct user_struct', and then you can easily just use > > > >'>f_cred->user->ratelimit' > > > > and you're done. Make sure the initial root user has it

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

On Wed, Feb 21, 2018 at 10:21 AM, Andi Kleen wrote: > > How about uid name spaces? Someone untrusted in a container could > create a lot of uids and switch between them. Anybody who does that deserves whatever the hell they get. You can already blow out a lot of other

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

> It's not about slowing down. > > It's about "user Xyz is messing with the system and reading efi vars > all the time" resulting in "user 'torvalds' is installing a kernel, > and actually wants to read efi vars, but can't". > > if you don't make it per-user, you're just replacing one DoS attack >

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

> But it should be fairly easy to just add a 'struct ratelimit_state' to > 'struct user_struct', and then you can easily just use > >'>f_cred->user->ratelimit' > > and you're done. Make sure the initial root user has it unlimited, and > limit it to something reasonable for all other user

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

On Wed, Feb 21, 2018 at 1:03 AM, Ard Biesheuvel wrote: > > The thing I like about rate limiting (for everyone including root) is > that it does not break anybody's workflow (unless EFI variable access > occurs on a hot path, in which case you're either a) asking for it,

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

On 21 February 2018 at 02:16, Linus Torvalds wrote: > 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

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

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

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

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

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

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

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

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

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

> 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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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