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: <3d82deb6-357d-0b54-ffd1-dce157674aad@intel.com>
Date:   Sat, 3 Sep 2022 10:13:10 +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:     Rick Edgecombe <rick.p.edgecombe@intel.com>,
        <pasha.tatashin@soleen.com>, <akpm@linux-foundation.org>,
        <linux-mm@kvack.org>
CC:     <linux-kernel@vger.kernel.org>
References: <20220902232732.12358-1-rick.p.edgecombe@intel.com>
From:   "Huang, Shaoqin" <shaoqin.huang@intel.com>
In-Reply-To: <20220902232732.12358-1-rick.p.edgecombe@intel.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?RXdUcWZVM3gwQUFUVWduemZFQytQMWxkYTZ1Q0dWZkp6RXRlNU0wWGY5c2Vq?=
 =?utf-8?B?ZENIQUZzdDBwWlFyODZsVXdkd0NSUGwybHMwRit4MWlTVGZ4Q3N5dmFUSlBl?=
 =?utf-8?B?K0JpY1pxVFJYNWEwaW5kcUdSRzBZUlJXeE1ua3daWmgrOFJadWNmSHVKTXI4?=
 =?utf-8?B?aldFTkRmTVM5aXc3alVzV2tyNlRFNHd2OUlQVnRKS281VWxGcWRRcHZMci9o?=
 =?utf-8?B?QS9jMFVIMzNHS0Q5Y1l6V05RamRneTlRYVA3ZjRIMXIyQi9DWldPTGQwZGNw?=
 =?utf-8?B?UkNhTFdraFlTMTRUUkZzUkM3cmRRRmhWT2kyUWFZaFdWLzIrRFNuVlVRdllQ?=
 =?utf-8?B?eFRIaVJZVnA0aGdiNFFCYm1xbUxkTmVZU1k3K0hoSFJCUU0zSTdGd2JJdDhL?=
 =?utf-8?B?YVJHTVMwL0pESXdxdnBFclArY05RNkltNXNoRkYxa3BsQ1VtMzdiZTBocDJM?=
 =?utf-8?B?Z28wNjVYOWhkSWlxZG85aDNRa1M1TzZUcG1PaWFxZkJYN2syRC9EZk9LclJx?=
 =?utf-8?B?MmdTNmxTZERhcEhncHYzbkFQU0RocHpZdkRjQ2RFdXRXSWF3QUJtaHNBZ2RH?=
 =?utf-8?B?em1qbXRPWWhIWGZrYTExU3hlN3dxVTJZRUU5ZnFWazRzVTVsdlZXY0l6ZDZj?=
 =?utf-8?B?T2RNazFMREJQZUs5S1hIVUt1MVFqV3JseXFTVnM4NkZWYTZiVzJWS2l4UHpi?=
 =?utf-8?B?Ni9taEhRZVR4bytmeWkva1Zja3BxVy9GdUFTQ1QvcUxlTnVKWHZtZ2x2WjJJ?=
 =?utf-8?B?R2d6cFYzcSt1dVkxcnBtdkM4NG5TR3FFaTZTUE81bXJ0My9MUEgwSzZoT1Rn?=
 =?utf-8?B?TythRStBWXpxY2p6UmllRjhHOGJOYlJpSmNQblBNcGUzcmVtaWFaNy9ob0Zk?=
 =?utf-8?B?bGU2L2JzWEpYUnR5ZVh0QWY0a1ZWS1VpdlJJWnYwWnN1azVpeWFsWm16blE5?=
 =?utf-8?B?WGwwWmIrcjRJbzIvbHNhWi83UUE2bzd6TmRKM29MVWlnZS9yc3R1Wm9KYXVk?=
 =?utf-8?B?QzZ1NWthdWo3cmdJb2RWandBNEhhcFhIMnh3T2l2bTFNcnUrcEUrYkY3QVMw?=
 =?utf-8?B?amRRamhrZkxhQytQd0ErRGcwSFVuUG0xOWUzNVFiWURJTDRIbEo5OWdYekpG?=
 =?utf-8?B?cStlV2dlc0YxUkU0Z0cyNk5iTHl5LzdobW5ablg4R1VpQTBuNGpqRUhpNFJN?=
 =?utf-8?B?djAzOE5wZ3ZjTHlCcENTQXU3WUJMUmVFeXM2ajBGYmNQQlRySFplYXg2K3VF?=
 =?utf-8?B?YTZ4d0V6dnJGRjdFb04vRWJDb09ibXNod1dKSVlyU0xZWGJoYzl4UUFESkkv?=
 =?utf-8?B?NHY3NGdqc1h5ZEM2TDg4Nk1teFZqV1A2SnM1RzAraWR1c20rR0FibWZocERk?=
 =?utf-8?B?Qm5mZU5MZGNLdHlPWFR1YVd0N1RwOXhpd3FrZGZkVWxiemk0QjMyQ2hESDRF?=
 =?utf-8?B?VmZRdElVR3VoV1gvejBSVmErQjVuYkYzbDcrWnFjdVVteUVaVzFpeVhXRTVE?=
 =?utf-8?B?eWpIZVA5ejdRZS9XQm9maFdKc0lwejBTNXp0c0ZtYWxhRTIwQ01HakMvNjJJ?=
 =?utf-8?B?dHA0UFg1cm5CNFRvaDdKUStUdUM2aUlIZVlBdktOQTlXbEZ0Q2VRaDNKbnF3?=
 =?utf-8?B?MisvUVIxcXFHcnlNNEdibGx4VTJtc1VaWlgvU2VxZFU3WlhpNkx0WU1EWE82?=
 =?utf-8?B?V09mUHBFNlFDRjJ2L1NUaDZ1bkViZFp1UW5zek93S1ZmZGxic0ZJOWtHZ0VQ?=
 =?utf-8?B?T1NSVkk3WW9ic3huK3dkMmFVZWptMDNFbmNQbFY5SXFjZHdyaHJwaVJldTM4?=
 =?utf-8?B?QTZoTDlKVG12WlBjcC8wOWsvbnl5Q1RyZm83eStrWHZRQ2VTSG9Oa2dkeWtx?=
 =?utf-8?B?ZDBxTlRGZGN6L21SWGtVekZwYmV5TkFsOFlwcWZGWUFyeERRZXNBTEYzREtk?=
 =?utf-8?B?YlVaeEFYL3BQM2NRcXFoUy9MY09TeHY1Kzh4MzdiSUdmU29HZzlEVXJYQ1JI?=
 =?utf-8?B?cmwzbVpmc1RuQjZkNFhZM2RkZTdCeVcycHZSbXd2RnNRajlBeE01djRhc1ls?=
 =?utf-8?B?dW1EaElVejhVMTBsaHhzazAzcHhvbGtESXc3UDFXVy82b0tiQVlWWm1Rb29O?=
 =?utf-8?B?S2EvQkN1NkYrOEtBNWNTN0NMV1hsZEZOaml1Mk1kWGdCbzQ4V0RaMlNyT0pP?=
 =?utf-8?B?b0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 809c0c39-020b-45c4-b514-08da8d51e47e
X-MS-Exchange-CrossTenant-AuthSource: MN2PR11MB3870.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Sep 2022 02:13:29.5785
 (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: 7gUhb9BO5Q7v5myMIxJut2Hv1HjZnfgXbVhPQfTatwdw6IeqlWuyfGbplKByOKeDUVZPQpw3tMGpUbsG8LDiCA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4896
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,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/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