Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <adf3a609-656f-82a2-6a4e-199a12de298b@intel.com>
Date:   Wed, 11 Jan 2023 17:43:18 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.6.1
Subject: Re: [PATCH 4/7] x86/vsyscall: Setup vsyscall to compromise LASS
 protection
Content-Language: en-US
To:     Sohil Mehta <sohil.mehta@intel.com>,
        <linux-kernel@vger.kernel.org>, <x86@kernel.org>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Ravi Shankar <ravi.v.shankar@intel.com>,
        "Tony Luck" <tony.luck@intel.com>, Paul Lai <paul.c.lai@intel.com>
References: <20230110055204.3227669-1-yian.chen@intel.com>
 <20230110055204.3227669-5-yian.chen@intel.com>
 <c2012105-0af6-6720-5a24-719dec775fed@intel.com>
From:   "Chen, Yian" <yian.chen@intel.com>
In-Reply-To: <c2012105-0af6-6720-5a24-719dec775fed@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WVFCbzF1V2NKZ3hGMlZ5cGRFNjBGcjhBODhVWjhwUFlUSWJlVzFyS1J0OGZx?=
 =?utf-8?B?ZDNSZXF0OEVmTnJzYlRRc0JTR21wU251MUtXck1ZbHl5VmNHWEo2S0tveEVn?=
 =?utf-8?B?KzBGVmdBZ2hSTVVwQ21tU0RjTFExWW9PQ0JTL1gzSkk3MWhDYS9CaEZqOG9s?=
 =?utf-8?B?eFN5dUpGYzliK3h6MURyZmRVNmxGRjZLZStvd2s2RDZVQ0dMVUVNdDloTmRy?=
 =?utf-8?B?SXh5bVNsVVVxUVdJbFdRanNmWEpwcHVleHZsM25jZ2RNbys2YWUwODVPODRT?=
 =?utf-8?B?c3ZoWkltUS9DVDJORXo1NDV0a0lKblpyNDhwNFRDMnVuczZQemI0N0ZOM2hp?=
 =?utf-8?B?czhKYkx4YVI1M3ZnSHVCU29BbkUyZ2V1YXhvSklzQUphYVBoSWNLbHNZUFlX?=
 =?utf-8?B?bXA0aU11UmZzRnpKRlZ3M2ptOUFabXhtR28yTThsU3ltdTlKcU9BMm9GbHRv?=
 =?utf-8?B?SVpNV1RwdFd1STRmM3ZxMWxVSzJJWVJsTlJYVktHa1R4YXEzYWo5TjkwZ0dX?=
 =?utf-8?B?NVNJbkNObUdrQm5HTm1xaEZYVWlURnp5UUNnbVh2d3dTWXNvSnRSMWdiR1k1?=
 =?utf-8?B?ejAvdFV1d1pFMW9JMTZaaWlVTkM4UjY3K0N5UGMxdnY1M0lLSEZvemtiV3ps?=
 =?utf-8?B?VGFkV1NIZWJKQzlxL3RTK0EvMTRQd2drQkd2RWZVNmVwaENPOE9NZVR0dnZU?=
 =?utf-8?B?ZmRpYlVGNXFHeU5pTnQzZWZRREptM0cvRFhQb0dsVWx6WDQ0OFBYOUVWU3pG?=
 =?utf-8?B?elBDa1N0TmZZSDVNeWVVQ2lWTFJGb0ZOTy80aHlPdDhaZE9MdjAxSzJIbWdB?=
 =?utf-8?B?eTVpbFpGWnJyNFJVMzVHQ3cwb3dlOVVScDhNNlhTZVdVaFR3MHlXbG05cFZ6?=
 =?utf-8?B?YkdCeCsrbDZxZStrUTYxZ0ZXQkdrYmdtRUp4T1FONG5mNllPVnZiV1BPR01T?=
 =?utf-8?B?OFRHc01JNkUvVVUrNlM4UDNnWU9vNndVdDRiTGIzRWNZT2NQNmY4a3dPdXdo?=
 =?utf-8?B?ejNQSUFPSS81SFp0NXpVRlA2cXNyL3gxc0d6Wlk0ZUVQYlk2RWtJd3FtYXly?=
 =?utf-8?B?eHJJZkhQSEdnVDJ3TXFweFk2WUFjSi9teXRXYncvaE5hUUdXNmduMnFtTDgy?=
 =?utf-8?B?S1p0K080WWtSS2hoanJvZmVBRy9WRkplSUdYOWVXL2FOUjFMcEt1NGtWckVK?=
 =?utf-8?B?SjdOUlJpZjhSR2xRbU1wNGl4MmFxRERzakNtZXFRZ0FHeGN1RVMzRmdPdS9Z?=
 =?utf-8?B?Z2hBaHFoM1BkWWgzMGtEb3IxVnhrdTF4ZnZkQ2pBZUdFTUVSMTlvZmI2NmpV?=
 =?utf-8?B?VmViQWtzQVRubWJwTjdiUSsvbEx0TzB5MUhuN3d4Mkpaay92aGJabEloTFFR?=
 =?utf-8?B?UjVXZXltazRJOFJKelIzcmVYOENtd0p2U2l2YXpuKzMwTExuN050eUtLVk1i?=
 =?utf-8?B?alI4YURralh0VDdWVnpCc1Y4dTZWTXpObXJFYXhMMDVkdGFiMUM5K3hxd3NJ?=
 =?utf-8?B?MmNzM0xxNkJDTi9GZW11Y0FSQWIwK2s2bHF6aHRjNlhkSUIxR1d6dGJYWG9V?=
 =?utf-8?B?d1Y3WEdDcnI4c3FOc1ZkdGFuL2NMUDJ0YVE1Wmx1TjU5TUY2V2xaTGJlQU8r?=
 =?utf-8?B?T0ZSVXJULzFMTjFaN2Rsb0p6TFZ2RUlVejFxQUIwQnZvS2lKRDBBNHJUY3VH?=
 =?utf-8?B?RSs5U2hTUUNkVFA4Y2N4K2tDdjNMTUt4cDR5QWthQ3kwc1FFU0VSMXdVZmEw?=
 =?utf-8?B?TTZwMzJCOG13YUt4T0hCNTlKZjQ1RCtibFJhdi8xaG1yS0x3dlNiMlQwL3Vp?=
 =?utf-8?B?Z2RxZFlxRCt0QzNvK1RDWjNTWk9xUkhvVDJaeHV4elEyZ0dua3A5YVY1KzZu?=
 =?utf-8?B?bWlEemJ0YWxUektSdUZneDRNREFTRDJzQTQwNWdtMC9jeU82TGVsQmhTcmI1?=
 =?utf-8?B?clVBRnRnVjFIQmRkeGtUbFRCamY3Z0ZMSUhNeFZjQ3huVlV6Y3A5am81SnpH?=
 =?utf-8?B?L2hYS0VpNVp0cTI4YU4xd2YvVU1sWjN3dHllVGpiYTRNZHgvMmxCeWJKdDBR?=
 =?utf-8?B?UTJGS3pyc092bXB2VzFGbDdwOFJidlp0WVpoazI4U3dOS2JMaGczNEQ2VUZU?=
 =?utf-8?Q?OY6tzhrhNT9wSJuxiRj8Tmi33?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 577fc941-6007-4194-5b41-08daf43e62d3
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB5262.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jan 2023 01:43:21.0622
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: RbcVFAlrRH2tabmQlj6uZnaglEV5MKXHqPBn4CnAkOlAOR1cMgFIW7BIhgh5tzXrndEMOs9vIX+ghFTsuu0imQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7017
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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