diff mbox series

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

Message ID 20230421075111.1391952-1-woodrow.shen@sifive.com (mailing list archive)
State Superseded
Headers show
Series [RFC] riscv: mm: Ensure prot of VM_WRITE and VM_EXEC must be readable | expand

Checks

Context Check Description
conchuod/cover_letter success Single patches do not need cover letters
conchuod/tree_selection success Guessed tree name to be fixes at HEAD 1b50f956c8fe
conchuod/fixes_present success Fixes tag present in non-next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 1 and now 1
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 3288 this patch: 3288
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 17609 this patch: 17609
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 3 this patch: 3
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 15 lines checked
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success Fixes tag looks correct
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Woodrow Shen April 21, 2023, 7:51 a.m. UTC 
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(-)

Comments

Alexandre Ghiti April 24, 2023, 7:46 a.m. UTC | #1 
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
Woodrow Shen April 25, 2023, 3:35 a.m. UTC | #2 
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
diff mbox series

Patch

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,