Hi,
This is the second iteration of a patch to improve the cmdline option parsing
for PTI.
Changelog
---------
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 | 59 +++++++++++++++++++++++++++--------------------
1 file changed, 34 insertions(+), 25 deletions(-)
--
2.25.1
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 <[email protected]>
---
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
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
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.
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.
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.
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.
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