Re: [PATCH 4/7] x86/vsyscall: Setup vsyscall to compromise LASS protection

["Chen, Yian" <yian.chen@intel.com>]

On 1/10/2023 4:34 PM, Sohil Mehta wrote:
> On 1/9/2023 9:52 PM, Yian Chen wrote:
> 
>> The user can also opt-out LASS in config file to build kernel
>> binary.
> 
> This line is unnecessary.
> 
Sure, this line can be dropped.

>> Signed-off-by: Yian Chen <yian.chen@intel.com>
>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>> ---
>>   Documentation/admin-guide/kernel-parameters.txt | 12 ++++++++----
>>   arch/x86/entry/vsyscall/vsyscall_64.c           | 14 ++++++++++++++
>>   2 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
>> b/Documentation/admin-guide/kernel-parameters.txt
>> index 6cfa6e3996cf..3988e0c8c175 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -6755,10 +6755,14 @@
>>               versions of glibc use these calls.  Because these
>>               functions are at fixed addresses, they make nice
>>               targets for exploits that can control RIP.
>> -
>> -            emulate     [default] Vsyscalls turn into traps and are
>> -                        emulated reasonably safely.  The vsyscall
>> -                    page is readable.
> 
> The existing documentation here is incorrect. The default vsyscall mode 
> is actually xonly. This has been so since:
> commit 625b7b7f79c6 (x86/vsyscall: Change the default vsyscall mode to 
> xonly)
>
Yes, you are right. but this patch can overwrite and correct existing 
one. I am assuming we don't need to correct the existing document first 
before update it for LASS.

>> +            In newer versions of Intel platforms that come with
> 
> Words such as "newer" in the kernel start losing meaning very quickly. 
> Also, this comment looks out of place in between the vsyscall sub-options.
> 
>> +            LASS(Linear Address Space separation) protection,
>> +            vsyscall is disabled by default. Enabling vsyscall
>> +            via the parameter overrides LASS protection.
>> +
Sure, I will take out this part change.
> 
> 
> IIUC, you are making the default mode somewhat dynamic.
> 
> vsyscall = xonly (if LASS is not enabled)
> vsyscall = none (if LASS is enabled)
> 
Yes, this looks better.

> The decision to disable vsyscall doesn't happen at compile time but it 
> is taken at runtime when the LASS feature is detected. This would make 
> the system behavior highly platform dependent.
> 
> Instead of doing this dance, can we provide a simplified behavior to the 
> user/admin and move the decision making to compile time?
> 
Current strategy is to disable vsyscall by default only for LASS capable 
platforms. So that the dynamic decision is likely a necessary.

> Option 1: A bigger hammer
> Set the default vsyscall option as CONFIG_LEGACY_VSYSCALL_NONE. 
> CONFIG_X86_LASS is set by default. Changing the compile time VSYSCALL 
> option would disable LASS.
>
This means to disable vsyscall by default for all platforms, doen't 
matter LASS. I am not sure if we want to go that far.

> Option 2: A smaller hammer
> CONFIG_X86_LASS is off by default. Vsyscall default stays as 
> CONFIG_LEGACY_VSYSCALL_XONLY. Selecting LASS automatically chooses 
> CONFIG_LEGACY_VSYSCALL_NONE. In this case, even if the hardware doesn't 
> support LASS, vsyscall would still remain disabled.
> 
This turns out to disable LASS by default. Then the LASS may not be 
taken in the first place.

> In both of these cases, any command line input to override the vsyscall 
> behavior can disable LASS as well.
> 
> 
>> +            emulate     [default if not LASS capable] Vsyscalls
>> +                    turn into traps and are emulated reasonably
>> +                    safely.  The vsyscall page is readable.
>>               xonly       Vsyscalls turn into traps and are
>>                           emulated reasonably safely.  The vsyscall
>> diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c 
>> b/arch/x86/entry/vsyscall/vsyscall_64.c
>> index 4af81df133ee..2691f26835d1 100644
>> --- a/arch/x86/entry/vsyscall/vsyscall_64.c
>> +++ b/arch/x86/entry/vsyscall/vsyscall_64.c
>> @@ -63,6 +63,12 @@ static int __init vsyscall_setup(char *str)
>>           else
>>               return -EINVAL;
>> +        if (cpu_feature_enabled(X86_FEATURE_LASS) &&
>> +            vsyscall_mode != NONE) {
>> +            setup_clear_cpu_cap(X86_FEATURE_LASS);
>> +            pr_warn("LASS disabled by command line enabling 
>> vsyscall\n");
> 
> A warning seems too drastic here. A pr_info() should probably suffice.
> 
sure I will modify it to use pr_info.

>> +        }
>> +
>>           return 0;
>>       }
>> @@ -379,6 +385,14 @@ void __init map_vsyscall(void)
>>       extern char __vsyscall_page;
>>       unsigned long physaddr_vsyscall = __pa_symbol(&__vsyscall_page);
>> +    /*
>> +     * When LASS is on, vsyscall triggers a #GP fault,
>> +     * so that force vsyscall_mode to NONE.
>> +     */
> 
> This comment doesn't make much sense nor does it provide the full 
> picture. Some of the reasoning from the cover letter/commit log can be 
> duplicated here.
> 
sure, How about a more detail inline comment as following:
+	/*
+        * When LASS protection is on, user space vsyscall triggers
+        * a #GP fault since the vsyscall page is
+        * 0xffffffffff600000-0xffffffffff601000,
+	 * so that force vsyscall_mode to NONE and disable this mapping.
+	 */

>> +    if (cpu_feature_enabled(X86_FEATURE_LASS)) {
>> +        vsyscall_mode = NONE;
>> +        return;
>> +    }
>>       /*
>>        * For full emulation, the page needs to exist for real.  In
>>        * execute-only mode, there is no PTE at all backing the vsyscall
> 
Thanks,
Yian