Re: [GIT PULL] EFI fix

[Linus Torvalds]

On Fri, Jan 11, 2019 at 6:22 AM Ard Biesheuvel
 wrote:
>
> I was hoping we could merge this patch (so we can backport it), but
> resolve the conflict by dropping the kmemleak_ignore() again [..]

Well, we'd drop the new #include line also, since it would be
pointless without the kmemleak_ignore().

End result: there would be nothing left. Better not to merge it at all.

It's easy enough to backport, and just say "done differently upstream
in commit 80424b02d42b ("efi: Reduce the amount of memblock
reservations for persistent allocations").

The stable tree doesn't require that the *same* commits be upstream,
it only requires that the fixes be upstream and Greg want a pointer
to the upstream fix just so that they know they're not fixing
something that might still be broken upstream.

See for example (just random googling)

   
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=37435f7e80ef9adc32a69013c18f135e3f434244

which shows that "fixed differently upstream" case and points to why.

  Linus

Re: [GIT PULL] EFI fix

[Linus Torvalds]

On Thu, Jan 10, 2019 at 11:46 PM Ingo Molnar  wrote:
>
> A single fix that adds an annotation to resolve a kmemleak false
> positive.

This one is apparently obviated by commit 80424b02d42b ("efi: Reduce
the amount of memblock reservations for persistent allocations")

   Linus

Re: Random crashes with i386 and efi boots

[Linus Torvalds]

On Tue, Sep 11, 2018 at 7:42 AM Joerg Roedel  wrote:
>
> So I am in favor of changing the order in efi_call_phys_epilog() too.

Yeah, the only reason the order was changed was that it seemed more
logical to use the new cr3 to load the GDT. But that was clearly
wrong.

So ack on just changing the order, and my bad for suggesting the
change in the first place.

Linus

Re: [PATCH] x86/mm: Simplify p[g4um]d_page() macros

[Linus Torvalds]

On Mon, Aug 20, 2018 at 1:37 PM Andi Kleen  wrote:
>
> From: Andi Kleen 
>
> Create a pgd_pfn() macro similar to the p[4um]d_pfn() macros and then
> use the p[g4um]d_pfn() macros in the p[g4um]d_page() macros instead of
> duplicating the code.

When doing backports, _please_ explicitly specify which commit this is
upstream too.

Also, the original upstream patch is credited to Tom Lendacky.

Or is there something I'm not seeing, and this is different from
commit fd7e315988b7 ("x86/mm: Simplify p[g4um]d_page() macros")?

   Linus

Re: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 9:30 PM, Matthew Garrett  wrote:
>
> Bear in mind that I'm talking about defaults here

Mattyhew, I really want you to look yourself in the mirror.

Those defaults are really horrible defautls for real technical reasons.

You asked me why when I questioned this, but then when I replied, you
entirely ignored it.

So let me repeat: the defaults are *horrible*. They are horrible for a
very simple reason: kernel behavior changes that depend on some subtle
boot difference are truly nasty to debug, and nasty to get coverage
for.

And this "subtle boot difference" is really bad because it's a
difference that has a particularly bad pattern: pretty much not a
single mainline kernel developer will have secure boot enabled,
exactly because it's so inconvenient for testing.

So what does that mean?

It means that the default is *actively* bad for kernel development. It
means that the people who do kernel development will not be testing
the behavior that "normal" users will actually see.

If you do not see why that is a HORRIBLY BAD THING, I don't know what to say.

Seriously. It's a nasty nasty default behavior. It's absolutely
disastrously wrong.

And then when people call you out on this bad linkage of this feature
with secure boot, you spent a *LOT* of time being dishonest about it.
Instead of answeing a simple technical question, you did just about
everything you could to avoid answering it.

You initially turned the question back into a "Why would you want to?"
rather than just answering.

Then you spent a whole lot of time coming up with completely wrong
excuses that had no actual technical reason for them.

And even now, you're trying to ignore the question, and the REASON for
the question.

See above: the default is really horrendously bad, and is just about
the *worst* default you could ever pick from a kernel development
angle.

So when a kernel developer - both me and Andy - ask you about the
reason for that HORRIBLY BAD default, then you had better stop dancing
around the issue, and be honesy.

Instead, you bring up complete red herrings:

> If a user has a configuration where you're able to verify that userspace
> has some degree of protection (eg, disk encryption keys are in the TPM and
> won't unseal if someone's switched out the kernel) then it's reasonable for
> userland (or a kernel command line option) to enable the functionality.

This line of arguyment of yours ends up STILL being complete and utter garbage.

There is not a single shred of evidence that there is some kind of
"reasonable to enable the functionality" based on completely unrelated
matters.

See above on why such stupid linkages are a bad bad idea. Absolutely
*ANY* time you make that decision silently for a user, you will just
be doing the wrong thing.

You will do the wrong thing for security, but equally importantly, you
will be doing the wrong thing just for *development* and *test
coverage*.

> What I'm afraid of is this turning into a "security" feature that ends up
> being circumvented in most scenarios where it's currently deployed - eg,
> module signatures are mostly worthless in the non-lockdown case because you
> can just grab the sig_enforce symbol address and then kexec a preamble that
> flips it back to N regardless of the kernel config.

Honestyly, all of your arguments are made up shit.

This argument, for example, is just a complete red herring.

Do you want to protect against somebody flipping "sig_enforce"? Makes
perfect sense to me.

Then WHY is that not just a config option for extra hardening?
Seriously. I'd use it. I have never *ever* felt the need to switch
"sig_enforce" off, and I always build with MODULE_SIG_FORCE and
MODULE_SIG_ALL.

Getting rid of that switch entirely for security reasons sounds just
_fine_ to me.

So you use these *stupid* things as "arguments" for why you think you
want to do something. But you're putting the cart before the horse:
you have a particular end result you want to get to, and then you make
up arguments for why you want to get there.

Seriously, go back to that coverage and testing issue. Go back to the
*fundamental* technical issue that we want kernel developers to
actually *test* the code that users are running.

*Gasp*.

Yeah, I know, it's a completely radical idea, but it's true. Having
developers test and run the code actual real humans are using is a
truly revolutionary concept in security too.

> I'm making this argument from the perspective of "What should the kernel do
> when it has no additional information".

And I'm telling you that you're ignoring the fact that you picked a
truly horrendously shitty default.

And then you spent a *lot* of time giving misleading and bad
information about why you picked that shitty default, and instead just
questioning the people who asked you an actual and really simply
technical question.

  Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the 

Re: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 6:30 PM, Justin Forbes  wrote:
>>
>> If there actually was a good explanation for the tie-in, it should
>> have been front-and-center and explained as such.
>>
> Honestly, yes, the major distros have been shipping this patch set for years
> now, and every time it comes to upstream, the same damn arguments emerge.

Well, I think it's because the explanations have been bogus.

Just look at this thread. It took closer to a hundred emails (ok, so
I'm exaggerating, but not _that_ much) until the *real* reason for the
tie-in was actually exposed.

For the first 50+ emails, the explanation was "oh, only if you do
secure boot does this make sense".

Which is still pure BULLSHIT. Of _course_ that kind of stuff raises
peoples hackles and makes people not trust the messenger - he's
clearly being evasive and there must be something else going on.

So instead of the bullshit explanations, just explain the purely
_practical_ side.

Because I find it a *lot* more convincing to hear:

   "We'd like to just enable it all the time, but it's known to break
some unusual hardware cases that we can't fix in software, and we
wanted *some* way to disable it that requires explicit and verified
user intervention to do that, and disabling secure boot is the
easiest hack we could come up with".

See? No bullshit. Just straight talk about the *actual* reason why
people decided on this particular tie-in, and admitting that it's a
hack, but also clearly stating the reason for the hack.

Now, I still don't necessarily agree that it's the best possible
option, but when stated in those terms I at least understand why that
option was picked as a reasonable one, and it changes the discussion a
lot, and (at least for me) makes it much more palatable.

Because as long as the explanation is just some "you must use secure
boot or you've already lost and further security is pointless"
hocus-pocus magical thinking, I immediately go "no, that sounds
completely bogus, and it makes testing and coverage much worse, we've
done other things quite like that without this secure boot tie-in".

 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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 6:13 PM, Matthew Garrett  wrote:
>
> There are four cases:

No.

Matthew., stop with the agenda already.

This shit is what I'm talking about:

> Verified Boot off, lockdown on: Perception of security improvement that's
> trivially circumvented (and so bad)

You're doing some serious value judgement that is simply bogus.

If lockdown actually helps avoid CPL0 execution attacks, then it helps
even if secure more is off.

Sure, you can do things like try to install kernels and reboot, but
honestly, that's not "trivially circumvented". It can be quite hard to
hide even if you don't have secure boot. Things like disk encryption
(common for a lot of people) for example means that you simply won't
be booting that machine without the user noticing.

Or think of virtual machines - which people often use on purpose for
security things. Again, they very much are _not_ going to have secure
boot, but it's not necessarily even possible to "replace the kernel
and reboot" at all, because the kernel came from outside the virtual
machine entirely, and rebooting might just kill the VM rather than
restart anything.

So I really think you're pushing this whole "not secure boot" means
"trivial circumvention" much much too hard.

To the point of it being an outright lie.

I think the kind of people who run stuff in virtual machines could
easily want to also enable lockdown measures, simply to reduce the
attack window within that VM. Wouldn't you agree? Those are often
security-conscious people.

This is what I mean by having an agenda.  We all know you are a big
proponent of secure boot. But it seems to cloud your arguments, by
turning your assumptions and your agenda into an "argument" that is
simply not even TRUE.

See what I'm unhappy about?

> Verified Boot on, lockdown off: Perception of security improvement that's
> trivially circumvented (and so bad), status quo in mainline kernels

I think this is entirely false too. Again, the "trivial circumvention"
shows a bias and agenda that isn't really all that true.

> Of these four options, only two make sense.

No.

You say that, because you have that bias and that agenda.

But that simply doesn't make it true.

Now, what actually seems to be a real and valid argument is *this* part:

> This makes it easy for a user to switch
> between the two states that make sense by running a single command and then
> following some prompts on the next reboot. The alternative would be to
> provide a signed kernel that always enabled lockdown and an unsigned kernel
> that didn't, which would (a) increase load on distributions and (b) force
> users to both run mokutil --disable-validation and also install a different
> kernel.

THAT is an actual argument. Admittedly I think it's a horrible hack,
but it's a hack that can be explained without outright lying. And it
may be a hack that is "the best we can reasonably do"

See what I'm saying?

One argument is based on your value judgments that not everybody else
believes in.

The other argument is based purely on cold hard particular facts.

Guess which argument is better for people who aren't Matthew Garrett?

That said, wouldn't it be equally good to just make the whole thing be
a protected EFI variable instead? Once you have physical access to the
EFI shell (to turn off secure boot) you have access to that too.

Which would allow the "switch off/on" case even if there are other
reasons why changing secure boot isn't a great option (possibly
because secure boot isn't an option to begin with due to being so
invonvenient).

  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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 5:25 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
>
> Honestly, I don't think the patchset is viable at all in that case.

.. or rather, it's probably viable only for distributions that already
have reasons to only care about controlled hardware environments, ie
Chromebooks etc.

But a chome OS install wouldn't care about the whole "secure boot or
not" issue anyway, because they'd also control that side, an they
might as well just enable it unconditionally.

In contrast, the generic distros can't enable it anyway if it breaks
random hardware.  And it wouldn't be about secure boot or not, but
about the random hardware choice.

 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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 5:16 PM, Matthew Garrett  wrote:
>
> I ignored it because it's not a viable option. Part of the patchset
> disables various kernel command line options. If there's a kernel command
> line option that disables the patchset then it's pointless.

Honestly, I don't think the patchset is viable at all in that case.

No way will any sane distribution take it, potentially breaking a lot
of machines, and have no way to unbreak them except for "oh, btw, you
have to disable secure boot to get things to work again".

That would be insane.

So you'd better allow some command line options.

One reasonable option may be to just disable lockdown by default (to
make machines work reliably), and then have a "if you're anal about
security, add 'lockdown' to the kernel command line".

People who care about this already need to check the secure boot
status, so this would be just one more thing they'd check.

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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 5:10 PM, Matthew Garrett  wrote:
>
>> Exactly like EVERY OTHER KERNEL CONFIG OPTION.
>
> So your argument is that we should make the user experience worse? Without
> some sort of verified boot mechanism, lockdown is just security theater.
> There's no good reason to enable it unless you have some mechanism for
> verifying that you booted something you trust.

Wow. Way to snip the rest of the email where I told you what the
solution was. Let me repeat it here, since you so conveniently missed
it and deleted it:

>> Or, like a lot of other kernel options, maybe have a way to just
>> disable it on the kernel command line, and let the user know about it.
>>
>> That would still be better than disabling secure boot entirely in your
>> world view, so it's (a) more convenient and (b) better.

Matthew, it's simply not worth continuing talking with you.

I'll just not pull this crap, and vendors that you convince to do
stupid things have only themselves to blame.

You clearly have an agenda, and are not willing to look at arguments
against your idiotic choices.

 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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 5:04 PM, Matthew Garrett  wrote:
>
> How? When there are random DMA-capable PCI devices that are driven by
> userland tools that are mmap()ing the BARs out of sysfs, how do we
> simultaneously avoid breaking those devices while also preventing the
> majority of users from being vulnerable to an attacker just DMAing over the
> kernel?

.. if that ends up being a real problem, then you print a warning and
tell people to use the kernel command line to disable things.

And if it's a big and common problem, then the answer may be that
lockdown has to be entirely OFF by default, and you instead just tell
people to enable it manually with a kernel command line option.

Still better than telling them to disable/enable secure boot, which
they may or may not even be able to to.

 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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 4:59 PM, Matthew Garrett  wrote:
>
> Ok. So we can build distribution kernels that *always* have this on, and to
> turn it off you have to disable Secure Boot and install a different kernel.

Bingo.

Exactly like EVERY OTHER KERNEL CONFIG OPTION.

Just like all the ones that I've mentioned several times.

Or, like a lot of other kernel options, maybe have a way to just
disable it on the kernel command line, and let the user know about it.

That would still be better than disabling secure boot entirely in your
world view, so it's (a) more convenient and (b) better.

Again, in no case does it make sense to tie it into "how did we boot".
Because that's just inconvenient for everybody.

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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 4:47 PM, Matthew Garrett  wrote:
>> Another way of looking at this: if lockdown is a good idea to enable
>> when you booted using secure boot, then why isn't it a good idea when
>> you *didn't* boot using secure boot?
>
> Because it's then trivial to circumvent and the restrictions aren't worth
> the benefit.

Bullshit.

If there those restrictions cause problems, they need to be fixed regardless.

In fact, from a debuggability standpoint, you want to find the
problems early, on those kernel development machines that had secure
boot explicitly turned off because it's such a pain.

And if they can't be fixed, then the user is going to disable lockdown
regardless of how he booted the machine.

In no situation is "depending on how you booted" a good choice.

Either you can enable it or you can't. If you can, good. And if you
can't, it has nothing to do with secure boot.

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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 4:56 PM, David Howells  wrote:
=>
> Most users haven't even given this a moment's thought, aren't even aware of
> the issues, don't even know to ask and, for them, it makes no difference.
> They trust their distribution to deal with stuff they don't know about.

Right.

Like perhaps trusting the distribution to just enable all those
security measures _regaredless_ of whether they booted in using secure
boot or not?

See?

If lockdown breaks something, the distro would need to fix it
regardless of secure boot.

So why is the enablement dependent on it again?

I'm not arguing "lockdown shouldn't be on".

I'm arguing "lockdown being on or off has _nothing_ to do with whether
the machine was booted in EFI mode with secure boot or not".

You don't seem to get it.

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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 4:45 PM, Matthew Garrett  wrote:
>> Be honest now. It wasn't generally users who clamored for it.
>
> If you ask a user whether they want a system that lets an attacker replace
> their kernel or one that doesn't, what do you think their answer is likely
> to be?

Goddamnit.

We both know what the answer will be.

And it will have *nothing* to do with secure boot.

So *you* be honest now.

Because you clearly aren't.

Seriously. Go ask that question to a random person:

 "Do you want a system that lets an attacker replace their kernel or
one that doesn't?"

and don't ask anything else.

Do you really think they'll answer "no, I don't want an attacker to
replace my kernel, but only if I booted with secure boot"?

Honestly, now.

> Again, what is your proposed mechanism for ensuring that off the shelf
> systems can be configured in a way that makes this possible?

If you think lockdown is a good idea, and you enabled it, then IT IS ENABLED.

No idiotic "secure boot or not" garbage.

Because secure boot or not isn't *relevant*.

Christ, we already have things like

 - CONFIG_STRICT_KERNEL_RWX

 - CONFIG_STRICT_DEVMEM

 - CONFIG_HARDENED_USERCOPY

 - CONFIG_MODULE_SIG_ALL (and friends)

and absolutely *NONE* of them depend on whether the kernel was booted
with secure boot or not.

And I claim that it would be completely idiotic and broken if they did.

And - not entirely unrelated - I claim that it is COMPLETELY IDIOTIC
AND BROKEN to make some new "lockdown" option depend on it.

Comprende?

Really. Your arguments make no sense. They are all fundamentally
broken for the simple reason that all your "but secure boot implies
XYZ" are pure and utter bullshit, because all your arguments are valid
whether secure boot happened or not.

See? Secure boot has *NOTHING* do to with anything.  It has nothing to
do with loading only signed kernel modules. It has nothing to do with
your lockdown patches.

Either lockdown is good or not. It's that simple. But the goodness has
nothing to do with secure boot.

  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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 4:26 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
>
> Magically changing kernel behavior depending on some subtle and often
> unintentional bootup behavior detail is completely idiotic.

Another way of looking at this: if lockdown is a good idea to enable
when you booted using secure boot, then why isn't it a good idea when
you *didn't* boot using secure boot?

That's the flip side of this whole argument.

People who boot without secure boot may be wanting all the same
protections. Maybe you have to disable it when you build your own
kernel, for example. Does that suddenly mean that lockdown is now a
bad idea?

And if it does, explain it. Explain why it's a bad idea to enable
without secure boot, but is a good idea to enable *with* secure boot.

In other words: explain the tie-in.

Because I really don't see it. All I see is illogical blathering that
tries to conflate issues that have nothing to do with each other.

Please explain to me why a distro or a user would want lockdown to be
disabled just because the user didn't use secure boot, but suddenly if
it's booted on another machine, it's not just a good idea, but
mandatory in your world view?

Honestly, if I were a distro maintainer, the *last* thing I'd want is
the kernel to act fundamentally differently in some security context
depending on some random bootup condition.

  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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 4:12 PM, David Howells  wrote:
>
> What use is secure boot if processes run as root can subvert your kernel?

Stop this idiocy.

The above has now been answered multiple times, several different ways.

The "point" of secure boot may be that you had no choice, or there was
no point at all, it just came that way.

Or the "point" of secure boot may be that you don't trust anybody else
than yourself, but once you've booted you do trust what you booted.

But the *real* point is that this has nothing what-so-ever to do with
secure boot. You may want (or not want) lockdown independently of it.
Don't tie magic boot issues with kernel runtime behavior.

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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 4:17 PM, Matthew Garrett  wrote:
>
> 1) Secure Boot is intended to permit the construction of a boot chain that
> only runs ring 0 code that the user considers trustworthy

No.

That may be *one* intention, for some people.

It's not an a-priori one for the actual user.

> 2) Allowing arbitrary user code to run in ring 0 without affirmative
> consent on the part of the user is therefore incompatible with the goals of
> Secure Boot

Again, that has absolutely zero relevance.

Those goals are not the *users* goals.

Be honest now. It wasn't generally users who clamored for it.

If the user actually wanted it, and is asking for it, he can enable
it. Independently of secure boot, which the user generally has little
control over.

> 3) This patchset provides a mechanism to alter the behaviour of the kernel
> such that it is significantly more difficult for arbitrary user code to run
> in ring 0 without affirmative user consent

That difficulty already exists, the new thing isn't somehow related to
that at all.

Look at our "uyou can only load modules if you're root" rules. Or the
"you can only load modules if they are signed".

See a pattern there? They don't magically enable themselves (or
disable themselves) depending on whether you booted with secure boot
or not.

Yet they are a HELL OF A LOT MORE IMPORTANT than this new patch series.

> 4) Providing a mechanism for automatically enabling this behaviour when
> running in a context that is intended to restrict access to ring 0 is a
> rational thing to do, because otherwise it is difficult to achieve the
> objective in (1)

No. See why it's *NOT* rational, as explained already several times.

Magically changing kernel behavior depending on some subtle and often
unintentional bootup behavior detail is completely idiotic.

It would be idiotic if it was that "check kernel module signatures"
check. This is no less idiotic.

Seriously, listen to your own arguments. If they don't make sense for
checking kernel module signatures, why the hell would they make sense
for something like lockdown.

THE TWO THINGS ARE ENTIRELY INDEPENDENT.

I'm done with you. You're not listening, and you're repeating bogus
arguments that make no sense.

No way in hell will I merge anything like this.

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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 4:08 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
>
> This discussion is over until you give an actual honest-to-goodness
> reason for why you tied the two features together. No more "Why not?"
> crap.

Side note: I suspect the reason is something along the lines of "there
are political reasons".

But dammit, if that's the case, those should be documented and
explained, not answered with "why not" when people ask why something
is the case.

   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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 3:51 PM, Matthew Garrett  wrote:
>
> Lockdown is clearly useful without Secure Boot (and I intend to deploy it
> that way for various things), but I still don't understand why you feel
> that the common case of booting a kernel from a boot chain that's widely
> trusted derives no benefit from it being harder to subvert that kernel into
> subverting that boot chain.

It has NOTHING TO DO WITH "HARDER TO SUBVERT".

THE TWO FEATURES HAVE NOTHING TO DO WITH EACH OTHER WHAT-SO-EVER.

I do not want my kernel to act differently depending on some really
esoteric detail in how it was booted. That is fundamentally wrong.

Is that really so hard to understand?

Look at it this way: maybe lockdown breaks some application because
that app does something odd. I get a report of that happening, and it
so happens that the reporter is running the same distro I am, so I try
it with his exact kernel configuration, and it works for me.

It is *entirely* non-obvious that the reporter happened to run a
distro kernel that had secure boot enabled, and I obviously do not.

See what the problem is? Tying these things magically together IS A BAD IDEA.

And when people ask you why you did it, YOU HAVE YET TO COME UP WITH A
SINGLE ACTUAL RESPONSE.

Instead, you just ask people why they care, or tell people to not enable it.

Seriously, Matthew, it's WRONG to tie things together in magic ways
when they have nothing what-so-ever to do with each other.

So no. The answer is simply "don't tie the two things together".

And dammit, if you tie them together, you had damn well have a good
reason. So far, your reasons have _literally_ been "Why not?" and
tried to make the onus be on others to explain to you why not.

That's not the right approach to begin with, Matthew.  The onus is on
*you* to explain why you tied them together, not on others to explain
to you - over and over - that they have nothing to do with each other.

This discussion is over until you give an actual honest-to-goodness
reason for why you tied the two features together. No more "Why not?"
crap.

 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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 3:39 PM, Andy Lutomirski  wrote:
>
> Sure.  I have no problem with having an upstream kernel have a
> lockdown feature, although I think that feature should distinguish
> between reads and writes.  But I don't think the upstream kernel
> should apply a patch that ties any of this to Secure Boot without a
> genuine technical reason why it makes sense.

So this is where I violently agree with Andy.

For example, I love signed kernel modules. The fact that I love them
has absolutely zero to do with secure boot, though. There is
absolutely no linkage between the two issues: I use (self-)signed
kernel modules simply because I think it's a good thing in general.

The same thing is true of some lockdown patch. Maybe it's a good thing
in general. But whether it's a good thing is _entirely_ independent of
any secure boot issue. I can see using secure boot without it, but I
can very much also see using lockdown without secure boot.

The two things are simply entirely orthogonal. They have _zero_
overlap. I'm not seeing why they'd be linked at all in any way.

   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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 2:08 PM, Matthew Garrett  wrote:
>
> Secure Boot ensures that the firmware will only load signed bootloaders. If
> a signed bootloader loads a kernel that's effectively an unsigned
> bootloader, there's no point in using Secure Boot

Bullshit.

I may want to know that I'm running *my* kernel, but once that is the
case, I trust it.

In fact, I tend to trust it more than some random vendor key. You should too.

Your whole argument is FUNDAMENTALLY garbage. It's the Disney kind of
garbage. It was garbage back then, and it's garbage now.

It is also garbage for a simple technical reason: secure boot can be
hard to turn off. Sometimes "turn off" means "you just have to add
your own keys".

Yes, on x86 hardware at least at some point MS actually had the rule
that it has to be something you can turn off. That rule is apparently
not true on ARM, though.

Seriously. You sound like you're parroting some party line, not like
you are answering the actual question.

So again: why do you conflate the two issues?

If you want lockdown, fine, enable it. But what the F*CK does that
have to do with whether you had secure boot or not?

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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 1:54 PM, Matthew Garrett  wrote:
>
>> .. maybe you don't *want* secure boot, but it's been pushed in your
>> face by people with an agenda?
>
> Then turn it off, or build a self-signed kernel that doesn't do this?

Umm. So you asked a question, and then when you got an answer you said
"don't do that then".

The fact is, some hardware pushes secure boot pretty hard. That has
*nothing* to do with some "lockdown" mode.

Why do you conflate the two? That was the original question. You
replied with another question. People answered yours.

NOW ANSWER THE ORIGINAL QUESTION, DAMMIT.

 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: [GIT PULL] Kernel lockdown for secure boot

[Linus Torvalds]

On Tue, Apr 3, 2018 at 9:29 AM, Matthew Garrett  wrote:
> On Tue, Apr 3, 2018 at 8:11 AM Andy Lutomirski  wrote:
>> Can you explain that much more clearly?  I'm asking why booting via
>> UEFI Secure Boot should enable lockdown, and I don't see what this has
>> to do with kexec.  And "someone blacklist[ing] your key in the
>> bootloader" sounds like a political issue, not a technical issue.
>
> A kernel that allows users arbitrary access to ring 0 is just an
> overfeatured bootloader. Why would you want secure boot in that case?

.. maybe you don't *want* secure boot, but it's been pushed in your
face by people with an agenda?

Seriously.

  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: [GIT PULL] EFI updates for v4.17

[Linus Torvalds]

On Mon, Apr 2, 2018 at 3:41 AM, Ingo Molnar  wrote:
>
>  - Use efi_switch_mm() on x86 instead of manipulating %cr3 directly

This seems to cause new warnings, both on my laptop and my desktop.
I'm surprised you don't see them.

Looks like this:

WARNING: CPU: 0 PID: 0 at arch/x86/include/asm/tlbflush.h:134
load_new_mm_cr3+0x114/0x170
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.16.0-02277-gbc16d4052f1a #1
Hardware name: System manufacturer System Product Name/Z170-K,
BIOS 3301 02/08/2017
RIP: 0010:load_new_mm_cr3+0x114/0x170
RSP: :9b203e38 EFLAGS: 00010046
RAX:  RBX: 9b26f5a0 RCX: 
RDX:  RSI:  RDI: 9b20a000
RBP: 9b203e90 R08:  R09: 0f63eb29
R10: 9b203ea8 R11: c3292018 R12: 
R13: 9b2e1180 R14: 0001ee80 R15: 
FS:  () GS:968df6c0() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 968df6fff000 CR3: 0004261e6002 CR4: 000606b0
DR0:  DR1:  DR2: 
DR3:  DR6: fffe0ff0 DR7: 0400
Call Trace:
 switch_mm_irqs_off+0x267/0x590
 switch_mm+0xe/0x20
 efi_switch_mm+0x3e/0x50
 efi_enter_virtual_mode+0x43f/0x4da
 start_kernel+0x3bf/0x458
 secondary_startup_64+0xa5/0xb0

and the reason seems to be some oddity of secondary_startup() doing
this very early, because the warning is this:

VM_WARN_ON_ONCE(!this_cpu_has(X86_FEATURE_PCID));

and obviously I *do* have X86_FEATURE_PCID on my machine, but
presumably that bit hasn't been set yet this early in the secondary
CPU boot.

The above is a pure guess from the call trace, maybe there's something
else going on.

Everything seems to *work* despite the warning, but this needs fixing.

  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 v2] efivarfs: Limit the rate for non-root to read files

[Linus Torvalds]

On Thu, Feb 22, 2018 at 9:54 AM, Luck, Tony  wrote:
> With the new "while/nap" change there would still be one message
> per second, but the number of callbacks suppressed should be 1
> (unless the user has many threads doing reads).
>
> Maybe it is good to know that an application is doing something
> stupid and we should drop that line from the patch and let the
> warnings flow?

I think the "one message per second" is fine.

Looks good. Do I get this through the EFI tree, or should I just take
it directly?

   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 v2] efivarfs: Limit the rate for non-root to read files

[Linus Torvalds]

On Thu, Feb 22, 2018 at 9:15 AM, Luck, Tony  wrote:
>
> In efivarfs if the limit is exceeded when reading, we take an
> interruptible nap for 50ms and check the rate limit again.

Ok, turning that 'if' into a 'while' makes the sleeping work even in
the presence of lots of threads, and that all looks very simple.

I'm certainly ok with this. I'm assuming this has been tested and
gives nice warnings too?

   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] efivarfs: Limit the rate for non-root to read files

[Linus Torvalds]

On Wed, Feb 21, 2018 at 5:45 PM, Luck, Tony  wrote:
>
> Linus suggested per-user rate limit to solve this.

Note that you also need to serialize per user, because otherwise..

> +   if (!__ratelimit(>f_cred->user->ratelimit))
> +   usleep_range(1, 1);

..this doesn't really ratelimit anything, because you can just start a
thousand threads, and they all end up being rate-limited, but they all
just sleep for 10ms each, so you can get a hundred thousand accesses
per second anyway.

To fix that, you can either:

 - just make it return -EAGAIN instead of sleeping (which probably
just works fine and doesn't break anything and is simple)

 - add a per-user mutex, and do the usleep inside of it, so that
anybody who tries to do a thousand threads will just be serialized by
the mutex.

Note that the mutex needs to be per-user, because otherwise it will be
a DoS for the other users.

Of course, to avoid *another* DoS, the mutex should probably be
interruptible, and return -EAGAIN, so that you don't have a thousand
thread waiting for the mutex and have something that is effectively
unkillable for ten seconds.

Can it be hard and annoying to avoid DoS by rate limiting? Why, yes.
Yes it can.

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

[Linus Torvalds]

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) because you are using a bunch of the available ratelimit reading
> the efivars.
>

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
with another one!

   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

[Linus Torvalds]

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.

Delaying sounds ok, I guess.

But the "obvious" implementation may be simple:

static void efi_ratelimit(void)
{
static DEFINE_RATELIMIT_STATE(ratelimit, HZ, 100);

if (!__ratelimit())
msleep(10);
}
}

but the above is actually completely broken.

Why? If you have multiple processes, they can each only do a hundred
per second, but globally they can do millions per second by just
having a few thousand threads. They all sleep, but..

So how do you restrict it *globally*?

If you put this all inside a lock like a mutex, you can generate
basically arbitrary delays, and you're back to the DoS schenario. A
fair lock will allow thousands of waiters to line up and make the
delay be

But maybe I'm missing some really obvious way. You *can* make the
msleep be a spinning wait instead, and rely on the scheduler, I guess.

Or maybe I'm just stupid and am overlooking the obvious case.

Again, making the ratelimiting be per-user makes all of these issues
go away. Then one user cannot delay another one.

 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

[Linus Torvalds]

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 resources that way. If you can
create users indiscriminately enough, you can bypass most other
resource limits too.

If you think containers protect against security issues from untrusted
users, I have a bridge to sell you.

  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

[Linus Torvalds]

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, or
> b) the guy trying to DoS us), and that it can make exploitation of any
> potential Spectre v1 vulnerabilities impractical at the same time.

I do agree that ratelimiting would seem to be the simple solution.

We _would_ presumably need to make it per-user, so that one user
cannot just DoS another user.

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

So I think a rate-limiting thing wouldn't be overly complicated. We
could make the rate-limiting be some completely generic thing, not
tying it to efivars itself, but just saying "this is for random
"occasional" things where we are ok with a user doing a hundred
operations per second, but if somebody tries to do millions, they get
shut down".

Realistically, even root is fine with those, but letting root in the
initial namespace be entirely unlimited is obviously a pretty
reasonable thing to do.

So it might be a few tens of lines of code or something, including the
initialization of that new user struct entry.

I think the real issue is testing and just doing it.

Anybody?

   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

[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

[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: [GIT PULL] hash addresses printed with %p

[Linus Torvalds]

On Thu, Nov 30, 2017 at 12:10 PM, Greg Kroah-Hartman
 wrote:
>
> So changing it to use __ATTR() should fix this remaning leakage up.
> That is if we even really need to export these values at all.  What does
> userspace do with them?  Shouldn't they just be in debugfs instead?

So what I find distasteful here is how sysfs has these "helper" macros
that are clearly designed to over-share.

The __ATTR macro is a lot  more complicated to use than the
__ATTR_RO/WO/RW macros, but those macros end up giving everybody read
access (ok, not the WO one)

So honestly, I think the "helper" functions should be deprecated
simply because they basically encourage people to make everything
world-readable.

Which is why most of sysfs is world-readable, whether it makes sense or not.

It would have been better had they just taken the actual mode, I suspect.

(And it would be better yet if the code didn't use that disgusting
S_IRUGO, which pretty much everybody has to think about to figure out
it's 0444)

   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: [GIT PULL] hash addresses printed with %p

[Linus Torvalds]

On Wed, Nov 29, 2017 at 1:14 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
>
> Not because %pK itself changed, but because the semantics of %p did.
> The baseline moved, and the "safe" version did not.

Btw, that baseline for me is now that I can do

  ./scripts/leaking_addresses.pl | wc -l
  18

and of those 18 hits, six are false positives (looks like bitmaps in
the uevent keys).

The remaining 12 are from the EFI runtime map files
(/sys/firmware/efi/runtime-map/*). They should presumably not be
world-readable, but sadly the kset_create_and_add() helper seems to do
that by default.

I think the sysfs code makes it insanely too easy to make things
world-readable. You try to be careful, and mark things read-only etc,
but __ATTR_RO() jkust means S_IRUGO, which means world-readable.

There seems to be no convenient model for kobjects having better
permissions. Greg?

  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: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Linus Torvalds]

On Tue, Nov 14, 2017 at 12:31 PM, Matthew Garrett  wrote:
>
>> This is all theoretical security masturbation. The _real_ attacks have
>> been elsewhere.
>
> People made the same argument about Secure Boot, and then we
> discovered that people *were* attacking the boot chain. As we secure
> other components, the attackers move elsewhere. This is an attempt to
> block off an avenue of attack before it's abused.

The thing is, if you have attested the system from boot, then you've
already attested the firmware before it even gets loaded.

And if you haven't, then you can't trust anything else anyway.

So I really don't see your point.

 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: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Linus Torvalds]

On Tue, Nov 14, 2017 at 11:58 AM, Matthew Garrett  wrote:
>
> Our ability to determine that userland hasn't been tampered with
> depends on the kernel being trustworthy. If userland can upload
> arbitrary firmware to DMA-capable devices then we can no longer trust
> the kernel. So yes, firmware is special.

You're ignoring the whole "firmware is already signed by the hardware
manufacturer and we don't even have access to it" part.

You're also ignoring the fact that we can't trust firmware _anyway_,
as Alan pointed out.

Seriously. Some of the worst security issues have been with exactly
the fact that we can't trust the hardware to begin with, where
firmware/hardware combinations are not trusted to begin with.

This is all theoretical security masturbation. The _real_ attacks have
been elsewhere.

   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: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Linus Torvalds]

On Tue, Nov 14, 2017 at 4:21 AM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> On Mon, 2017-11-13 at 14:09 -0800, Linus Torvalds wrote:
>>
>> Seriously, if you have firmware in /lib/firmware, and you don't trust
>> it, what the hell are you doing?
>
> I might "trust" the files in /lib/firmware, but I also want to make
> sure that they haven't changed.  File signatures provide file
> provenance and integrity guarantees.

Sure. But that has absolutely nothing to do with "firmware".

It is equally true of /usr/bin/* and pretty much everything in the system.

It's this insane "firmware is special" that I disagree with. It's not
special at all.

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: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Linus Torvalds]

On Mon, Nov 13, 2017 at 1:44 PM, David Howells  wrote:
>
> Whilst that may be true, we either have to check signatures on every bit of
> firmware that the appropriate driver doesn't say is meant to be signed or not
> bother.

I vote for "not bother".

Seriously, if you have firmware in /lib/firmware, and you don't trust
it, what the hell are you doing?

Oh, it's one of those "let's protect people from themselves, so that
they can't possibly break Disney^W^W be terrorists - but but the
children" things again, isn't it?

Watch me care.

  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 6/6] Documentation/x86: Update EFI memory region description

[Linus Torvalds]

On Fri, Nov 13, 2015 at 1:29 AM, Matt Fleming  wrote:
> On Fri, 13 Nov, at 10:22:10AM, Ingo Molnar wrote:
>
> You've snipped the patch hunk that gives the address range used,

I'm actually wondering if we should strive to make the UEFI stuff more
like a user process, and just map the UEFI mappings in low memory in
that magic UEFI address space.

We won't be able to run those things *as* user space, since I assume
the code will want to do a lot of kernely things, but shouldn't we aim
to make it look as much like that as possible? Maybe some day we could
even strive to run it in some controlled environment (ie user space
with fixups, virtual machine, whatever), but even if we never get
there it sounds like a potentially good idea to try to set up the
mappings to move in that direction..

No big hurry, and maybe there are good reasons not to go that way. The
first step is indeed just to get rid of the WX mappings in the normal
kernel page tables.

   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] efi: Add newline to cper DIMM location messages

[Linus Torvalds]

On Mon, Jan 27, 2014 at 10:43 AM, Luck, Tony tony.l...@intel.com wrote:
 We forgot the newline at the end of two messages. Add it.

So I don't think the patch is necessarily wrong, but does it actually matter?

The thing is, unless the next printk is a continuation (PR_CONT), the
newline should be added at that point. And if it isn't, then that's a
bug. So I'm wondering how this issue got noticed - does it actually
cause user-visible issues, or was it just code reading?

Because I'm wondering if we eventually should start thinking of that
final '\n' as pointless noise... The fact is, printk has different
semantics from printf, and maybe we should start taking advantage of
that rather than just slavishly follow tradition?

   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] efi: Add newline to cper DIMM location messages

[Linus Torvalds]

On Mon, Jan 27, 2014 at 11:48 AM, Luck, Tony tony.l...@intel.com wrote:
 The thing is, unless the next printk is a continuation (PR_CONT), the
 newline should be added at that point. And if it isn't, then that's a
 bug. So I'm wondering how this issue got noticed - does it actually
 cause user-visible issues, or was it just code reading?

 The next thing that happened was a debug printk() that didn't have
 any log level specified.

 printk(CMCI on cpu%d\n, smp_processor_id());

 and no newline was added.

Ok, I think that's a bug. Only an explicit KERN_CONT prefix should
cause a line continuation.

The new msg code seems to be terminally confused about this, and for
example, seems to completely ignore the KERN_CONT flag, and instead
tests for just LOG_PREFIX (which is set for any prefix, not the
KERN_CONT 'c' prefix).

And then it turns LOG_CONT to be about the *current* printk missing a
newline. So the whole new msg code seems to  be really confused about
what the whole KERN_CONT thing was all about, and mixed it up with
whether the message ended in a newline or not, which is entirely
bogus.

Adding more people.  Looking at the code in vprintk_emit(), somebody
is really confused.

Anyway, attached is a *totally* untested patch. Comments?

  Linus
 kernel/printk/printk.c | 43 ---
 1 file changed, 12 insertions(+), 31 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b1d255f04135..fae5c8ab607d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1561,7 +1561,9 @@ asmlinkage int vprintk_emit(int facility, int level,
level = kern_level - '0';
case 'd':   /* KERN_DEFAULT */
lflags |= LOG_PREFIX;
+   break;
case 'c':   /* KERN_CONT */
+   lflags |= LOG_CONT;
break;
}
text_len -= end_of_header - text;
@@ -1575,40 +1577,19 @@ asmlinkage int vprintk_emit(int facility, int level,
if (dict)
lflags |= LOG_PREFIX|LOG_NEWLINE;
 
-   if (!(lflags  LOG_NEWLINE)) {
-   /*
-* Flush the conflicting buffer. An earlier newline was missing,
-* or another task also prints continuation lines.
-*/
-   if (cont.len  (lflags  LOG_PREFIX || cont.owner != current))
-   cont_flush(LOG_NEWLINE);
-
-   /* buffer line if possible, otherwise store it right away */
-   if (!cont_add(facility, level, text, text_len))
-   log_store(facility, level, lflags | LOG_CONT, 0,
- dict, dictlen, text, text_len);
-   } else {
-   bool stored = false;
-
-   /*
-* If an earlier newline was missing and it was the same task,
-* either merge it with the current buffer and flush, or if
-* there was a race with interrupts (prefix == true) then just
-* flush it out and store this line separately.
-* If the preceding printk was from a different task and missed
-* a newline, flush and append the newline.
-*/
-   if (cont.len) {
-   if (cont.owner == current  !(lflags  LOG_PREFIX))
-   stored = cont_add(facility, level, text,
- text_len);
+   if (cont.len) {
+   if (cont.owner != current || !(lflags  LOG_CONT)) {
cont_flush(LOG_NEWLINE);
+   lflags = ~LOG_CONT;
}
-
-   if (!stored)
-   log_store(facility, level, lflags, 0,
- dict, dictlen, text, text_len);
}
+
+   /*
+* If LOG_CONT is set, try to add the new message to any old one.
+* If that fails - or LOG_CONT wasmn't set - create a new record.
+*/
+   if (!(lflags  LOG_CONT) || !cont_add(facility, level, text, text_len))
+   log_store(facility, level, lflags, 0, dict, dictlen, text, 
text_len);
printed_len += text_len;
 
/*

Re: [PATCH] efi: Add newline to cper DIMM location messages

[Linus Torvalds]

On Mon, Jan 27, 2014 at 2:50 PM, Joe Perches j...@perches.com wrote:
 On Mon, 2014-01-27 at 14:09 -0800, Linus Torvalds wrote

 Ok, I think that's a bug. Only an explicit KERN_CONT prefix should
 cause a line continuation.

 I don't think it's a bug.
 That's never been the case and isn't the case today.

We've certainly had different behavior over time, but we've tried to
generally go into the do the right thing direction.

See (for example)

git show v3.3:kernel/printk.c | less -N

and look at lines 885-901.

Now, that code only triggers if there is *any* prefix (and I think
that we should probably make it trigger for the no-prefix case too),
but the thing is, we seem to really have broken that concept of
KERN_CONT being special.

And we seem to have broken that not so much because we've screwed up
the KERN_CONT logic itself, but because we ended up basically saying
without any prefix, we do the old pre-linebreak default continuation
thing, so then we can make KERN_CONT empty, so now we have to do the
default continuation even if it doesn't make sense.

And _that_ in turn is a problem, because it means we missed the oh,
we should make the non-prefix case actually be the same as
KERN_DEFAULT, not KERN_CONT.

Which I think is a bit sad. Because I think the better choice (from a
internal kernel usability standpoint) would have been to say no
prefix means KERN_DEFAULT.

 In the past, if the printk buffer string prefix was
 c, the emitted string pointer was advanced by 3
 and whatever else was emitted.

Yes, but we then also did that whole let's break lines so that printk
is easier to use without getting ugly behavior etc. That happened

 A dev_level though is always effectively newline
 terminated even if it doesn't have end in an explicit
 '\n'.  pr_cont uses after a dev_level are always
 started on a new line.

But we're not talking about dev_xyz. We're talking about printk().

Now, printk() will inevitably behave a bit differently (if for no
other reason than the fact that it by definition *doesn't* have a
device, and *doesn't* have a specific log-level), and I think it's
somewhat reasonable that the dev_xyz() functions end up having the
model where they are always effectively single lines. printk() shares
much of the logic, and generally they should be *similar*, but clearly
they won't ever really be the same.

But that actually argues *more* for moving to a world where that
(common, but also often-forgotten) final '\n' at the end should be
de-emphasized. So I don't think it would be at all wrong to expect
that

printk(Line A);
printk(Line B);

should print two lines. We very much had that KERN_CONT to encode the
*exception* to this, for the (not very common) cases where we end up
listing multiple things on one line.

Now, I realize that we broke that at some point, and after having
broken it, we seem to have then said KERN_CONT doesn't do anything,
so let's make it an empty string.

But quite frankly, wouldn't it be much nicer if it was KERN_DEFAULT
that was the empty string, and KERN_CONT was the yes, we want to
explicitly continue this line one? So that the nide default behavior
would be that we'd not have these silly \n is meaningful *unless*
the next print-out happens to be a dev_xyzzy() or has an explicit
level.

 btw: today, KERN_CONT is an empty string, not 'c'.

 include/linux/kern_levels.h:#define KERN_CONT   

Yes, I noticed that when I started looking at the history of this
thing. I just think we would be better off doing it the other way
around with KERN_DEFAULT instead, and basically move away from having
'\n' be so meaningful at the end.

Hmm?

   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] efi: Add newline to cper DIMM location messages

[Linus Torvalds]

On Mon, Jan 27, 2014 at 4:44 PM, Joe Perches j...@perches.com wrote:

 There are very few uses overall of KERN_DEFAULT.

 I think KERN_DEFAULT should be removed actually.
 All the uses might be better with a specific level.

It's a long time ago, but I think one of the reasons for KERN_DEFAULT
was some of the precursor discussion to a different interface, where
people wanted to make the prefix a separate argument in order to
*force* it to be used. And that meant that you needed a separate value
for the default case, since you couldn't just skip it.

And that obviously never happened, and yes, KERN_DEFAULT is useless. I
agree that it could be removed, I'd just like the no explicit level
to behave like KERN_DEFAULT does..

 Of course if people *really* want things on the same line, they usually end
 up using sprintf() to save the individual pieces to some buffer and the dump
 that out using one printk() call

 No, actually, they don't.

 There's probably much more code that uses KERN_CONT
 or no KERN_ level where it's intended that the printks
 be merged than uses of intermediate buffers.

 There are many uses of %pV that effectively serve as a
 mechanism to avoid using multiple printks though.

KERN_CONT is kind of hacky, but the thing is, it's very convenient.
And it's often used when it's important that things be reasonably
dense (in order to not make useless small details end up as lots of
useless separate lines), but for things that aren't really
*important*.

Sometimes people could generate single strings, but KERN_CONT is all
about convenience for those print-outs. And generating a bigger string
and then printing that all out in one go is often non-productive, when
you very much want to get incremental output (some of the really
traditional cases are for things like testing where you print the test
name and then ok afterwards, where you need to know that you're
testing even if the ok never happens - or _especially_ if the ok never
happens).

It's not a good interface, no. But it's a very simple and convenient
one for simple lists of supported features etc, and it actually does
have some cases where it would be actively *wrong* to try to do it the
right way with fancy preformatting.

 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: [regression, bisected] x86: efi: Pass boot services variable info to runtime code

[Linus Torvalds]

On Fri, 31 May 2013, Matthew Garrett wrote:
 
 I agree that a revert is probably the right thing to do here, but the 
 original patch was there to permit a more accurate calculation of the 
 amount of nvram in use, not to provide additional debug information. 
 Reverting it is going to differently break a different set of systems

So differently break doesn't matter, if it's old breakage, and people 
thus don't really expect it to work. We need to fix bugs without *new* 
breakage, and quite frankly, I have been distressed by hearing the EFI 
specifications mentioned so many times in this thread.

Firmware specs are pure and utter garbage. They are irrelevant. Firmware 
is buggy, and will always be buggy. The spec doesn't matter. We should 
use firmware for loading the kernel, and as little else as humanly 
possible.

I'm very disappointed in how the EFI code doesn't seem to understand that. 
There's tons of these stupid EFI variable crap that simply shouldn't 
matter. Quite frankly, we'd be better off ignoring as much of it by 
default as at all possible. Exactly because the more of an EFI interface 
we have, the more we open us up to th einevitable firmware bugs.

Anyway, I'm traveling with absolutely horrendous internet access, so can 
somebody please send a description of the revert with the relevant 
information, because I literally have a hard time extracting it all from 
this thread because my email access is so slow and flaky... Make it easy 
for me to do the revert with a good explanation message, please,

   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: [GIT PULL] EFI urgent fix

[Linus Torvalds]

On Fri, Apr 26, 2013 at 9:30 AM, Matt Fleming m...@console-pimps.org wrote:
 Hi Peter, Linus,

 I've got a small patch that fixes a crash for the Google folks and their
 EFI SMI driver, which was caused by dereferencing a garbage pointer.

Hmm. I already took this from the earlier email you sent. Did it change since?

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