diff mbox series

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

Message ID 20230425034407.1601585-1-woodrow.shen@sifive.com (mailing list archive)
State Superseded
Headers show
Series [RFC,v1] 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 warning WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: 8aeb7b17f04e ("RISC-V: Make mmap() with PROT_WRITE imply PROT_READ")'
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes fail Problems with Fixes tag: 1
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Woodrow Shen April 25, 2023, 3:44 a.m. UTC 
From: Hsieh-Tseng Shen <woodrow.shen@sifive.com>

The commit 8aeb7b1 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_READ_EXEC` and `PAGE_READ_EXEC`
should be just used instead. This aligns the other arches (i.e arm64)
for protection_map.

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

Comments

Conor Dooley April 25, 2023, 6:54 a.m. UTC | #1 
Hey Hsieh-Tseng,

On Tue, Apr 25, 2023 at 11:44:07AM +0800, Woodrow Shen wrote:
> From: Hsieh-Tseng Shen <woodrow.shen@sifive.com>
> 
> The commit 8aeb7b1 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_READ_EXEC` and `PAGE_READ_EXEC`
> should be just used instead. This aligns the other arches (i.e arm64)
> for protection_map.
> 
> Fixes: 8aeb7b1 ("RISC-V: Make mmap() with PROT_WRITE imply PROT_READ")

This fixes tag should be:
Fixes: 8aeb7b17f04e ("RISC-V: Make mmap() with PROT_WRITE imply PROT_READ")
as checkpatch would have told you :/ Perhaps Palmer can fix that up on
application?

> Signed-off-by: Hsieh-Tseng Shen <woodrow.shen@sifive.com>
> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---

This is not v1, it's v2 right?
Please include a changelog between versions.

Also, what makes it an RFC, it's an actual fix that you would like
merged, right?

Thanks,
Conor.

>  arch/riscv/include/asm/pgtable.h | 3 +--
>  arch/riscv/mm/init.c             | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index f641837ccf31..05eda3281ba9 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -165,8 +165,7 @@ 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_COPY_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..cc48b0d93a98 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -286,7 +286,7 @@ static const pgprot_t protection_map[16] = {
>  	[VM_EXEC]					= PAGE_EXEC,
>  	[VM_EXEC | VM_READ]				= PAGE_READ_EXEC,
>  	[VM_EXEC | VM_WRITE]				= PAGE_COPY_EXEC,
> -	[VM_EXEC | VM_WRITE | VM_READ]			= PAGE_COPY_READ_EXEC,
> +	[VM_EXEC | VM_WRITE | VM_READ]			= PAGE_COPY_EXEC,
>  	[VM_SHARED]					= PAGE_NONE,
>  	[VM_SHARED | VM_READ]				= PAGE_READ,
>  	[VM_SHARED | VM_WRITE]				= PAGE_SHARED,
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Woodrow Shen April 25, 2023, 10:10 a.m. UTC | #2 
Hi Conor,

On Tue, Apr 25, 2023 at 2:55 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> Hey Hsieh-Tseng,
>
> On Tue, Apr 25, 2023 at 11:44:07AM +0800, Woodrow Shen wrote:
> > From: Hsieh-Tseng Shen <woodrow.shen@sifive.com>
> >
> > The commit 8aeb7b1 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_READ_EXEC` and `PAGE_READ_EXEC`
> > should be just used instead. This aligns the other arches (i.e arm64)
> > for protection_map.
> >
> > Fixes: 8aeb7b1 ("RISC-V: Make mmap() with PROT_WRITE imply PROT_READ")
>
> This fixes tag should be:
> Fixes: 8aeb7b17f04e ("RISC-V: Make mmap() with PROT_WRITE imply PROT_READ")
> as checkpatch would have told you :/ Perhaps Palmer can fix that up on
> application?
>
> > Signed-off-by: Hsieh-Tseng Shen <woodrow.shen@sifive.com>
> > Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
>
> This is not v1, it's v2 right?
> Please include a changelog between versions.

Yes, it's v2. I copied the outdated comment as my first version was
verified by script correctly. Will fix it in v2.

>
> Also, what makes it an RFC, it's an actual fix that you would like
> merged, right?

I was suggested to use "RFC" internally, so should I just re-submit v2
with [RESEND v2]?
Thanks,
Woodrow

>
> Thanks,
> Conor.
>
> >  arch/riscv/include/asm/pgtable.h | 3 +--
> >  arch/riscv/mm/init.c             | 2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index f641837ccf31..05eda3281ba9 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -165,8 +165,7 @@ 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_COPY_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..cc48b0d93a98 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -286,7 +286,7 @@ static const pgprot_t protection_map[16] = {
> >       [VM_EXEC]                                       = PAGE_EXEC,
> >       [VM_EXEC | VM_READ]                             = PAGE_READ_EXEC,
> >       [VM_EXEC | VM_WRITE]                            = PAGE_COPY_EXEC,
> > -     [VM_EXEC | VM_WRITE | VM_READ]                  = PAGE_COPY_READ_EXEC,
> > +     [VM_EXEC | VM_WRITE | VM_READ]                  = PAGE_COPY_EXEC,
> >       [VM_SHARED]                                     = PAGE_NONE,
> >       [VM_SHARED | VM_READ]                           = PAGE_READ,
> >       [VM_SHARED | VM_WRITE]                          = PAGE_SHARED,
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
Conor Dooley April 25, 2023, 10:19 a.m. UTC | #3 
On Tue, Apr 25, 2023 at 06:10:36PM +0800, Woodrow Shen wrote:
> Hi Conor,
> 
> On Tue, Apr 25, 2023 at 2:55 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > Hey Hsieh-Tseng,
> >
> > On Tue, Apr 25, 2023 at 11:44:07AM +0800, Woodrow Shen wrote:
> > > From: Hsieh-Tseng Shen <woodrow.shen@sifive.com>
> > >
> > > The commit 8aeb7b1 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_READ_EXEC` and `PAGE_READ_EXEC`
> > > should be just used instead. This aligns the other arches (i.e arm64)
> > > for protection_map.
> > >
> > > Fixes: 8aeb7b1 ("RISC-V: Make mmap() with PROT_WRITE imply PROT_READ")
> >
> > This fixes tag should be:
> > Fixes: 8aeb7b17f04e ("RISC-V: Make mmap() with PROT_WRITE imply PROT_READ")
> > as checkpatch would have told you :/ Perhaps Palmer can fix that up on
> > application?
> >
> > > Signed-off-by: Hsieh-Tseng Shen <woodrow.shen@sifive.com>
> > > Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > ---
> >
> > This is not v1, it's v2 right?
> > Please include a changelog between versions.
> 
> Yes, it's v2. I copied the outdated comment as my first version was
> verified by script correctly. Will fix it in v2.
> 
> >
> > Also, what makes it an RFC, it's an actual fix that you would like
> > merged, right?
> 
> I was suggested to use "RFC" internally, so should I just re-submit v2
> with [RESEND v2]?

If you want? It's Palmer's call whether he'd be happy with fixing up the
tag or not, not mine.

If you do resend, it would just be "v2" though IMO.

Cheers,
Conor.
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index f641837ccf31..05eda3281ba9 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -165,8 +165,7 @@  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_COPY_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..cc48b0d93a98 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -286,7 +286,7 @@  static const pgprot_t protection_map[16] = {
 	[VM_EXEC]					= PAGE_EXEC,
 	[VM_EXEC | VM_READ]				= PAGE_READ_EXEC,
 	[VM_EXEC | VM_WRITE]				= PAGE_COPY_EXEC,
-	[VM_EXEC | VM_WRITE | VM_READ]			= PAGE_COPY_READ_EXEC,
+	[VM_EXEC | VM_WRITE | VM_READ]			= PAGE_COPY_EXEC,
 	[VM_SHARED]					= PAGE_NONE,
 	[VM_SHARED | VM_READ]				= PAGE_READ,
 	[VM_SHARED | VM_WRITE]				= PAGE_SHARED,