linux-kernel - Re: [PATCH] powerpc: Add set_memory_{p/np}() and remove set_memory

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <984da14c339ae81b45518002081f791153fe69dd.camel@russell.cc>
Date:   Mon, 13 Dec 2021 14:36:47 +1000
From:   Russell Currey <ruscur@...sell.cc>
To:     Christophe Leroy <christophe.leroy@...roup.eu>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>
Cc:     Maxime Bizon <mbizon@...ebox.fr>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] powerpc: Add set_memory_{p/np}() and remove
 set_memory_attr()

On Fri, 2021-12-10 at 08:09 +0000, Christophe Leroy wrote:
> set_memory_attr() was implemented by commit 4d1755b6a762
> ("powerpc/mm:
> implement set_memory_attr()") because the set_memory_xx() couldn't
> be used at that time to modify memory "on the fly" as explained it
> the commit.
> 
> But set_memory_attr() uses set_pte_at() which leads to warnings when
> CONFIG_DEBUG_VM is selected, because set_pte_at() is unexpected for
> updating existing page table entries.
> 
> The check could be bypassed by using __set_pte_at() instead,
> as it was the case before commit c988cfd38e48 ("powerpc/32:
> use set_memory_attr()") but since commit 9f7853d7609d ("powerpc/mm:
> Fix set_memory_*() against concurrent accesses") it is now possible
> to use set_memory_xx() functions to update page table entries
> "on the fly" because the update is now atomic.
> 
> For DEBUG_PAGEALLOC we need to clear and set back _PAGE_PRESENT.
> Add set_memory_np() and set_memory_p() for that.
> 
> Replace all uses of set_memory_attr() by the relevant set_memory_xx()
> and remove set_memory_attr().
> 
> Reported-by: Maxime Bizon <mbizon@...ebox.fr>
> Fixes: c988cfd38e48 ("powerpc/32: use set_memory_attr()")
> Cc: stable@...r.kernel.org
> Depends-on: 9f7853d7609d ("powerpc/mm: Fix set_memory_*() against
> concurrent accesses")
> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>

Reviewed-by: Russell Currey <ruscur@...sell.cc>

One comment below:

> diff --git a/arch/powerpc/include/asm/set_memory.h
> b/arch/powerpc/include/asm/set_memory.h
> index b040094f7920..061f1766a8a4 100644
> --- a/arch/powerpc/include/asm/set_memory.h
> +++ b/arch/powerpc/include/asm/set_memory.h
> @@ -6,6 +6,8 @@
>  #define SET_MEMORY_RW  1
>  #define SET_MEMORY_NX  2
>  #define SET_MEMORY_X   3
> +#define SET_MEMORY_NP  4
> +#define SET_MEMORY_P   5

It might be nice to have a comment somewhere in set_memory.h explaining
that {p/np} = present/not present.  RO/RW/NX/X are commonly used, "p"
as shorthand for "present" is less obvious.  x86's set_memory.h has a
nice comment covering everything as an example.

>  int change_memory_attr(unsigned long addr, int numpages, long
> action);
>  
> @@ -29,6 +31,14 @@ static inline int set_memory_x(unsigned long addr,
> int numpages)
>         return change_memory_attr(addr, numpages, SET_MEMORY_X);
>  }
>  
> -int set_memory_attr(unsigned long addr, int numpages, pgprot_t
> prot);
> +static inline int set_memory_np(unsigned long addr, int numpages)
> +{
> +       return change_memory_attr(addr, numpages, SET_MEMORY_NP);
> +}
> +
> +static inline int set_memory_p(unsigned long addr, int numpages)
> +{
> +       return change_memory_attr(addr, numpages, SET_MEMORY_P);
> +}
>  
>  #endif