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: <8ab04a74-3635-e902-6d28-df24b5753acc@intel.com>
Date:   Tue, 6 Sep 2022 09:01:00 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.13.0
Subject: Re: [PATCH] mm: Check writable zero page in page table check
Content-Language: en-US
To:     Pasha Tatashin <pasha.tatashin@soleen.com>
CC:     Rick Edgecombe <rick.p.edgecombe@intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-mm <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>
References: <20220902232732.12358-1-rick.p.edgecombe@intel.com>
 <3d82deb6-357d-0b54-ffd1-dce157674aad@intel.com>
 <CA+CK2bCAC4uQr_nrJM=mbP8DSpR7Vz=OGF9q7wufU_i4Wk3GBw@mail.gmail.com>
From:   "Huang, Shaoqin" <shaoqin.huang@intel.com>
In-Reply-To: <CA+CK2bCAC4uQr_nrJM=mbP8DSpR7Vz=OGF9q7wufU_i4Wk3GBw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?a3hTQTdnL3plaXQvR0kwcDUwY3JwOTNGd3c3TFhjNVRNQ0JZOWFhR3dvRjMy?=
 =?utf-8?B?WkRucHBmTmdLNVQ3a01IUFk2WW5qbGFRUmtGQmd4dmI0V1lXRFFpZm02cEc3?=
 =?utf-8?B?K2VKZTdrRmlRdGw5MVdmajlGWGhKOWtDb3JndDdlTCs0bFlZb09tUTMxY3hZ?=
 =?utf-8?B?MmpOd1JrbVFvSmlHdVdyaXRMN09OeWp4cUIzZkdPZEFYNUcvY2U2dkhFRkJh?=
 =?utf-8?B?anVKazhrajdibC9NM09EUlhFM1Q2cGRsMzV1V1J2RUNYRHdwbC9FUDkxM0xi?=
 =?utf-8?B?cE1QNEQycVQ4V3BobUR1TFdYWWYrc3hFZzA5ZEZoZGRCV2w3N0liaTRmV0o1?=
 =?utf-8?B?RGhnb2VuRWVpbnc1USt3Z29udXlnbHNJcVBGaHd4dVhyS3pacUxMeU5SMnpk?=
 =?utf-8?B?cGMyVmRjZUw0UmVEZzcrSG5PS0xZN0UyaHd0QnBOKzh3ekszKy8xOGwxb3NG?=
 =?utf-8?B?SnRwT2VaS2FsdTFjeFhYQWJWWk45VURRckF1aWJuY2RkN3JpVm5ING9aT0pu?=
 =?utf-8?B?RzZhcm1JNzFEQWExMHkrdzF6Ynk2Tlc1bC9yS2tCbTNqazJPS0c0VmI1a2Mw?=
 =?utf-8?B?VlBXRTcvdHFCMlVVcGgxNXBTTmgwTHFZa0lIczlJeElOQnRJa0hKTldJNE1J?=
 =?utf-8?B?WnpVY0s5UzBOTjdPTjU4R2ZsV1hJK0dJWU9jaW5ES1c3eUkrbVVUeDBBd1hj?=
 =?utf-8?B?WFhUeFNjRERhNVZKWFNWOWo1bTFaWkZENXRRRVJlYUtML2xPV0V2SWhGSkQ3?=
 =?utf-8?B?dFRUUHNjOVpWcDdVMGUyYTFRMzhEM1FqVjJyOUJxdUwxWnZ5QWk4NG8rcmlG?=
 =?utf-8?B?N0kzVzVlTVpQMG1HYXA1aE12WisxQm1TV0JvdHhaZW04RmRnQW9LM0NzZWVQ?=
 =?utf-8?B?OE1OcnpTZjFTZGM0THZDZjZSREQvMmlKV0RSMERjb012Q09CQW5mZ2lWSkZt?=
 =?utf-8?B?aFBqcUtEeVVOdENXR1JVSDhoUmpoUTFya2V4WTYxL3pmcTJXTVk1QkthcFpX?=
 =?utf-8?B?SVVPOUpNbWhFOGRCZkdNYUFFSFJTcWdMQ1NZakZjaGkvSWF5TUFxejRpeVRJ?=
 =?utf-8?B?cFBZbUJhWHdtQWorSTgzUk5XS1hQNGpIOExJcEdCQy94azFJWktnYWFUaTNr?=
 =?utf-8?B?cFRzTEZRTXlCdTJUbHFBUUtzSU4rUEFLVjRZMUtUdUY2WWtWTktKNzJZTTJs?=
 =?utf-8?B?TTNOZFJ5czN3bHBzVnlXbk5WblYvZ0VieHJuQUhEVUVySzVEMG1CVXA4dmkr?=
 =?utf-8?B?Y0E4MTY1Y3lNMEE0UFNNb1I0MThRVmlWQ0Q5S2dNRHZ2cG9keksvYTBjZWFQ?=
 =?utf-8?B?TWM2Rmhud2RzcEV2OEd3K3VhTGZ1QXIvaU1Gd1pUMmV4V1lRSnJWTmVmTnlF?=
 =?utf-8?B?N3B5R3JxRVg0dy9mSEZIaWZIRjlRK3U5VktvT0xrRTc2ai81VXVueWpOZUt4?=
 =?utf-8?B?MWNnUVZ2c1R0eEdtRHZBMHFIUURRSjVxaU43QndPOXhWWXhlZzUrUi9reDFV?=
 =?utf-8?B?U1ErOGdvS21wc2lDclRMM2xnTWRDWkFpTjBZQXhncG54dU1vVWpiK241NHRD?=
 =?utf-8?B?NElyZWpsUTR4TC9kSFpuYTdNd3VQT3ZWMDZYQUV1QlJzYWF6Nm5PNkM2ZDZx?=
 =?utf-8?B?d1FNZEVjUytIMnVZQ2xxa0xEMUltcmRpdzRsRC9TdHFwbmN1SzY4cFRVRlNa?=
 =?utf-8?B?UThWMFBta1UrM3hDVDY1TUtwNm51a2NJRW9GS1paVVNnS2w2RUI1YkJoRmIv?=
 =?utf-8?B?QU1yalNOSWIreU9UTUdpa2tUbDJCYjRsL3lHZldReUtkUkJqQ3hXclFtTDR1?=
 =?utf-8?B?S3BBWitXdUViTWJnUkZaMmVERUJJbVFYMlY5SVVieFB4UktYeUwrLzFmdXBu?=
 =?utf-8?B?d1ZjU3dDc0c5bndwM2Z3RWNmaEc5Y0ZDUFBwNEpYaE83cVZ6U002TFgrZFh3?=
 =?utf-8?B?cVVRT1JROG1scGFLamV5Mm5saC8xa1NIR001NkZjdkdhOUI3QktlTzJLbDE4?=
 =?utf-8?B?bUdndnNnSEV4bktjeVoyNUg1SU1DWEkyREFhVjF3aVFUam1KNFFVdmVNM0oz?=
 =?utf-8?B?WEswSzkyY29wWEUxb3hpd0NZamxzK25Wc210ZEtWdi9NdnZoTFlYVytUR1Av?=
 =?utf-8?B?WTMra0JPWWg1QXIvd2gyUnJwV21wSi9kWng1SXVoUU1CTFNLTTZrN3RRUnVO?=
 =?utf-8?B?Qnc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: cc60b530-1586-4aba-c405-08da8fa34920
X-MS-Exchange-CrossTenant-AuthSource: MN2PR11MB3870.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Sep 2022 01:01:09.6609
 (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: tCoCDfMgXycivWqyyRFqh7pqIL9qdEWzR+Ifv6G71B824eHe/LmCE5GmgFFUqZI9F35nYcNL8lESzaM38uXcWw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR11MB1810
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-3.8 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,T_SCC_BODY_TEXT_LINE 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 9/6/2022 8:37 AM, Pasha Tatashin wrote:
> Hi Shaoqin,
> 
> The idea behind page table check is to prevent some types of memory
> corruptions: i.e. prevent false page sharing, and memory leaking
> between address spaces. This is an optional security feature for
> setups where it is more dangerous to leak data than to crash the
> machine. Therefore, when page table check detects illegal page sharing
> it immediately crashes the kernel. I think we can have a
> page_table_check option that would change BUG_ON to WARN_ON() (or to
> WARN_ON_ONCE(), since once corruption is detected I believe it might
> show up many times again)
Hi, Pasha,

Thanks for your explanation. That's make sense.

> 
> Pasha
> 
> On Fri, Sep 2, 2022 at 10:13 PM Huang, Shaoqin <shaoqin.huang@intel.com> wrote:
>>
>>
>>
>> On 9/3/2022 7:27 AM, Rick Edgecombe wrote:
>>> The zero page should remain all zero, so that it can be mapped as
>>> read-only for read faults of memory that should be zeroed. If it is ever
>>> mapped writable to userspace, it could become non-zero and so other apps
>>> would unexpectedly get non-zero data. So the zero page should never be
>>> mapped writable to userspace. Check for this condition in
>>> page_table_check_set().
>>>
>>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>>>
>>> ---
>>>
>>> Hi,
>>>
>>> CONFIG_PAGE_TABLE_CHECK is pretty explicit about what it checks (and
>>> doesn't mention the zero page), but this condition seems to fit with the
>>> general category of "pages mapped wrongly to userspace". I added it
>>> locally to help me debug something. Maybe it's more widely useful >>>
>>>    mm/page_table_check.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/mm/page_table_check.c b/mm/page_table_check.c
>>> index e2062748791a..665ece0d55d4 100644
>>> --- a/mm/page_table_check.c
>>> +++ b/mm/page_table_check.c
>>> @@ -102,6 +102,8 @@ static void page_table_check_set(struct mm_struct *mm, unsigned long addr,
>>>        if (!pfn_valid(pfn))
>>>                return;
>>>
>>> +     BUG_ON(is_zero_pfn(pfn) && rw);
>>> +
>>
>> Why we need use BUG_ON() here? Based on [1], we should avoid to use the
>> BUG_ON() due to it will panic the machine.
>>
>> [1]: https://lore.kernel.org/lkml/20220824163100.224449-1-david@redhat.com/
>>
>>>        page = pfn_to_page(pfn);
>>>        page_ext = lookup_page_ext(page);
>>>        anon = PageAnon(page);
>>>
>>> base-commit: b90cb1053190353cc30f0fef0ef1f378ccc063c5
>