[PATCH v2 0/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[PATCH v2 0/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[Jo Van Bulck]

Hi,

This is the third iteration of a patch to improve the cmdline option parsing
for PTI. This reverts largely back to the first iteration and cleans up the
code to remove any attempts at backwards compatible behavior for clearly
conflicting options when users erroneously combine pti= nopti and
mitigations=off as per Dave's suggestions [1].

[1] https://lore.kernel.org/all/b9bbb279-fa8f-0784-900f-114ce186cbb3@intel.com/

Behavior
--------

For reference, behavior with this patch is now as follows in case of any
conflicting options.

1. Latest in order of nopti pti= takes priority:

KERNEL_CMDLINE="nopti pti=on"
[    0.021779] Kernel/User page tables isolation: enabled
Mitigation: PTI

KERNEL_CMDLINE="pti=on nopti"
[    0.010289] Kernel/User page tables isolation: disabled on command line.
Vulnerable

2. Passing mitigations=off will unconditionally disable PTI:

KERNEL_CMDLINE="mitigations=off pti=on"
[    0.008331] Kernel/User page tables isolation: disabled on command line.
Vulnerable

KERNEL_CMDLINE="pti=on mitigations=off"
[    0.008495] Kernel/User page tables isolation: disabled on command line.
Vulnerable

Changelog
---------

v3
  - Revert backwards compatibility ugliness for conflicting options (Dave)

v2
  - Split pti=off and mitigations=off checks (Sohil)
  - Ensure backwards compatibility for conflicting options (Sohil)

Best,
Jo

Jo Van Bulck (1):
  x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

 arch/x86/mm/pti.c | 55 ++++++++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

-- 
2.25.1

[PATCH 1/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[Jo Van Bulck]

Parse the pti= and nopti cmdline options using early_param to fix 'Unknown
kernel command line parameters "nopti", will be passed to user space'
warnings in the kernel log when nopti or pti= are passed to the kernel
cmdline on x86 platforms. Additionally allow the kernel to warn for
malformed pti= options.

Link: https://lore.kernel.org/all/b9bbb279-fa8f-0784-900f-114ce186cbb3@intel.com/
Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 arch/x86/mm/pti.c | 55 ++++++++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 78414c6d1..7575e224d 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -69,6 +69,7 @@ static void __init pti_print_if_secure(const char *reason)
 		pr_info("%s\n", reason);
 }
 
+/* Assume mode is auto unless overridden via cmdline below. */
 static enum pti_mode {
 	PTI_AUTO = 0,
 	PTI_FORCE_OFF,
@@ -77,50 +78,50 @@ static enum pti_mode {
 
 void __init pti_check_boottime_disable(void)
 {
-	char arg[5];
-	int ret;
-
-	/* Assume mode is auto unless overridden. */
-	pti_mode = PTI_AUTO;
-
 	if (hypervisor_is_type(X86_HYPER_XEN_PV)) {
 		pti_mode = PTI_FORCE_OFF;
 		pti_print_if_insecure("disabled on XEN PV.");
 		return;
 	}
 
-	ret = cmdline_find_option(boot_command_line, "pti", arg, sizeof(arg));
-	if (ret > 0)  {
-		if (ret == 3 && !strncmp(arg, "off", 3)) {
-			pti_mode = PTI_FORCE_OFF;
-			pti_print_if_insecure("disabled on command line.");
-			return;
-		}
-		if (ret == 2 && !strncmp(arg, "on", 2)) {
-			pti_mode = PTI_FORCE_ON;
-			pti_print_if_secure("force enabled on command line.");
-			goto enable;
-		}
-		if (ret == 4 && !strncmp(arg, "auto", 4)) {
-			pti_mode = PTI_AUTO;
-			goto autosel;
-		}
-	}
-
-	if (cmdline_find_option_bool(boot_command_line, "nopti") ||
-	    cpu_mitigations_off()) {
+	if (cpu_mitigations_off())
 		pti_mode = PTI_FORCE_OFF;
+	if (pti_mode == PTI_FORCE_OFF) {
 		pti_print_if_insecure("disabled on command line.");
 		return;
 	}
+	if (pti_mode == PTI_FORCE_ON) {
+		pti_print_if_secure("force enabled on command line.");
+		goto enable;
+	}
 
-autosel:
 	if (!boot_cpu_has_bug(X86_BUG_CPU_MELTDOWN))
 		return;
 enable:
 	setup_force_cpu_cap(X86_FEATURE_PTI);
 }
 
+static int __init pti_parse_cmdline(char *arg)
+{
+	if (!strcmp(arg, "off"))
+		pti_mode = PTI_FORCE_OFF;
+	else if (!strcmp(arg, "on"))
+		pti_mode = PTI_FORCE_ON;
+	else if (!strcmp(arg, "auto"))
+		pti_mode = PTI_AUTO;
+	else
+		return -EINVAL;
+	return 0;
+}
+early_param("pti", pti_parse_cmdline);
+
+static int __init pti_parse_cmdline_nopti(char *arg)
+{
+	pti_mode = PTI_FORCE_OFF;
+	return 0;
+}
+early_param("nopti", pti_parse_cmdline_nopti);
+
 pgd_t __pti_set_user_pgtbl(pgd_t *pgdp, pgd_t pgd)
 {
 	/*
-- 
2.25.1

Re: [PATCH v2 0/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[Sohil Mehta]

Hi Jo,

On 8/12/2023 8:54 AM, Jo Van Bulck wrote:
> Hi,
> 
> This is the third iteration of a patch to improve the cmdline option parsing
> for PTI.

You missed updating the version number in the cover letter subject.
Also, it is useful to have the version number in the individual patch
subject as well. (Just something to keep in mind for future patches.)

Usually git takes care of it automatically, if you do this:

	git format-patch --cover-letter -v3 -1 -o patches/

Sohil

Re: [PATCH 1/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[Sohil Mehta]

On 8/12/2023 8:54 AM, Jo Van Bulck wrote:
>  arch/x86/mm/pti.c | 55 ++++++++++++++++++++++++-----------------------
>  1 file changed, 28 insertions(+), 27 deletions(-)
> 

This version is very similar to the original patch and much simpler.
Sorry about the unnecessary churn.

Apart from the minor nits below,
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>


> +	if (cpu_mitigations_off())
>  		pti_mode = PTI_FORCE_OFF;
> +	if (pti_mode == PTI_FORCE_OFF) {
>  		pti_print_if_insecure("disabled on command line.");
>  		return;
>  	}

A new line here would be useful.

> +	if (pti_mode == PTI_FORCE_ON) {
> +		pti_print_if_secure("force enabled on command line.");
> +		goto enable;
> +	}
>  
> -autosel:
>  	if (!boot_cpu_has_bug(X86_BUG_CPU_MELTDOWN))
>  		return;
>  enable:
>  	setup_force_cpu_cap(X86_FEATURE_PTI);
>  }
>  

Was there an issue with the flow you had in the original patch? It was
avoiding the goto label and flow was a bit more linear.

> if (pti_mode == PTI_AUTO && !boot_cpu_has_bug(X86_BUG_CPU_MELTDOWN))
>  		return;
> 
> if (pti_mode == PTI_FORCE_ON)
> 	pti_print_if_secure("force enabled on command line.");
> 
> setup_force_cpu_cap(X86_FEATURE_PTI);

Sohil

Re: [PATCH 1/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[Jo Van Bulck]

On 14.08.23 14:12, Sohil Mehta wrote:
> On 8/12/2023 8:54 AM, Jo Van Bulck wrote:
>>   arch/x86/mm/pti.c | 55 ++++++++++++++++++++++++-----------------------
>>   1 file changed, 28 insertions(+), 27 deletions(-)
>>
> 
> This version is very similar to the original patch and much simpler.
> Sorry about the unnecessary churn.
> 
> Apart from the minor nits below,
> Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>

No problem, thanks for the help!

> 
>> +	if (cpu_mitigations_off())
>>   		pti_mode = PTI_FORCE_OFF;
>> +	if (pti_mode == PTI_FORCE_OFF) {
>>   		pti_print_if_insecure("disabled on command line.");
>>   		return;
>>   	}
> 
> A new line here would be useful.

Added in next revision.

> Was there an issue with the flow you had in the original patch? It was
> avoiding the goto label and flow was a bit more linear.

No, the original flow also works and I agree that an explicit PTI_AUTO 
check may indeed be preferable. Reverting this in the next patch iteration.

Best,
Jo

Re: [PATCH v2 0/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[Jo Van Bulck]

On 14.08.23 13:43, Sohil Mehta wrote:
> You missed updating the version number in the cover letter subject.
> Also, it is useful to have the version number in the individual patch
> subject as well. (Just something to keep in mind for future patches.)
> 
> Usually git takes care of it automatically, if you do this:
> 
> 	git format-patch --cover-letter -v3 -1 -o patches/

Thank you for catching this and pointing out the git option, this is 
indeed very helpful to keep in mind!

Best,
Jo

Re: [PATCH 1/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[Jo Van Bulck]

On 11.08.23 17:08, Dave Hansen wrote:
> On 8/11/23 16:58, Sohil Mehta wrote:
>> I agree this doesn't have to be this complex. PTI_FORCE_AUTO is unnecessary.

> It's worth *ZERO* hassle.  The docs say:
> That's 100% unambiguous.
> 
> If you do "mitigations=off pti=auto", you might as well have done
> "pti=auto nopti" which is nonsense.
> 
> The kernel shouldn't fall over and die, but the user gets to hold the
> (undefined) pieces at this point.
> 
> Please let's not make this more complicated than it has to be.

Thank you both for the suggestions. I agree the code got overly complex 
and unnecessary when users are clearly passing conflicting options. So I 
prepared another patch iteration to largely revert back to the original 
proposed patch, i.e., *without* backwards compatible behavior when pti= 
nopti and mitigations=off are erroneously combined.

I'll post the new patch shortly.

Best,
Jo

Re: [PATCH 1/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[Sohil Mehta]

On 8/11/2023 5:08 PM, Dave Hansen wrote:
> It's worth *ZERO* hassle.  The docs say:
> 
>>         mitigations=
> ...
>>                         off
>>                                 Disable all optional CPU mitigations.  This
>>                                 improves system performance, but it may also
>>                                 expose users to several CPU vulnerabilities.
>>                                 Equivalent to: 
> ...
>>                                                nopti [X86,PPC]
> 
> That's 100% unambiguous.
> 

Ah! I missed that. Sorry about the trouble.

Re: [PATCH 1/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[Dave Hansen]

On 8/11/23 16:58, Sohil Mehta wrote:
> On 8/11/2023 4:42 PM, Dave Hansen wrote:
>> On 8/11/23 16:27, Jo Van Bulck wrote:
>>> Not sure which option would best match kernel coding guidelines?
>> This sound like it's getting a bit out of hand and reaching far beyond
>> cleaning up some (mostly) harmless warnings.
>>
> I agree this doesn't have to be this complex. PTI_FORCE_AUTO is unnecessary.
> 
>> pti=auto does *not* need to override mitigations=off.
> I think only pti=on needs to override mitigations=off i.e. the User is
> saying turn off mitigations but keep PTI enabled. This should be fairly
> easy to achieve with the current enum. If it is not then it's not worth
> the hassle.

It's worth *ZERO* hassle.  The docs say:

>         mitigations=
...
>                         off
>                                 Disable all optional CPU mitigations.  This
>                                 improves system performance, but it may also
>                                 expose users to several CPU vulnerabilities.
>                                 Equivalent to: 
...
>                                                nopti [X86,PPC]

That's 100% unambiguous.

If you do "mitigations=off pti=auto", you might as well have done
"pti=auto nopti" which is nonsense.

The kernel shouldn't fall over and die, but the user gets to hold the
(undefined) pieces at this point.

Please let's not make this more complicated than it has to be.

Re: [PATCH 1/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[Sohil Mehta]

On 8/11/2023 4:42 PM, Dave Hansen wrote:
> On 8/11/23 16:27, Jo Van Bulck wrote:

>> Not sure which option would best match kernel coding guidelines?
> 
> This sound like it's getting a bit out of hand and reaching far beyond
> cleaning up some (mostly) harmless warnings.
> 

I agree this doesn't have to be this complex. PTI_FORCE_AUTO is unnecessary.

> pti=auto does *not* need to override mitigations=off.

I think only pti=on needs to override mitigations=off i.e. the User is
saying turn off mitigations but keep PTI enabled. This should be fairly
easy to achieve with the current enum. If it is not then it's not worth
the hassle.

Re: [PATCH 1/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[Dave Hansen]

On 8/11/23 16:27, Jo Van Bulck wrote:
> On 11.08.23 14:36, Jo Van Bulck wrote:>   static enum pti_mode {
>>       PTI_AUTO = 0,
>> +    PTI_FORCE_AUTO,
>>       PTI_FORCE_OFF,
>>       PTI_FORCE_ON
>>   } pti_mode;
> 
> I introduced a new PTI_FORCE_AUTO value here to make pti=auto override
> any mitigations=off parameter. However, I realize now that this may
> inadvertently affect other functions that test for pti_mode == PTI_AUTO
> (eg in pti_kernel_image_global_ok()).
> 
> Having 2 constants PTI_AUTO and PTI_FORCE_AUTO is arguably not very
> neat, so we should better get rid of this. I see several options:
> 
> - not have pti=auto override mitigations=off
> - have a global var to indicate pti= argument was passed
> - set pti_mode = PTI_AUTO in the pti_mode == PTI_FORCE_AUTO if branch
> 
> Not sure which option would best match kernel coding guidelines?

This sound like it's getting a bit out of hand and reaching far beyond
cleaning up some (mostly) harmless warnings.

I bet we have a billion command-line parameters that conflict with each
other.  mitigations=off and pti=auto is probably the least of our
worries.  Nobody in their right mind is going to say, oh, I *only* want
PTI, I don't care about any other mitigations.  That's nuts.

mitigations=off is the big hammer.  If you set that, you're basically
shouting from the rooftops, "moar speed!!"  You don't get security after
that.

pti=auto does *not* need to override mitigations=off.

Re: [PATCH 1/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[Jo Van Bulck]

On 11.08.23 14:36, Jo Van Bulck wrote:>   static enum pti_mode {
>   	PTI_AUTO = 0,
> +	PTI_FORCE_AUTO,
>   	PTI_FORCE_OFF,
>   	PTI_FORCE_ON
>   } pti_mode;

I introduced a new PTI_FORCE_AUTO value here to make pti=auto override 
any mitigations=off parameter. However, I realize now that this may 
inadvertently affect other functions that test for pti_mode == PTI_AUTO 
(eg in pti_kernel_image_global_ok()).

Having 2 constants PTI_AUTO and PTI_FORCE_AUTO is arguably not very 
neat, so we should better get rid of this. I see several options:

- not have pti=auto override mitigations=off
- have a global var to indicate pti= argument was passed
- set pti_mode = PTI_AUTO in the pti_mode == PTI_FORCE_AUTO if branch

Not sure which option would best match kernel coding guidelines?

Best,
Jo

[PATCH 1/1] x86/pti: Fix kernel warnings for pti= and nopti cmdline options.

[Jo Van Bulck]

Parse the pti= and nopti cmdline options using early_param to fix 'Unknown
kernel command line parameters "nopti", will be passed to user space'
warnings in the kernel log when nopti or pti= are passed to the kernel
cmdline on x86 platforms. Additionally allow the kernel to warn for
malformed pti= options.

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 arch/x86/mm/pti.c | 59 +++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 25 deletions(-)

diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 78414c6d1..da42e75dc 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -69,47 +69,34 @@ static void __init pti_print_if_secure(const char *reason)
 		pr_info("%s\n", reason);
 }
 
+/*
+ * Assume mode is auto unless overridden via cmdline below, where pti= takes
+ * priority over nopti and mitigations=off.
+ */
 static enum pti_mode {
 	PTI_AUTO = 0,
+	PTI_FORCE_AUTO,
 	PTI_FORCE_OFF,
 	PTI_FORCE_ON
 } pti_mode;
 
 void __init pti_check_boottime_disable(void)
 {
-	char arg[5];
-	int ret;
-
-	/* Assume mode is auto unless overridden. */
-	pti_mode = PTI_AUTO;
-
 	if (hypervisor_is_type(X86_HYPER_XEN_PV)) {
 		pti_mode = PTI_FORCE_OFF;
 		pti_print_if_insecure("disabled on XEN PV.");
 		return;
 	}
 
-	ret = cmdline_find_option(boot_command_line, "pti", arg, sizeof(arg));
-	if (ret > 0)  {
-		if (ret == 3 && !strncmp(arg, "off", 3)) {
-			pti_mode = PTI_FORCE_OFF;
-			pti_print_if_insecure("disabled on command line.");
-			return;
-		}
-		if (ret == 2 && !strncmp(arg, "on", 2)) {
-			pti_mode = PTI_FORCE_ON;
-			pti_print_if_secure("force enabled on command line.");
-			goto enable;
-		}
-		if (ret == 4 && !strncmp(arg, "auto", 4)) {
-			pti_mode = PTI_AUTO;
-			goto autosel;
-		}
+	if (pti_mode == PTI_FORCE_ON) {
+		pti_print_if_secure("force enabled on command line.");
+		goto enable;
 	}
-
-	if (cmdline_find_option_bool(boot_command_line, "nopti") ||
-	    cpu_mitigations_off()) {
+	if (pti_mode == PTI_FORCE_AUTO)
+		goto autosel;
+	if (cpu_mitigations_off())
 		pti_mode = PTI_FORCE_OFF;
+	if (pti_mode == PTI_FORCE_OFF) {
 		pti_print_if_insecure("disabled on command line.");
 		return;
 	}
@@ -121,6 +108,28 @@ void __init pti_check_boottime_disable(void)
 	setup_force_cpu_cap(X86_FEATURE_PTI);
 }
 
+static int __init pti_parse_cmdline(char *arg)
+{
+	if (!strcmp(arg, "off"))
+		pti_mode = PTI_FORCE_OFF;
+	else if (!strcmp(arg, "on"))
+		pti_mode = PTI_FORCE_ON;
+	else if (!strcmp(arg, "auto"))
+		pti_mode = PTI_FORCE_AUTO;
+	else
+		return -EINVAL;
+	return 0;
+}
+early_param("pti", pti_parse_cmdline);
+
+static int __init pti_parse_cmdline_nopti(char *arg)
+{
+	if (cmdline_find_option(boot_command_line, "pti", NULL, 0) == -1)
+		pti_mode = PTI_FORCE_OFF;
+	return 0;
+}
+early_param("nopti", pti_parse_cmdline_nopti);
+
 pgd_t __pti_set_user_pgtbl(pgd_t *pgdp, pgd_t pgd)
 {
 	/*
-- 
2.25.1