Re: [PATCH] mm: Check writable zero page in page table check

["Huang, Shaoqin" <shaoqin.huang@intel.com>]

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
>