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

[Huang, Shaoqin]

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@xxxxxxxxx> 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@xxxxxxxxx>
> > >
> > > ---
> > >
> > > 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@xxxxxxxxxx/
> >
> > >        page = pfn_to_page(pfn);
> > >        page_ext = lookup_page_ext(page);
> > >        anon = PageAnon(page);
> > >
> > > base-commit: b90cb1053190353cc30f0fef0ef1f378ccc063c5