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

[PATCH RESEND] 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>
---

Resending this as I haven't heard back yet. I'd be happy to incorporate any
feedback.

Also adding test output before/after patch for reference:

dmesg | grep -e "page tables isolation" -e "Command line" \
             -e "Malformed" -e "Unknown kernel command line parameters" \
             && cat /sys/devices/system/cpu/vulnerabilities/meltdown

Before patch
============

KERNEL_CMDLINE="nopti"
 [    0.000000] Command line: root=/dev/vda console=ttyS0 nopti
 [    0.009875] Kernel/User page tables isolation: disabled on command line.
 [    0.021498] Unknown kernel command line parameters "nopti", will be passed to user space.
 Vulnerable

KERNEL_CMDLINE="pti=off"
 [    0.000000] Command line: root=/dev/vda console=ttyS0 pti=off
 [    0.009564] Kernel/User page tables isolation: disabled on command line.
 [    0.019542] Unknown kernel command line parameters "pti=off", will be passed to user space.
 Vulnerable

KERNEL_CMDLINE="pti=invalid"
 [    0.000000] Command line: root=/dev/vda console=ttyS0 pti=invalid
 [    0.021409] Unknown kernel command line parameters "pti=invalid", will be passed to user space.
 [    0.022411] Kernel/User page tables isolation: enabled
 Mitigation: PTI

After patch
===========

KERNEL_CMDLINE="nopti"
 [    0.000000] Command line: root=/dev/vda console=ttyS0 nopti
 [    0.009775] Kernel/User page tables isolation: disabled on command line.
 Vulnerable

KERNEL_CMDLINE="pti=off"
 [    0.000000] Command line: root=/dev/vda console=ttyS0 pti=off
 [    0.009879] Kernel/User page tables isolation: disabled on command line.
 Vulnerable

KERNEL_CMDLINE="pti=invalid"
 [    0.000000] Command line: root=/dev/vda console=ttyS0 pti=invalid
 [    0.000000] Malformed early option 'pti'
 [    0.020892] Kernel/User page tables isolation: enabled
 Mitigation: PTI


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

diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 78414c6d1..ea5841cf9 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,47 @@ 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 (pti_mode == PTI_FORCE_OFF || cpu_mitigations_off()) {
 		pti_mode = PTI_FORCE_OFF;
 		pti_print_if_insecure("disabled on command line.");
 		return;
 	}
 
-autosel:
-	if (!boot_cpu_has_bug(X86_BUG_CPU_MELTDOWN))
+	if (pti_mode == PTI_AUTO && !boot_cpu_has_bug(X86_BUG_CPU_MELTDOWN))
 		return;
-enable:
+
+	if (pti_mode == PTI_FORCE_ON)
+		pti_print_if_secure("force enabled on command line.");
 	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)
 {
 	/*

base-commit: 1399419a8db7b3d6083b47062358d95dc8ec9663
-- 
2.25.1

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

[Sohil Mehta]

On 8/8/2023 12:56 PM, Jo Van Bulck wrote:

> -
> -	if (cmdline_find_option_bool(boot_command_line, "nopti") ||
> -	    cpu_mitigations_off()) {
> +	if (pti_mode == PTI_FORCE_OFF || cpu_mitigations_off()) {

Can mitigations be off through some other mechanisms such as kernel config?

Maybe split the mitigations_off check into a separate if and it's own
unique print message?

The existing code might have the same issue as well.

Also, with the separated check you can avoid the unnecessary re-setting
of pti_mode when pti_mode == PTI_FORCE_OFF is true.


>  		pti_mode = PTI_FORCE_OFF;>  		pti_print_if_insecure("disabled on command line.");
>  		return;
>  	}
>  
> -autosel:
> -	if (!boot_cpu_has_bug(X86_BUG_CPU_MELTDOWN))
> +	if (pti_mode == PTI_AUTO && !boot_cpu_has_bug(X86_BUG_CPU_MELTDOWN))
>  		return;
> -enable:
> +
> +	if (pti_mode == PTI_FORCE_ON)
> +		pti_print_if_secure("force enabled on command line.");
>  	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);
> +

In the rare case that both pti= and nopti is set the existing code seems
to ignore the nopti option. Would the new implementation do the same?

Sohil

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

[Jo Van Bulck]

Thank you for the code review!

On 08.08.23 17:13, Sohil Mehta wrote:> Can mitigations be off through 
some other mechanisms such as kernel config?

Yes, from the kernel documentation [1]:

"It can be enabled by setting CONFIG_PAGE_TABLE_ISOLATION=y at compile 
time. Once enabled at compile-time, it can be disabled at boot with the 
'nopti' or 'pti=' kernel parameters"

In my understanding, if PTI is disabled at compile-time the full pti.c 
file is excluded and this code is never executed. I validated that, when 
compiling with CONFIG_PAGE_TABLE_ISOLATION=n, any nopti/pti= parameters 
are reported as unknown and 
/sys/devices/system/cpu/vulnerabilities/meltdown is reported as 
vulnerable. I validated this both with and without the proposed patch.

> Maybe split the mitigations_off check into a separate if and it's own
> unique print message?
> Also, with the separated check you can avoid the unnecessary re-setting
> of pti_mode when pti_mode == PTI_FORCE_OFF is true.

Thanks, makes sense. I'll make sure to do this in the next patch revision.

> In the rare case that both pti= and nopti is set the existing code seems
> to ignore the nopti option. Would the new implementation do the same?

Good point. In my understanding, passing such conflicting options is 
undefined as per the specification [2] and I'm not sure if backwards 
compatibility is a requirement?

That being said, I can see the argument that in this case of 
security-sensitive functionality, it may be desirable to maintain 
identical behavior for identical kernel parameter combinations and 
sequences. The current patch does indeed _not_ guarantee this. 
Particularly, I found there are currently 2 divergent cases:

CASE 1: PTI= > NOPTI
====================

Before patch pti= always takes priority:

KERNEL_CMDLINE="nopti pti=on"
[    0.022721] Unknown kernel command line parameters "nopti pti=on", 
will be passed to user space.
[    0.024146] Kernel/User page tables isolation: enabled
Mitigation: PTI

KERNEL_CMDLINE="pti=on nopti"
[    0.020566] Unknown kernel command line parameters "nopti pti=on", 
will be passed to user space.
[    0.021576] Kernel/User page tables isolation: enabled
Mitigation: PTI

After patch behavior depends on which option comes last in order:

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

CASE 2: MITIGATIONS=off
=======================

Before patch pti= always overrides mitigations=:

KERNEL_CMDLINE="mitigations=off pti=on"
[    0.017404] Unknown kernel command line parameters "pti=on", will be 
passed to user space.
[    0.018239] Kernel/User page tables isolation: enabled
Mitigation: PTI

KERNEL_CMDLINE="pti=on mitigations=off"
[    0.017356] Unknown kernel command line parameters "pti=on", will be 
passed to user space.
[    0.018232] Kernel/User page tables isolation: enabled
Mitigation: PTI

After patch, mitigations=off always takes priority:

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


--> I can update the patch to ensure backwards-compatible behavior in 
both cases for the next patch revision.

[1] https://www.kernel.org/doc/html/latest/arch/x86/pti.html
[2] 
https://www.kernel.org/doc/html/latest/admin-guide/kernel-parameters.html

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

[Sohil Mehta]

On 8/11/2023 11:23 AM, Jo Van Bulck wrote:

> On 08.08.23 17:13, Sohil Mehta wrote:> Can mitigations be off through 
> some other mechanisms such as kernel config?
> I validated this both with and without the proposed patch.
> 

Great, thanks for checking that. The existing print is accurate then. If
it is printed "disabled on command line.", then PTI has been disabled
due to a command line option.

>> Maybe split the mitigations_off check into a separate if and it's own
>> unique print message?
>> Also, with the separated check you can avoid the unnecessary re-setting
>> of pti_mode when pti_mode == PTI_FORCE_OFF is true.
> 
> Thanks, makes sense. I'll make sure to do this in the next patch revision.
> 

Based on above, even when you split the if check only a single print
would be enough, right?

>> In the rare case that both pti= and nopti is set the existing code seems
>> to ignore the nopti option. Would the new implementation do the same?
> 
> Good point. In my understanding, passing such conflicting options is 
> undefined as per the specification [2] and I'm not sure if backwards 
> compatibility is a requirement?
> 

> That being said, I can see the argument that in this case of 
> security-sensitive functionality, it may be desirable to maintain 
> identical behavior for identical kernel parameter combinations and 
> sequences. 

I don't believe that is a requirement either. Sometimes kernel command
lines can get very long and people make mistakes. I just thought it is
neat that the current code is defaulting that way and we should probably
keep the same behavior since it makes sense.

> 
> --> I can update the patch to ensure backwards-compatible behavior in 
> both cases for the next patch revision.
> 

I agree, in both cases pti= overriding the other option (nopti or
mitigations=off) sounds reasonable to me.

Sohil

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

[Jo Van Bulck]

On 11.08.23 12:13, Sohil Mehta wrote:> Based on above, even when you 
split the if check only a single print
> would be enough, right?

Yes, I agree these both cases can simply print "disabled on command 
line." (as in the original code) IMHO

> I don't believe that is a requirement either. Sometimes kernel command
> lines can get very long and people make mistakes. I just thought it is
> neat that the current code is defaulting that way and we should probably
> keep the same behavior since it makes sense.

Makes sense indeed.

> I agree, in both cases pti= overriding the other option (nopti or
> mitigations=off) sounds reasonable to me.

I prepared a revised patch for this and will post this shortly.

Best,
Jo