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

[Chen, Yian]

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@xxxxxxxxx>
> > Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
> > ---
> >   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