[RFC PATCH] riscv: mm: Ensure prot of VM_WRITE and VM_EXEC must be readable

[RFC PATCH] riscv: mm: Ensure prot of VM_WRITE and VM_EXEC must be readable

[Woodrow Shen]

From: Hsieh-Tseng Shen <woodrow.shen@sifive.com>

The commit 8aeb7b17f04e ("RISC-V: Make mmap() with PROT_WRITE imply PROT_READ")
allows riscv to use mmap with PROT_WRITE only, and meanwhile mmap with w+x is
also permitted. However, when userspace tries to access this page with
PROT_WRITE|PROT_EXEC, which causes infinite loop at load page fault as well as
it triggers soft lockup. According to riscv privileged spec, 
"Writable pages must also be marked readable". The fix to drop the
`PAGE_COPY_EXEC` and then `PAGE_COPY_READ_EXEC` should be just used instead.
This aligns the other arches (i.e arm64) for protection_map.

Fixes: 8aeb7b17f04e ("RISC-V: Make mmap() with PROT_WRITE imply PROT_READ")
Signed-off-by: Hsieh-Tseng Shen <woodrow.shen@sifive.com>
---
 arch/riscv/include/asm/pgtable.h | 1 -
 arch/riscv/mm/init.c             | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index f641837ccf31..bb1e05367739 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -165,7 +165,6 @@ extern struct pt_alloc_ops pt_ops __initdata;
 					 _PAGE_EXEC | _PAGE_WRITE)
 
 #define PAGE_COPY		PAGE_READ
-#define PAGE_COPY_EXEC		PAGE_EXEC
 #define PAGE_COPY_READ_EXEC	PAGE_READ_EXEC
 #define PAGE_SHARED		PAGE_WRITE
 #define PAGE_SHARED_EXEC	PAGE_WRITE_EXEC
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 0f14f4a8d179..8b8c6ad85fdb 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -285,7 +285,7 @@ static const pgprot_t protection_map[16] = {
 	[VM_WRITE | VM_READ]				= PAGE_COPY,
 	[VM_EXEC]					= PAGE_EXEC,
 	[VM_EXEC | VM_READ]				= PAGE_READ_EXEC,
-	[VM_EXEC | VM_WRITE]				= PAGE_COPY_EXEC,
+	[VM_EXEC | VM_WRITE]				= PAGE_COPY_READ_EXEC,
 	[VM_EXEC | VM_WRITE | VM_READ]			= PAGE_COPY_READ_EXEC,
 	[VM_SHARED]					= PAGE_NONE,
 	[VM_SHARED | VM_READ]				= PAGE_READ,
-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATCH] riscv: mm: Ensure prot of VM_WRITE and VM_EXEC must be readable

[Alexandre Ghiti]

Hi Woodrow,

On Fri, Apr 21, 2023 at 9:51 AM Woodrow Shen <woodrow.shen@sifive.com> wrote:
>
> From: Hsieh-Tseng Shen <woodrow.shen@sifive.com>
>
> The commit 8aeb7b17f04e ("RISC-V: Make mmap() with PROT_WRITE imply PROT_READ")
> allows riscv to use mmap with PROT_WRITE only, and meanwhile mmap with w+x is
> also permitted. However, when userspace tries to access this page with
> PROT_WRITE|PROT_EXEC, which causes infinite loop at load page fault as well as
> it triggers soft lockup. According to riscv privileged spec,
> "Writable pages must also be marked readable". The fix to drop the
> `PAGE_COPY_EXEC` and then `PAGE_COPY_READ_EXEC` should be just used instead.
> This aligns the other arches (i.e arm64) for protection_map.
>
> Fixes: 8aeb7b17f04e ("RISC-V: Make mmap() with PROT_WRITE imply PROT_READ")
> Signed-off-by: Hsieh-Tseng Shen <woodrow.shen@sifive.com>
> ---
>  arch/riscv/include/asm/pgtable.h | 1 -
>  arch/riscv/mm/init.c             | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index f641837ccf31..bb1e05367739 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -165,7 +165,6 @@ extern struct pt_alloc_ops pt_ops __initdata;
>                                          _PAGE_EXEC | _PAGE_WRITE)
>
>  #define PAGE_COPY              PAGE_READ
> -#define PAGE_COPY_EXEC         PAGE_EXEC
>  #define PAGE_COPY_READ_EXEC    PAGE_READ_EXEC
>  #define PAGE_SHARED            PAGE_WRITE
>  #define PAGE_SHARED_EXEC       PAGE_WRITE_EXEC
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 0f14f4a8d179..8b8c6ad85fdb 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -285,7 +285,7 @@ static const pgprot_t protection_map[16] = {
>         [VM_WRITE | VM_READ]                            = PAGE_COPY,
>         [VM_EXEC]                                       = PAGE_EXEC,
>         [VM_EXEC | VM_READ]                             = PAGE_READ_EXEC,
> -       [VM_EXEC | VM_WRITE]                            = PAGE_COPY_EXEC,
> +       [VM_EXEC | VM_WRITE]                            = PAGE_COPY_READ_EXEC,
>         [VM_EXEC | VM_WRITE | VM_READ]                  = PAGE_COPY_READ_EXEC,
>         [VM_SHARED]                                     = PAGE_NONE,
>         [VM_SHARED | VM_READ]                           = PAGE_READ,
> --
> 2.34.1
>

This looks sane, and it aligns the behaviour with VM_SHARED | VM_EXEC
| VM_WRITE which implies read. One nit though: since PAGE_COPY_EXEC is
not used anymore, I would rename PAGE_COPY_READ_EXEC into
PAGE_COPY_EXEC and remove PAGE_COPY_READ_EXEC (so that PAGE_COPY_EXEC
is the equivalent of PAGE_SHARED_EXEC).

So you can add in your next version:

Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>

Thanks!

Alex

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATCH] riscv: mm: Ensure prot of VM_WRITE and VM_EXEC must be readable

[Woodrow Shen]

Hi Alexandre,

On Mon, Apr 24, 2023 at 3:47 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> Hi Woodrow,
>
> On Fri, Apr 21, 2023 at 9:51 AM Woodrow Shen <woodrow.shen@sifive.com> wrote:
> >
> > From: Hsieh-Tseng Shen <woodrow.shen@sifive.com>
> >
> > The commit 8aeb7b17f04e ("RISC-V: Make mmap() with PROT_WRITE imply PROT_READ")
> > allows riscv to use mmap with PROT_WRITE only, and meanwhile mmap with w+x is
> > also permitted. However, when userspace tries to access this page with
> > PROT_WRITE|PROT_EXEC, which causes infinite loop at load page fault as well as
> > it triggers soft lockup. According to riscv privileged spec,
> > "Writable pages must also be marked readable". The fix to drop the
> > `PAGE_COPY_EXEC` and then `PAGE_COPY_READ_EXEC` should be just used instead.
> > This aligns the other arches (i.e arm64) for protection_map.
> >
> > Fixes: 8aeb7b17f04e ("RISC-V: Make mmap() with PROT_WRITE imply PROT_READ")
> > Signed-off-by: Hsieh-Tseng Shen <woodrow.shen@sifive.com>
> > ---
> >  arch/riscv/include/asm/pgtable.h | 1 -
> >  arch/riscv/mm/init.c             | 2 +-
> >  2 files changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index f641837ccf31..bb1e05367739 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -165,7 +165,6 @@ extern struct pt_alloc_ops pt_ops __initdata;
> >                                          _PAGE_EXEC | _PAGE_WRITE)
> >
> >  #define PAGE_COPY              PAGE_READ
> > -#define PAGE_COPY_EXEC         PAGE_EXEC
> >  #define PAGE_COPY_READ_EXEC    PAGE_READ_EXEC
> >  #define PAGE_SHARED            PAGE_WRITE
> >  #define PAGE_SHARED_EXEC       PAGE_WRITE_EXEC
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index 0f14f4a8d179..8b8c6ad85fdb 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -285,7 +285,7 @@ static const pgprot_t protection_map[16] = {
> >         [VM_WRITE | VM_READ]                            = PAGE_COPY,
> >         [VM_EXEC]                                       = PAGE_EXEC,
> >         [VM_EXEC | VM_READ]                             = PAGE_READ_EXEC,
> > -       [VM_EXEC | VM_WRITE]                            = PAGE_COPY_EXEC,
> > +       [VM_EXEC | VM_WRITE]                            = PAGE_COPY_READ_EXEC,
> >         [VM_EXEC | VM_WRITE | VM_READ]                  = PAGE_COPY_READ_EXEC,
> >         [VM_SHARED]                                     = PAGE_NONE,
> >         [VM_SHARED | VM_READ]                           = PAGE_READ,
> > --
> > 2.34.1
> >
>
> This looks sane, and it aligns the behaviour with VM_SHARED | VM_EXEC
> | VM_WRITE which implies read. One nit though: since PAGE_COPY_EXEC is
> not used anymore, I would rename PAGE_COPY_READ_EXEC into
> PAGE_COPY_EXEC and remove PAGE_COPY_READ_EXEC (so that PAGE_COPY_EXEC
> is the equivalent of PAGE_SHARED_EXEC).
>
> So you can add in your next version:

Thanks for the advice, I'll update the next version soon.
Woodrow

>
>
> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
>
> Thanks!
>
> Alex

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv