[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 at rivosinc.com> wrote:
>
> Hi Woodrow,
>
> On Fri, Apr 21, 2023 at 9:51 AM Woodrow Shen <woodrow.shen at sifive.com> wrote:
> >
> > From: Hsieh-Tseng Shen <woodrow.shen at 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 at 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 at rivosinc.com>
>
> Thanks!
>
> Alex