[PATCH] x86/mm: Unbreak modules that rely on external PAGE_KERNEL availability

[PATCH] x86/mm: Unbreak modules that rely on external PAGE_KERNEL availability

[Jiri Kosina]

From: Jiri Kosina <jkosina@suse.cz>

Commit

  7744ccdbc16f0 ("x86/mm: Add Secure Memory Encryption (SME) support")

as a side-effect made PAGE_KERNEL all of a sudden unavailable to modules 
which can't make use of EXPORT_SYMBOL_GPL() symbols.

This is because once SME is enabled, sme_me_mask (which is introduced as 
EXPORT_SYMBOL_GPL) makes its way to PAGE_KERNEL through _PAGE_ENC, causing 
imminent build failure for all the modules which make use of all the 
EXPORT-SYMBOL()-exported API (such as vmap(), __vmalloc(), 
remap_pfn_range(), ...).

Exporting (as EXPORT_SYMBOL()) interfaces (and having done so for ages) 
that take pgprot_t argument, while making it impossible to -- all of a 
sudden -- pass PAGE_KERNEL to it, feels rather incosistent.

Restore the original behavior and make it possible to pass PAGE_KERNEL to 
all its EXPORT_SYMBOL() consumers.

Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 arch/x86/mm/mem_encrypt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 16c5f37933a2..0286327e65fa 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -40,7 +40,7 @@ static char sme_cmdline_off[] __initdata = "off";
  * section is later cleared.
  */
 u64 sme_me_mask __section(.data) = 0;
-EXPORT_SYMBOL_GPL(sme_me_mask);
+EXPORT_SYMBOL(sme_me_mask);
 
 /* Buffer used for early in-place encryption by BSP, no locking needed */
 static char sme_early_buffer[PAGE_SIZE] __aligned(PAGE_SIZE);

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/mm: Unbreak modules that rely on external PAGE_KERNEL availability

[Thomas Gleixner]

On Wed, 8 Nov 2017, Jiri Kosina wrote:

> From: Jiri Kosina <jkosina@suse.cz>
> 
> Commit
> 
>   7744ccdbc16f0 ("x86/mm: Add Secure Memory Encryption (SME) support")
> 
> as a side-effect made PAGE_KERNEL all of a sudden unavailable to modules 
> which can't make use of EXPORT_SYMBOL_GPL() symbols.
> 
> This is because once SME is enabled, sme_me_mask (which is introduced as 
> EXPORT_SYMBOL_GPL) makes its way to PAGE_KERNEL through _PAGE_ENC, causing 
> imminent build failure for all the modules which make use of all the 
> EXPORT-SYMBOL()-exported API (such as vmap(), __vmalloc(), 
> remap_pfn_range(), ...).
> 
> Exporting (as EXPORT_SYMBOL()) interfaces (and having done so for ages) 
> that take pgprot_t argument, while making it impossible to -- all of a 
> sudden -- pass PAGE_KERNEL to it, feels rather incosistent.
> 
> Restore the original behavior and make it possible to pass PAGE_KERNEL to 
> all its EXPORT_SYMBOL() consumers.

To be honest, I fundamentaly hate this, because proprietary crap out there
more or less holds the kernel hostage in its decisions of marking new
functionality GPL only. You have already a choice by disabling SME, but
sure you want to get everything: new features and proprietary stuff.

I fear, that I can't prevent this from being applied, but whoever picks up
that patch, please add:

Despised-by: Thomas Gleixner <tglx@linutronix.de>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/mm: Unbreak modules that rely on external PAGE_KERNEL availability

[Linus Torvalds]

On Wed, Nov 8, 2017 at 12:47 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Despised-by: Thomas Gleixner <tglx@linutronix.de>

So I despise the patch for a different reason: I hate it when we
export data symbols like this.

But that's independent of the GPL-vs-not issue, and it's not like this
is the first time it happens.

Generally we should export _functionality_, not data.

The fact that we need to export access to some random global data that
we then use in an inline function or macro to me says that the code
was not designed right to begin with.

But yeah, I guess we can't fix that easily as-is, and people who just
randomly slap _GPL() on the export should stop doing that. It's *not*
a default thing, quite the reverse. It should be something that is so
core - but also so _meaningful_ - that using it is a big flag that
you're not just a random driver or something.

So slapping _GPL on some random piece of data that that doesn't
actually imply anything at all for copyright derivation is
fundamentally broken and stupid.

               Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/mm: Unbreak modules that rely on external PAGE_KERNEL availability

[Borislav Petkov]

On Wed, Nov 08, 2017 at 01:09:29PM -0800, Linus Torvalds wrote:
> Generally we should export _functionality_, not data.

Right, AFAIRC, the main reason for this being an export was because if
we hid it in a function, you'd have all those function calls as part of
the _PAGE_* macros and that's just crap.

And then having an accessor too for the mask which we export anyway is
kinda silly.

But maybe there's a different, nicer solution which we missed...

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix ImendA?rffer, Jane Smithard, Graham Norton, HRB 21284 (AG NA 1/4 rnberg)
-- 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/mm: Unbreak modules that rely on external PAGE_KERNEL availability

[Linus Torvalds]

On Wed, Nov 8, 2017 at 1:15 PM, Borislav Petkov <bp@suse.de> wrote:
>
> Right, AFAIRC, the main reason for this being an export was because if
> we hid it in a function, you'd have all those function calls as part of
> the _PAGE_* macros and that's just crap.

Yes, that would be worse.

I was thinking that maybe we could have a fixed "encrypt" bit in our
PTE, and then replace that "software bit" with whatever the real
hardware mask is (if any).

Because it's nasty to have these constants that _used_ to be
constants, and still _look_ like constants, suddely do stupid memory
reads from random kernel data.

So _this_ is the underflying problem:

  #define _PAGE_ENC  (_AT(pteval_t, sme_me_mask))

because that is simply not how the _PAGE_xyz macros should work!

So it should have been a fixed bit to begin with, and the dynamic part
should have been elsewhere.

The whole EXPORT_SYMBOL() thing is just a symptom of that fundamental
error. Modules - GPL or not - should _never_ have to know or care
about this _PAGE_ENC bit madness, simply because it shouldn't have
been there.

               Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/mm: Unbreak modules that rely on external PAGE_KERNEL availability

[Tom Lendacky]

On 11/8/2017 3:23 PM, Linus Torvalds wrote:
> On Wed, Nov 8, 2017 at 1:15 PM, Borislav Petkov <bp@suse.de> wrote:
>>
>> Right, AFAIRC, the main reason for this being an export was because if
>> we hid it in a function, you'd have all those function calls as part of
>> the _PAGE_* macros and that's just crap.
> 
> Yes, that would be worse.
> 
> I was thinking that maybe we could have a fixed "encrypt" bit in our
> PTE, and then replace that "software bit" with whatever the real
> hardware mask is (if any).
> 
> Because it's nasty to have these constants that _used_ to be
> constants, and still _look_ like constants, suddely do stupid memory
> reads from random kernel data.
> 
> So _this_ is the underflying problem:
> 
>    #define _PAGE_ENC  (_AT(pteval_t, sme_me_mask))
> 
> because that is simply not how the _PAGE_xyz macros should work!
> 
> So it should have been a fixed bit to begin with, and the dynamic part
> should have been elsewhere.
> 
> The whole EXPORT_SYMBOL() thing is just a symptom of that fundamental
> error. Modules - GPL or not - should _never_ have to know or care
> about this _PAGE_ENC bit madness, simply because it shouldn't have
> been there.

I'll look into that and see what I can come up with.

Thanks,
Tom

> 
>                 Linus
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/mm: Unbreak modules that rely on external PAGE_KERNEL availability

[Borislav Petkov]

On Wed, Nov 08, 2017 at 01:23:37PM -0800, Linus Torvalds wrote:
> I was thinking that maybe we could have a fixed "encrypt" bit in our
> PTE, and then replace that "software bit" with whatever the real
> hardware mask is (if any).

Right, I don't think that should be hard, unless I'm missing anything.
We read that bit from CPUID and that's bit 47 of the physical address
right now.

Do you think we could reuse one of those _PAGE_BIT_SOFTW*?

Right, and then set the proper *hardware* bit everytime we set a
pteval_t.

> Because it's nasty to have these constants that _used_ to be
> constants, and still _look_ like constants, suddely do stupid memory
> reads from random kernel data.
> 
> So _this_ is the underflying problem:
> 
>   #define _PAGE_ENC  (_AT(pteval_t, sme_me_mask))
> 
> because that is simply not how the _PAGE_xyz macros should work!

Yeah, I still have a funny feeling when looking at that but modulo
better solutions... :-\

> So it should have been a fixed bit to begin with, and the dynamic part
> should have been elsewhere.

Right, Tom, whaddya think? Do you see any issues with doing a software,
"mirror" bit of sorts and then converting to the C-bit when needed?

> The whole EXPORT_SYMBOL() thing is just a symptom of that fundamental
> error. Modules - GPL or not - should _never_ have to know or care
> about this _PAGE_ENC bit madness, simply because it shouldn't have
> been there.

Right, so every user of the PAGE_* macros needs to set the C-bit when
SME is enabled and everytime it creates a PTE so that the memory
controller knows how to do the access. I certainly like your idea but
we'd have to audit all the places where we need to convert to the C-bit
from the software encryption bit and how ugly that would get.

Btw, this is the other reason why the _PAGE_ENC bit is in the PAGE_*
macros: for full encryption, everything that deals with PTEs needs to
set the C-bit.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix ImendA?rffer, Jane Smithard, Graham Norton, HRB 21284 (AG NA 1/4 rnberg)
-- 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/mm: Unbreak modules that rely on external PAGE_KERNEL availability

[Tom Lendacky]

On 11/8/2017 3:45 PM, Borislav Petkov wrote:
> On Wed, Nov 08, 2017 at 01:23:37PM -0800, Linus Torvalds wrote:
>> I was thinking that maybe we could have a fixed "encrypt" bit in our
>> PTE, and then replace that "software bit" with whatever the real
>> hardware mask is (if any).
> 
> Right, I don't think that should be hard, unless I'm missing anything.
> We read that bit from CPUID and that's bit 47 of the physical address
> right now.
> 
> Do you think we could reuse one of those _PAGE_BIT_SOFTW*?
> 
> Right, and then set the proper *hardware* bit everytime we set a
> pteval_t.
> 
>> Because it's nasty to have these constants that _used_ to be
>> constants, and still _look_ like constants, suddely do stupid memory
>> reads from random kernel data.
>>
>> So _this_ is the underflying problem:
>>
>>    #define _PAGE_ENC  (_AT(pteval_t, sme_me_mask))
>>
>> because that is simply not how the _PAGE_xyz macros should work!
> 
> Yeah, I still have a funny feeling when looking at that but modulo
> better solutions... :-\
> 
>> So it should have been a fixed bit to begin with, and the dynamic part
>> should have been elsewhere.
> 
> Right, Tom, whaddya think? Do you see any issues with doing a software,
> "mirror" bit of sorts and then converting to the C-bit when needed?

I think that should be doable. It will take some investigation to see what
bit can be safely used and to find all of the areas where we would have to
translate the software bit to the hardware bit (as you mention below).

Thanks,
Tom

> 
>> The whole EXPORT_SYMBOL() thing is just a symptom of that fundamental
>> error. Modules - GPL or not - should _never_ have to know or care
>> about this _PAGE_ENC bit madness, simply because it shouldn't have
>> been there.
> 
> Right, so every user of the PAGE_* macros needs to set the C-bit when
> SME is enabled and everytime it creates a PTE so that the memory
> controller knows how to do the access. I certainly like your idea but
> we'd have to audit all the places where we need to convert to the C-bit
> from the software encryption bit and how ugly that would get.
> 
> Btw, this is the other reason why the _PAGE_ENC bit is in the PAGE_*
> macros: for full encryption, everything that deals with PTEs needs to
> set the C-bit.
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86/mm: Unbreak modules that rely on external PAGE_KERNEL availability

[Linus Torvalds]

On Wed, Nov 8, 2017 at 1:23 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So _this_ is the underlying problem:
>
>   #define _PAGE_ENC  (_AT(pteval_t, sme_me_mask))
>
> because that is simply not how the _PAGE_xyz macros should work!
>
> So it should have been a fixed bit to begin with, and the dynamic part
> should have been elsewhere.

Hmm. It's not an entirely new problem. We have that
"cachemode2protval()" thing, which causes he exact same thing, except
it accesses the __cachemode2pte_tbl[] array instead.

Which we also EXPORT_SYMBOL().

So I guess _PAGE_ENC isn't any worse than what we already had.

Of course, that at least doesn't trigger for the simple cases - only
_PAGE_CACHE_WP and _PAGE_NOCACHE end up triggering that
"cachemode2protval()" case.

I do wonder if we could perhaps at least try to unify these things a
bit, and export just one thing.

And maybe avoid accessing two completely different memory locasions
every time we use _PAGE_KERNEL or whatever.

But it all looks rather nasty, so for 4.14 clearly I should just apply
that trivial one-liner patch for now.

           Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>