riscv: mmap with PROT_WRITE but no PROT_READ is invalid

Commit Message

Celeste Liu May 31, 2022, 7:56 a.m. UTC

As mentioned in Table 4.5 in RISC-V spec Volume 2 Section 4.3, write
but not read is "Reserved for future use.". For now, they are not valid.
In the current code, -wx is marked as invalid, but -w- is not marked
as invalid.
This patch refines that judgment.

Reported-by: xctan <[e-mail]>
Co-developed-by: dram <[e-mail]>
Signed-off-by: dram <[e-mail]>
Co-developed-by: Ruizhe Pan <[e-mail]>
Signed-off-by: Ruizhe Pan <[e-mail]>
Signed-off-by: Celeste Liu <[e-mail]>
Cc: [e-mail]
Cc: Palmer Dabbelt <[e-mail]>
---
 arch/riscv/kernel/sys_riscv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Palmer Dabbelt July 21, 2022, 11:19 p.m. UTC | #1

On Tue, 31 May 2022 00:56:52 PDT (-0700), [e-mail] wrote:
> As mentioned in Table 4.5 in RISC-V spec Volume 2 Section 4.3, write
> but not read is "Reserved for future use.". For now, they are not valid.
> In the current code, -wx is marked as invalid, but -w- is not marked
> as invalid.
> This patch refines that judgment.
>
> Reported-by: xctan <[e-mail]>
> Co-developed-by: dram <[e-mail]>
> Signed-off-by: dram <[e-mail]>
> Co-developed-by: Ruizhe Pan <[e-mail]>
> Signed-off-by: Ruizhe Pan <[e-mail]>
> Signed-off-by: Celeste Liu <[e-mail]>
> Cc: [e-mail]
> Cc: Palmer Dabbelt <[e-mail]>
> ---
>  arch/riscv/kernel/sys_riscv.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> index 12f8a7fce78b..8a7880b9c433 100644
> --- a/arch/riscv/kernel/sys_riscv.c
> +++ b/arch/riscv/kernel/sys_riscv.c
> @@ -18,9 +18,8 @@ static long riscv_sys_mmap(unsigned long addr, unsigned long len,
>  	if (unlikely(offset & (~PAGE_MASK >> page_shift_offset)))
>  		return -EINVAL;
>
> -	if ((prot & PROT_WRITE) && (prot & PROT_EXEC))
> -		if (unlikely(!(prot & PROT_READ)))
> -			return -EINVAL;
> +	if (unlikely((prot & PROT_WRITE) && !(prot & PROT_READ)))
> +		return -EINVAL;
>
>  	return ksys_mmap_pgoff(addr, len, prot, flags, fd,
>  			       offset >> (PAGE_SHIFT - page_shift_offset));

Thanks, this is on for-next.

Eva Kotova Oct. 6, 2022, 7:17 p.m. UTC | #2

On Tue, 31 May 2022 00:56:52 PDT (-0700), [e-mail] wrote:
 > As mentioned in Table 4.5 in RISC-V spec Volume 2 Section 4.3, write
 > but not read is "Reserved for future use.". For now, they are not valid.
 > In the current code, -wx is marked as invalid, but -w- is not marked
 > as invalid.

This patch breaks OpenJDK/Java on RISC-V, as it tries to create a w-only 
protective page:

#
# There is insufficient memory for the Java Runtime Environment to continue.
# Native memory allocation (mmap) failed to map 4096 bytes for failed to 
allocate memory for PaX check.
# An error report file with more information is saved as:
# /root/hs_err_pid107.log

I bisected to this commit since on Linux 5.19+ java no longer works.
Perhaps some fallback should be implemented, to prevent userspace 
breakage. It is currently documented, that at least on i386 PROT_WRITE 
mappings imply PROT_READ (See man mmap(2) NOTES), this would be a good 
place to start.

Best regards,
Eva

Eva Kotova Oct. 6, 2022, 7:20 p.m. UTC | #3

On Tue, 31 May 2022 00:56:52 PDT (-0700), [e-mail] wrote:
 > As mentioned in Table 4.5 in RISC-V spec Volume 2 Section 4.3, write
 > but not read is "Reserved for future use.". For now, they are not valid.
 > In the current code, -wx is marked as invalid, but -w- is not marked
 > as invalid.

This patch breaks OpenJDK/Java on RISC-V, as it tries to create a w-only 
protective page:

#
# There is insufficient memory for the Java Runtime Environment to continue.
# Native memory allocation (mmap) failed to map 4096 bytes for failed to 
allocate memory for PaX check.
# An error report file with more information is saved as:
# /root/hs_err_pid107.log

I bisected to this commit since on Linux 5.19+ java no longer works.
Perhaps some fallback should be implemented, to prevent userspace 
breakage. It is currently documented, that at least on i386 PROT_WRITE 
mappings imply PROT_READ (See man mmap(2) NOTES), this would be a good 
place to start.

Best regards,
Eva

Conor Dooley Oct. 6, 2022, 7:26 p.m. UTC | #4

Hey Eva,

On Thu, Oct 06, 2022 at 10:20:02PM +0300, Eva Kotova wrote:
> On Tue, 31 May 2022 00:56:52 PDT (-0700), [e-mail] wrote:
> > As mentioned in Table 4.5 in RISC-V spec Volume 2 Section 4.3, write
> > but not read is "Reserved for future use.". For now, they are not valid.
> > In the current code, -wx is marked as invalid, but -w- is not marked
> > as invalid.
> 
> This patch breaks OpenJDK/Java on RISC-V, as it tries to create a w-only
> protective page:
> 
> #
> # There is insufficient memory for the Java Runtime Environment to continue.
> # Native memory allocation (mmap) failed to map 4096 bytes for failed to
> allocate memory for PaX check.
> # An error report file with more information is saved as:
> # /root/hs_err_pid107.log
> 
> I bisected to this commit since on Linux 5.19+ java no longer works.
> Perhaps some fallback should be implemented, to prevent userspace breakage.
> It is currently documented, that at least on i386 PROT_WRITE mappings imply
> PROT_READ (See man mmap(2) NOTES), this would be a good place to start.

Do these patches solve your problem by any chance?
https://lore.kernel.org/linux-riscv/[e-mail]/

I don't know the "area" at all, so it's a shot in the dark, but these
both have Fixes: tags for the patch that you are blaming.

Thanks,
Conor.

Conor Dooley Oct. 6, 2022, 7:29 p.m. UTC | #5

Hey Eva,
Resending as I think I may have replied to a mail with an invalid
reply-to address?

On Thu, Oct 06, 2022 at 10:20:02PM +0300, Eva Kotova wrote:
> On Tue, 31 May 2022 00:56:52 PDT (-0700), [e-mail] wrote:
>> As mentioned in Table 4.5 in RISC-V spec Volume 2 Section 4.3, write
>> but not read is "Reserved for future use.". For now, they are not valid.
>> In the current code, -wx is marked as invalid, but -w- is not marked
>> as invalid.
>
> This patch breaks OpenJDK/Java on RISC-V, as it tries to create a w-only
> protective page:
>
> #
> # There is insufficient memory for the Java Runtime Environment to continue.
> # Native memory allocation (mmap) failed to map 4096 bytes for failed to
> allocate memory for PaX check.
> # An error report file with more information is saved as:
> # /root/hs_err_pid107.log
>
> I bisected to this commit since on Linux 5.19+ java no longer works.
> Perhaps some fallback should be implemented, to prevent userspace breakage.
> It is currently documented, that at least on i386 PROT_WRITE mappings imply
> PROT_READ (See man mmap(2) NOTES), this would be a good place to start.

Do these patches solve your problem by any chance?
https://lore.kernel.org/linux-riscv/[e-mail]/

I don't know the "area" at all, so it's a shot in the dark, but these
both have Fixes: tags for the patch that you are blaming.

Thanks,
Conor.

Eva Kotova Oct. 6, 2022, 7:55 p.m. UTC | #6

Patch "[PATCH v4 2/2] riscv: Allow PROT_WRITE-only mmap()" applied 
cleanly over 5.19, fixed issues with OpenJDK.

I assume this is not yet merged into linux-next, because problem 
persists there, hope this gets merged soon.

Thanks,
Eva

Conor Dooley Oct. 6, 2022, 8:03 p.m. UTC | #7

On Thu, Oct 06, 2022 at 10:55:00PM +0300, Eva Kotova wrote:
> Patch "[PATCH v4 2/2] riscv: Allow PROT_WRITE-only mmap()" applied cleanly
> over 5.19, fixed issues with OpenJDK.
> 
> I assume this is not yet merged into linux-next, because problem persists
> there, hope this gets merged soon.

Yeah, not been applied yet. If you reply with a Tested-by that will help
though! Would be good to also note that it breaks userspace.
FYI, you've got an issue with your mail client, my msg-id from my last
mail (Yz8r71PjHlpLy+kR@spud) ended up as the reply-to address for this
one. The first email you sent tonight was fine though.

Thanks,
Conor.

Heinrich Schuchardt Oct. 11, 2022, 11:23 a.m. UTC | #8

On 10/6/22 21:17, Eva Kotova wrote:
> On Tue, 31 May 2022 00:56:52 PDT (-0700), [e-mail] wrote:
>  > As mentioned in Table 4.5 in RISC-V spec Volume 2 Section 4.3, write
>  > but not read is "Reserved for future use.". For now, they are not valid.
>  > In the current code, -wx is marked as invalid, but -w- is not marked
>  > as invalid.
> 
> This patch breaks OpenJDK/Java on RISC-V, as it tries to create a w-only 
> protective page:
> 
> #
> # There is insufficient memory for the Java Runtime Environment to 
> continue.
> # Native memory allocation (mmap) failed to map 4096 bytes for failed to 
> allocate memory for PaX check.
> # An error report file with more information is saved as:
> # /root/hs_err_pid107.log
> 
> I bisected to this commit since on Linux 5.19+ java no longer works.
> Perhaps some fallback should be implemented, to prevent userspace 
> breakage. It is currently documented, that at least on i386 PROT_WRITE 
> mappings imply PROT_READ (See man mmap(2) NOTES), this would be a good 
> place to start.

Which test case demonstrates the issue?

Best regards

Heinrich

> 
> Best regards,
> Eva
> 
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>

Celeste Liu Oct. 11, 2022, 11:31 a.m. UTC | #9

On 2022/10/11 19:23, Heinrich Schuchardt wrote:

> 
> On 10/6/22 21:17, Eva Kotova wrote:
>> On Tue, 31 May 2022 00:56:52 PDT (-0700), [e-mail] wrote:
>>  > As mentioned in Table 4.5 in RISC-V spec Volume 2 Section 4.3, write
>>  > but not read is "Reserved for future use.". For now, they are not 
>> valid.
>>  > In the current code, -wx is marked as invalid, but -w- is not marked
>>  > as invalid.
>>
>> This patch breaks OpenJDK/Java on RISC-V, as it tries to create a 
>> w-only protective page:
>>
>> #
>> # There is insufficient memory for the Java Runtime Environment to 
>> continue.
>> # Native memory allocation (mmap) failed to map 4096 bytes for failed 
>> to allocate memory for PaX check.
>> # An error report file with more information is saved as:
>> # /root/hs_err_pid107.log
>>
>> I bisected to this commit since on Linux 5.19+ java no longer works.
>> Perhaps some fallback should be implemented, to prevent userspace 
>> breakage. It is currently documented, that at least on i386 PROT_WRITE 
>> mappings imply PROT_READ (See man mmap(2) NOTES), this would be a good 
>> place to start.
> 
> Which test case demonstrates the issue?
> 
> Best regards
> 
> Heinrich
> 

In check_pax function[1], jdk use mmap with PROT_WRITE.

    void* p = ::mmap(NULL, size, PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,0);


[1]: https://github.com/openjdk/jdk/blob/f694f8a7671002559e7d23fdb65d5e9c768f9c03/src/hotspot/os/linux/os_linux.cpp#L4306

Best regards,
Celeste

Patch

diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
index 12f8a7fce78b..8a7880b9c433 100644
--- a/arch/riscv/kernel/sys_riscv.c
+++ b/arch/riscv/kernel/sys_riscv.c
@@ -18,9 +18,8 @@  static long riscv_sys_mmap(unsigned long addr, unsigned long len,
 	if (unlikely(offset & (~PAGE_MASK >> page_shift_offset)))
 		return -EINVAL;
 
-	if ((prot & PROT_WRITE) && (prot & PROT_EXEC))
-		if (unlikely(!(prot & PROT_READ)))
-			return -EINVAL;
+	if (unlikely((prot & PROT_WRITE) && !(prot & PROT_READ)))
+		return -EINVAL;
 
 	return ksys_mmap_pgoff(addr, len, prot, flags, fd,
 			       offset >> (PAGE_SHIFT - page_shift_offset));