diff mbox series

arm64/mm: fold check for KFENCE into can_set_direct_map()

Message ID 20220921074841.382615-1-rppt@kernel.org (mailing list archive)
State New, archived
Headers show
Series arm64/mm: fold check for KFENCE into can_set_direct_map() | expand

Commit Message

Mike Rapoport Sept. 21, 2022, 7:48 a.m. UTC
From: Mike Rapoport <rppt@linux.ibm.com>

KFENCE requires linear map to be mapped at page granularity, so that it
is possible to protect/unprotect single pages, just like with
rodata_full and DEBUG_PAGEALLOC.

Instead of repating

	can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE)

make can_set_direct_map() handle the KFENCE case.

This also prevents potential false positives in kernel_page_present()
that may return true for non-present page if CONFIG_KFENCE is enabled.

Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
---
 arch/arm64/mm/mmu.c      | 8 ++------
 arch/arm64/mm/pageattr.c | 8 +++++++-
 2 files changed, 9 insertions(+), 7 deletions(-)

Comments

Barry Song Sept. 21, 2022, 9 a.m. UTC | #1
On Wed, Sep 21, 2022 at 8:26 PM Mike Rapoport <rppt@kernel.org> wrote:
>
> From: Mike Rapoport <rppt@linux.ibm.com>
>
> KFENCE requires linear map to be mapped at page granularity, so that it
> is possible to protect/unprotect single pages, just like with
> rodata_full and DEBUG_PAGEALLOC.
>
> Instead of repating
>
>         can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE)
>
> make can_set_direct_map() handle the KFENCE case.
>
> This also prevents potential false positives in kernel_page_present()
> that may return true for non-present page if CONFIG_KFENCE is enabled.
>
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> ---
>  arch/arm64/mm/mmu.c      | 8 ++------
>  arch/arm64/mm/pageattr.c | 8 +++++++-
>  2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index e7ad44585f40..c5065abec55a 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -535,7 +535,7 @@ static void __init map_mem(pgd_t *pgdp)
>          */
>         BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end));
>
> -       if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> +       if (can_set_direct_map())
>                 flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>
>         /*
> @@ -1547,11 +1547,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
>
>         VM_BUG_ON(!mhp_range_allowed(start, size, true));
>
> -       /*
> -        * KFENCE requires linear map to be mapped at page granularity, so that
> -        * it is possible to protect/unprotect single pages in the KFENCE pool.
> -        */
> -       if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> +       if (can_set_direct_map())
>                 flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>
>         __create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> index 64e985eaa52d..d107c3d434e2 100644
> --- a/arch/arm64/mm/pageattr.c
> +++ b/arch/arm64/mm/pageattr.c
> @@ -21,7 +21,13 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
>
>  bool can_set_direct_map(void)
>  {
> -       return rodata_full || debug_pagealloc_enabled();
> +       /*
> +        * rodata_full, DEBUG_PAGEALLOC and KFENCE require linear map to be
> +        * mapped at page granularity, so that it is possible to
> +        * protect/unprotect single pages.
> +        */
> +       return rodata_full || debug_pagealloc_enabled() ||
> +               IS_ENABLED(CONFIG_KFENCE);

might be irrelevant, i wonder if rodata_full is too strict as
rodata_full is almost
always true since RODATA_FULL_DEFAULT_ENABLED is default true.


>  }
>
>  static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
> --
> 2.35.3
>

Thanks
Barry
Anshuman Khandual Sept. 21, 2022, 11:39 a.m. UTC | #2
On 9/21/22 13:18, Mike Rapoport wrote:
> From: Mike Rapoport <rppt@linux.ibm.com>
> 
> KFENCE requires linear map to be mapped at page granularity, so that it
> is possible to protect/unprotect single pages, just like with
> rodata_full and DEBUG_PAGEALLOC.
> 
> Instead of repating
> 
> 	can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE)
> 
> make can_set_direct_map() handle the KFENCE case.
> 
> This also prevents potential false positives in kernel_page_present()
> that may return true for non-present page if CONFIG_KFENCE is enabled.
> 
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> ---
>  arch/arm64/mm/mmu.c      | 8 ++------
>  arch/arm64/mm/pageattr.c | 8 +++++++-
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index e7ad44585f40..c5065abec55a 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -535,7 +535,7 @@ static void __init map_mem(pgd_t *pgdp)
>  	 */
>  	BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end));
>  
> -	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> +	if (can_set_direct_map())
>  		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>  
>  	/*
> @@ -1547,11 +1547,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
>  
>  	VM_BUG_ON(!mhp_range_allowed(start, size, true));
>  
> -	/*
> -	 * KFENCE requires linear map to be mapped at page granularity, so that
> -	 * it is possible to protect/unprotect single pages in the KFENCE pool.
> -	 */
> -	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> +	if (can_set_direct_map())
>  		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>  
>  	__create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> index 64e985eaa52d..d107c3d434e2 100644
> --- a/arch/arm64/mm/pageattr.c
> +++ b/arch/arm64/mm/pageattr.c
> @@ -21,7 +21,13 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
>  
>  bool can_set_direct_map(void)
>  {
> -	return rodata_full || debug_pagealloc_enabled();
> +	/*
> +	 * rodata_full, DEBUG_PAGEALLOC and KFENCE require linear map to be
> +	 * mapped at page granularity, so that it is possible to
> +	 * protect/unprotect single pages.
> +	 */
> +	return rodata_full || debug_pagealloc_enabled() ||
> +		IS_ENABLED(CONFIG_KFENCE);
>  }

Changing can_set_direct_map() also changes behaviour for other functions such as 

set_direct_map_default_noflush()
set_direct_map_invalid_noflush()
__kernel_map_pages()

Is that okay ?

>  
>  static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
Mike Rapoport Sept. 21, 2022, 3:15 p.m. UTC | #3
Hi Barry,

On Wed, Sep 21, 2022 at 09:00:28PM +1200, Barry Song wrote:
> On Wed, Sep 21, 2022 at 8:26 PM Mike Rapoport <rppt@kernel.org> wrote:
> >
> > From: Mike Rapoport <rppt@linux.ibm.com>
> >
> > KFENCE requires linear map to be mapped at page granularity, so that it
> > is possible to protect/unprotect single pages, just like with
> > rodata_full and DEBUG_PAGEALLOC.
> >
> > Instead of repating
> >
> >         can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE)
> >
> > make can_set_direct_map() handle the KFENCE case.
> >
> > This also prevents potential false positives in kernel_page_present()
> > that may return true for non-present page if CONFIG_KFENCE is enabled.
> >
> > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> > ---
> >  arch/arm64/mm/mmu.c      | 8 ++------
> >  arch/arm64/mm/pageattr.c | 8 +++++++-
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > index e7ad44585f40..c5065abec55a 100644
> > --- a/arch/arm64/mm/mmu.c
> > +++ b/arch/arm64/mm/mmu.c
> > @@ -535,7 +535,7 @@ static void __init map_mem(pgd_t *pgdp)
> >          */
> >         BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end));
> >
> > -       if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> > +       if (can_set_direct_map())
> >                 flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> >
> >         /*
> > @@ -1547,11 +1547,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
> >
> >         VM_BUG_ON(!mhp_range_allowed(start, size, true));
> >
> > -       /*
> > -        * KFENCE requires linear map to be mapped at page granularity, so that
> > -        * it is possible to protect/unprotect single pages in the KFENCE pool.
> > -        */
> > -       if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> > +       if (can_set_direct_map())
> >                 flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> >
> >         __create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
> > diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> > index 64e985eaa52d..d107c3d434e2 100644
> > --- a/arch/arm64/mm/pageattr.c
> > +++ b/arch/arm64/mm/pageattr.c
> > @@ -21,7 +21,13 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
> >
> >  bool can_set_direct_map(void)
> >  {
> > -       return rodata_full || debug_pagealloc_enabled();
> > +       /*
> > +        * rodata_full, DEBUG_PAGEALLOC and KFENCE require linear map to be
> > +        * mapped at page granularity, so that it is possible to
> > +        * protect/unprotect single pages.
> > +        */
> > +       return rodata_full || debug_pagealloc_enabled() ||
> > +               IS_ENABLED(CONFIG_KFENCE);
> 
> might be irrelevant, i wonder if rodata_full is too strict as
> rodata_full is almost
> always true since RODATA_FULL_DEFAULT_ENABLED is default true.

Not sure I follow. If either of these conditions is true the linear map
consists of base pages and it's possible to change attributes of each base
page. Whenever linear map contains block mapping, page attributes cannot be
modified.

And rodata_full might be false because
CONFIG_RODATA_FULL_DEFAULT_ENABLED was disabled at build time.
 
> >  }
> >
> >  static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
> > --
> > 2.35.3
> >
> 
> Thanks
> Barry
Mike Rapoport Sept. 21, 2022, 3:19 p.m. UTC | #4
Hi Anshuman,

On Wed, Sep 21, 2022 at 05:09:19PM +0530, Anshuman Khandual wrote:
> 
> 
> On 9/21/22 13:18, Mike Rapoport wrote:
> > From: Mike Rapoport <rppt@linux.ibm.com>
> > 
> > KFENCE requires linear map to be mapped at page granularity, so that it
> > is possible to protect/unprotect single pages, just like with
> > rodata_full and DEBUG_PAGEALLOC.
> > 
> > Instead of repating
> > 
> > 	can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE)
> > 
> > make can_set_direct_map() handle the KFENCE case.
> > 
> > This also prevents potential false positives in kernel_page_present()
> > that may return true for non-present page if CONFIG_KFENCE is enabled.
> > 
> > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> > ---
> >  arch/arm64/mm/mmu.c      | 8 ++------
> >  arch/arm64/mm/pageattr.c | 8 +++++++-
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > index e7ad44585f40..c5065abec55a 100644
> > --- a/arch/arm64/mm/mmu.c
> > +++ b/arch/arm64/mm/mmu.c
> > @@ -535,7 +535,7 @@ static void __init map_mem(pgd_t *pgdp)
> >  	 */
> >  	BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end));
> >  
> > -	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> > +	if (can_set_direct_map())
> >  		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> >  
> >  	/*
> > @@ -1547,11 +1547,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
> >  
> >  	VM_BUG_ON(!mhp_range_allowed(start, size, true));
> >  
> > -	/*
> > -	 * KFENCE requires linear map to be mapped at page granularity, so that
> > -	 * it is possible to protect/unprotect single pages in the KFENCE pool.
> > -	 */
> > -	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> > +	if (can_set_direct_map())
> >  		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> >  
> >  	__create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
> > diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> > index 64e985eaa52d..d107c3d434e2 100644
> > --- a/arch/arm64/mm/pageattr.c
> > +++ b/arch/arm64/mm/pageattr.c
> > @@ -21,7 +21,13 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
> >  
> >  bool can_set_direct_map(void)
> >  {
> > -	return rodata_full || debug_pagealloc_enabled();
> > +	/*
> > +	 * rodata_full, DEBUG_PAGEALLOC and KFENCE require linear map to be
> > +	 * mapped at page granularity, so that it is possible to
> > +	 * protect/unprotect single pages.
> > +	 */
> > +	return rodata_full || debug_pagealloc_enabled() ||
> > +		IS_ENABLED(CONFIG_KFENCE);
> >  }
> 
> Changing can_set_direct_map() also changes behaviour for other functions such as 
> 
> set_direct_map_default_noflush()
> set_direct_map_invalid_noflush()
> __kernel_map_pages()
> 
> Is that okay ?
 
Yes. Since KFENCE disables block mappings, these will actually change the
page tables.
Actually, before this change the test for can_set_direct_map() in these
functions was false negative when CONFIG_KFENCE=y
  
> >  static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
Barry Song Sept. 21, 2022, 9:45 p.m. UTC | #5
On Thu, Sep 22, 2022 at 3:16 AM Mike Rapoport <rppt@kernel.org> wrote:
>
> Hi Barry,
>
> On Wed, Sep 21, 2022 at 09:00:28PM +1200, Barry Song wrote:
> > On Wed, Sep 21, 2022 at 8:26 PM Mike Rapoport <rppt@kernel.org> wrote:
> > >
> > > From: Mike Rapoport <rppt@linux.ibm.com>
> > >
> > > KFENCE requires linear map to be mapped at page granularity, so that it
> > > is possible to protect/unprotect single pages, just like with
> > > rodata_full and DEBUG_PAGEALLOC.
> > >
> > > Instead of repating
> > >
> > >         can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE)
> > >
> > > make can_set_direct_map() handle the KFENCE case.
> > >
> > > This also prevents potential false positives in kernel_page_present()
> > > that may return true for non-present page if CONFIG_KFENCE is enabled.
> > >
> > > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> > > ---
> > >  arch/arm64/mm/mmu.c      | 8 ++------
> > >  arch/arm64/mm/pageattr.c | 8 +++++++-
> > >  2 files changed, 9 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > > index e7ad44585f40..c5065abec55a 100644
> > > --- a/arch/arm64/mm/mmu.c
> > > +++ b/arch/arm64/mm/mmu.c
> > > @@ -535,7 +535,7 @@ static void __init map_mem(pgd_t *pgdp)
> > >          */
> > >         BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end));
> > >
> > > -       if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> > > +       if (can_set_direct_map())
> > >                 flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> > >
> > >         /*
> > > @@ -1547,11 +1547,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
> > >
> > >         VM_BUG_ON(!mhp_range_allowed(start, size, true));
> > >
> > > -       /*
> > > -        * KFENCE requires linear map to be mapped at page granularity, so that
> > > -        * it is possible to protect/unprotect single pages in the KFENCE pool.
> > > -        */
> > > -       if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> > > +       if (can_set_direct_map())
> > >                 flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> > >
> > >         __create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
> > > diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> > > index 64e985eaa52d..d107c3d434e2 100644
> > > --- a/arch/arm64/mm/pageattr.c
> > > +++ b/arch/arm64/mm/pageattr.c
> > > @@ -21,7 +21,13 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
> > >
> > >  bool can_set_direct_map(void)
> > >  {
> > > -       return rodata_full || debug_pagealloc_enabled();
> > > +       /*
> > > +        * rodata_full, DEBUG_PAGEALLOC and KFENCE require linear map to be
> > > +        * mapped at page granularity, so that it is possible to
> > > +        * protect/unprotect single pages.
> > > +        */
> > > +       return rodata_full || debug_pagealloc_enabled() ||
> > > +               IS_ENABLED(CONFIG_KFENCE);
> >
> > might be irrelevant, i wonder if rodata_full is too strict as
> > rodata_full is almost
> > always true since RODATA_FULL_DEFAULT_ENABLED is default true.
>
> Not sure I follow. If either of these conditions is true the linear map
> consists of base pages and it's possible to change attributes of each base
> page. Whenever linear map contains block mapping, page attributes cannot be
> modified.

Hi Mike,
My question is irrelevant with your patch. It is more of another
topic.  i understand
we need to protect read-only data of kernel, but it seems overly
defensive. We are
getting the whole linear mapping PTE-mapped.

/sys/kernel/debug# cat kernel_page_tables
0x0000000000000000-0xffff608000000000   17179705856G PGD
0xffff608000000000-0xffff60bf40000000         253G PUD
0xffff60bf40000000-0xffff60bf40200000           2M PTE       RW NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60bf40200000-0xffff60bf41800000          22M PMD       ro NX SHD
AF NG     BLK UXN    MEM/NORMAL
0xffff60bf41800000-0xffff60bf41890000         576K PTE       ro NX SHD
AF NG         UXN    MEM/NORMAL
0xffff60bf41890000-0xffff60c04022e000     4171384K PTE       RW NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c04022e000-0xffff60c04022f000           4K PTE       ro NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c04022f000-0xffff60c042a3c000       41012K PTE       RW NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c042a3c000-0xffff60c042a3d000           4K PTE       ro NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c042a3d000-0xffff60c043431000       10192K PTE       RW NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c043431000-0xffff60c043432000           4K PTE       ro NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c043432000-0xffff60c0448e8000       21208K PTE       RW NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c0448e8000-0xffff60c0448e9000           4K PTE       ro NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c0448e9000-0xffff60c0448ed000          16K PTE       RW NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c0448ed000-0xffff60c0448ee000           4K PTE       ro NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c0448ee000-0xffff60c044a6b000        1524K PTE       RW NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c044a6b000-0xffff60c044a6c000           4K PTE       ro NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c044a6c000-0xffff60c044a74000          32K PTE       RW NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c044a74000-0xffff60c044a75000           4K PTE       ro NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c044a75000-0xffff60c044aaa000         212K PTE       RW NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c044aaa000-0xffff60c044aab000           4K PTE       ro NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c044aab000-0xffff60c053000000      234836K PTE       RW NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c053000000-0xffff60c080000000         720M PMD
0xffff60c080000000-0xffff610000000000         254G PUD
0xffff610000000000-0xffff800000000000          31T PGD
---[ Linear Mapping end ]---


For example, for the below,
0xffff60c04022e000-0xffff60c04022f000           4K PTE       ro NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c04022f000-0xffff60c042a3c000       41012K PTE       RW NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED

41012K PTE is really big, don't we have a chance to make it
block/cont-pte mapped by
some alignment tricks such as,

0xffff60c04022e000-0xffff60c04022f000           4K PTE       ro NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c04022e000-0xffff60c040230000          4K PTE       RW NX SHD
AF NG         UXN    MEM/NORMAL-TAGGED
0xffff60c040230000-0xffff60c040400000-         4K PTE       CONT..
0xffff60c040400000-                                           2MB PMD.....

>
> And rodata_full might be false because
> CONFIG_RODATA_FULL_DEFAULT_ENABLED was disabled at build time.
>
> > >  }
> > >
> > >  static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
> > > --
> > > 2.35.3
> > >
> >
> > Thanks
> > Barry
>
> --
> Sincerely yours,
> Mike.

Thanks
Barry
Anshuman Khandual Sept. 22, 2022, 2:51 a.m. UTC | #6
On 9/21/22 20:49, Mike Rapoport wrote:
> Hi Anshuman,
> 
> On Wed, Sep 21, 2022 at 05:09:19PM +0530, Anshuman Khandual wrote:
>>
>>
>> On 9/21/22 13:18, Mike Rapoport wrote:
>>> From: Mike Rapoport <rppt@linux.ibm.com>
>>>
>>> KFENCE requires linear map to be mapped at page granularity, so that it
>>> is possible to protect/unprotect single pages, just like with
>>> rodata_full and DEBUG_PAGEALLOC.
>>>
>>> Instead of repating
>>>
>>> 	can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE)
>>>
>>> make can_set_direct_map() handle the KFENCE case.
>>>
>>> This also prevents potential false positives in kernel_page_present()
>>> that may return true for non-present page if CONFIG_KFENCE is enabled.
>>>
>>> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
>>> ---
>>>  arch/arm64/mm/mmu.c      | 8 ++------
>>>  arch/arm64/mm/pageattr.c | 8 +++++++-
>>>  2 files changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>> index e7ad44585f40..c5065abec55a 100644
>>> --- a/arch/arm64/mm/mmu.c
>>> +++ b/arch/arm64/mm/mmu.c
>>> @@ -535,7 +535,7 @@ static void __init map_mem(pgd_t *pgdp)
>>>  	 */
>>>  	BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end));
>>>  
>>> -	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
>>> +	if (can_set_direct_map())
>>>  		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>>>  
>>>  	/*
>>> @@ -1547,11 +1547,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
>>>  
>>>  	VM_BUG_ON(!mhp_range_allowed(start, size, true));
>>>  
>>> -	/*
>>> -	 * KFENCE requires linear map to be mapped at page granularity, so that
>>> -	 * it is possible to protect/unprotect single pages in the KFENCE pool.
>>> -	 */
>>> -	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
>>> +	if (can_set_direct_map())
>>>  		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>>>  
>>>  	__create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
>>> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
>>> index 64e985eaa52d..d107c3d434e2 100644
>>> --- a/arch/arm64/mm/pageattr.c
>>> +++ b/arch/arm64/mm/pageattr.c
>>> @@ -21,7 +21,13 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
>>>  
>>>  bool can_set_direct_map(void)
>>>  {
>>> -	return rodata_full || debug_pagealloc_enabled();
>>> +	/*
>>> +	 * rodata_full, DEBUG_PAGEALLOC and KFENCE require linear map to be
>>> +	 * mapped at page granularity, so that it is possible to
>>> +	 * protect/unprotect single pages.
>>> +	 */
>>> +	return rodata_full || debug_pagealloc_enabled() ||
>>> +		IS_ENABLED(CONFIG_KFENCE);
>>>  }
>>
>> Changing can_set_direct_map() also changes behaviour for other functions such as 
>>
>> set_direct_map_default_noflush()
>> set_direct_map_invalid_noflush()
>> __kernel_map_pages()
>>
>> Is that okay ?
>  
> Yes. Since KFENCE disables block mappings, these will actually change the
> page tables.
> Actually, before this change the test for can_set_direct_map() in these
> functions was false negative when CONFIG_KFENCE=y

Okay but then should not this have a "Fixes:" tag as well ?

>   
>>>  static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
>
Anshuman Khandual Sept. 22, 2022, 2:52 a.m. UTC | #7
On 9/21/22 13:18, Mike Rapoport wrote:
> From: Mike Rapoport <rppt@linux.ibm.com>
> 
> KFENCE requires linear map to be mapped at page granularity, so that it
> is possible to protect/unprotect single pages, just like with
> rodata_full and DEBUG_PAGEALLOC.
> 
> Instead of repating
> 
> 	can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE)
> 
> make can_set_direct_map() handle the KFENCE case.
> 
> This also prevents potential false positives in kernel_page_present()
> that may return true for non-present page if CONFIG_KFENCE is enabled.
> 
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

> ---
>  arch/arm64/mm/mmu.c      | 8 ++------
>  arch/arm64/mm/pageattr.c | 8 +++++++-
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index e7ad44585f40..c5065abec55a 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -535,7 +535,7 @@ static void __init map_mem(pgd_t *pgdp)
>  	 */
>  	BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end));
>  
> -	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> +	if (can_set_direct_map())
>  		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>  
>  	/*
> @@ -1547,11 +1547,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
>  
>  	VM_BUG_ON(!mhp_range_allowed(start, size, true));
>  
> -	/*
> -	 * KFENCE requires linear map to be mapped at page granularity, so that
> -	 * it is possible to protect/unprotect single pages in the KFENCE pool.
> -	 */
> -	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> +	if (can_set_direct_map())
>  		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>  
>  	__create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> index 64e985eaa52d..d107c3d434e2 100644
> --- a/arch/arm64/mm/pageattr.c
> +++ b/arch/arm64/mm/pageattr.c
> @@ -21,7 +21,13 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
>  
>  bool can_set_direct_map(void)
>  {
> -	return rodata_full || debug_pagealloc_enabled();
> +	/*
> +	 * rodata_full, DEBUG_PAGEALLOC and KFENCE require linear map to be
> +	 * mapped at page granularity, so that it is possible to
> +	 * protect/unprotect single pages.
> +	 */
> +	return rodata_full || debug_pagealloc_enabled() ||
> +		IS_ENABLED(CONFIG_KFENCE);
>  }
>  
>  static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
Mike Rapoport Sept. 22, 2022, 4:35 a.m. UTC | #8
On Thu, Sep 22, 2022 at 08:21:38AM +0530, Anshuman Khandual wrote:
> 
> On 9/21/22 20:49, Mike Rapoport wrote:
> > Hi Anshuman,
> > 
> > On Wed, Sep 21, 2022 at 05:09:19PM +0530, Anshuman Khandual wrote:
> >>
> >>
> >> On 9/21/22 13:18, Mike Rapoport wrote:
> >>> From: Mike Rapoport <rppt@linux.ibm.com>
> >>>
> >>> KFENCE requires linear map to be mapped at page granularity, so that it
> >>> is possible to protect/unprotect single pages, just like with
> >>> rodata_full and DEBUG_PAGEALLOC.
> >>>
> >>> Instead of repating
> >>>
> >>> 	can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE)
> >>>
> >>> make can_set_direct_map() handle the KFENCE case.
> >>>
> >>> This also prevents potential false positives in kernel_page_present()
> >>> that may return true for non-present page if CONFIG_KFENCE is enabled.
> >>>
> >>> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> >>> ---
> >>>  arch/arm64/mm/mmu.c      | 8 ++------
> >>>  arch/arm64/mm/pageattr.c | 8 +++++++-
> >>>  2 files changed, 9 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >>> index e7ad44585f40..c5065abec55a 100644
> >>> --- a/arch/arm64/mm/mmu.c
> >>> +++ b/arch/arm64/mm/mmu.c
> >>> @@ -535,7 +535,7 @@ static void __init map_mem(pgd_t *pgdp)
> >>>  	 */
> >>>  	BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end));
> >>>  
> >>> -	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> >>> +	if (can_set_direct_map())
> >>>  		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> >>>  
> >>>  	/*
> >>> @@ -1547,11 +1547,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
> >>>  
> >>>  	VM_BUG_ON(!mhp_range_allowed(start, size, true));
> >>>  
> >>> -	/*
> >>> -	 * KFENCE requires linear map to be mapped at page granularity, so that
> >>> -	 * it is possible to protect/unprotect single pages in the KFENCE pool.
> >>> -	 */
> >>> -	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> >>> +	if (can_set_direct_map())
> >>>  		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> >>>  
> >>>  	__create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
> >>> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> >>> index 64e985eaa52d..d107c3d434e2 100644
> >>> --- a/arch/arm64/mm/pageattr.c
> >>> +++ b/arch/arm64/mm/pageattr.c
> >>> @@ -21,7 +21,13 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
> >>>  
> >>>  bool can_set_direct_map(void)
> >>>  {
> >>> -	return rodata_full || debug_pagealloc_enabled();
> >>> +	/*
> >>> +	 * rodata_full, DEBUG_PAGEALLOC and KFENCE require linear map to be
> >>> +	 * mapped at page granularity, so that it is possible to
> >>> +	 * protect/unprotect single pages.
> >>> +	 */
> >>> +	return rodata_full || debug_pagealloc_enabled() ||
> >>> +		IS_ENABLED(CONFIG_KFENCE);
> >>>  }
> >>
> >> Changing can_set_direct_map() also changes behaviour for other functions such as 
> >>
> >> set_direct_map_default_noflush()
> >> set_direct_map_invalid_noflush()
> >> __kernel_map_pages()
> >>
> >> Is that okay ?
> >  
> > Yes. Since KFENCE disables block mappings, these will actually change the
> > page tables.
> > Actually, before this change the test for can_set_direct_map() in these
> > functions was false negative when CONFIG_KFENCE=y
> 
> Okay but then should not this have a "Fixes:" tag as well ?
 
I feel that this is more of a theoretical bug and it's not worth
backporting to stable.
   
> >>>  static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
> >
Catalin Marinas Sept. 29, 2022, 5:54 p.m. UTC | #9
On Wed, 21 Sep 2022 10:48:41 +0300, Mike Rapoport wrote:
> From: Mike Rapoport <rppt@linux.ibm.com>
> 
> KFENCE requires linear map to be mapped at page granularity, so that it
> is possible to protect/unprotect single pages, just like with
> rodata_full and DEBUG_PAGEALLOC.
> 
> Instead of repating
> 
> [...]

Applied to arm64 (for-next/misc), thanks!

[1/1] arm64/mm: fold check for KFENCE into can_set_direct_map()
      https://git.kernel.org/arm64/c/b9dd04a20f81
diff mbox series

Patch

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index e7ad44585f40..c5065abec55a 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -535,7 +535,7 @@  static void __init map_mem(pgd_t *pgdp)
 	 */
 	BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end));
 
-	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
+	if (can_set_direct_map())
 		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
 
 	/*
@@ -1547,11 +1547,7 @@  int arch_add_memory(int nid, u64 start, u64 size,
 
 	VM_BUG_ON(!mhp_range_allowed(start, size, true));
 
-	/*
-	 * KFENCE requires linear map to be mapped at page granularity, so that
-	 * it is possible to protect/unprotect single pages in the KFENCE pool.
-	 */
-	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
+	if (can_set_direct_map())
 		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
 
 	__create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index 64e985eaa52d..d107c3d434e2 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c
@@ -21,7 +21,13 @@  bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
 
 bool can_set_direct_map(void)
 {
-	return rodata_full || debug_pagealloc_enabled();
+	/*
+	 * rodata_full, DEBUG_PAGEALLOC and KFENCE require linear map to be
+	 * mapped at page granularity, so that it is possible to
+	 * protect/unprotect single pages.
+	 */
+	return rodata_full || debug_pagealloc_enabled() ||
+		IS_ENABLED(CONFIG_KFENCE);
 }
 
 static int change_page_range(pte_t *ptep, unsigned long addr, void *data)