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;
MIME-Version: 1.0
References: <20220902232732.12358-1-rick.p.edgecombe@intel.com> <3d82deb6-357d-0b54-ffd1-dce157674aad@intel.com>
In-Reply-To: <3d82deb6-357d-0b54-ffd1-dce157674aad@intel.com>
From:   Pasha Tatashin <pasha.tatashin@soleen.com>
Date:   Mon, 5 Sep 2022 20:37:27 -0400
Message-ID: <CA+CK2bCAC4uQr_nrJM=mbP8DSpR7Vz=OGF9q7wufU_i4Wk3GBw@mail.gmail.com>
Subject: Re: [PATCH] mm: Check writable zero page in page table check
To:     "Huang, Shaoqin" <shaoqin.huang@intel.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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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)

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