linux-riscv.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/2] Make mmap() with PROT_WRITE imply PROT_READ
@ 2022-09-15 19:37 Andrew Bresticker
  2022-09-15 19:37 ` [PATCH v4 1/2] riscv: Make VM_WRITE imply VM_READ Andrew Bresticker
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Andrew Bresticker @ 2022-09-15 19:37 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Paul Walmsley, Celeste Liu, dram, Ruizhe Pan, Conor.Dooley,
	linux-riscv, linux-kernel, Andrew Bresticker

Commit 2139619bcad7 ("riscv: mmap with PROT_WRITE but no PROT_READ is
invalid") made mmap() reject mappings with only PROT_WRITE set in an
attempt to fix an observed inconsistency in behavior when attempting
to read from a PROT_WRITE-only mapping. The root cause of this behavior
was actually that while RISC-V's protection_map maps VM_WRITE to
readable PTE permissions (since write-only PTEs are considered reserved
by the privileged spec), the page fault handler considered loads from
VM_WRITE-only VMAs illegal accesses. Fix the underlying cause by
handling faults in VM_WRITE-only VMAs (patch 1) and then re-enable
use of mmap(PROT_WRITE) (patch 2), making RISC-V's behavior consistent
with all other architectures that don't support write-only PTEs.

Both patches are tagged as fixes for the aforementioned commit since that
commit made a userspace visible change that will break any software relying
on mmap(PROT_WRITE). (Also cc: stable since the offending commit was
itself backported to stable).

v1 -> v2: Allow handling of load faults in VM_WRITE VMAs
v2 -> v3: Split into two pathces
v3 -> v4: Fixes tags (+ this cover letter)

Andrew Bresticker (2):
  riscv: Make VM_WRITE imply VM_READ
  riscv: Allow PROT_WRITE-only mmap()

 arch/riscv/kernel/sys_riscv.c | 3 ---
 arch/riscv/mm/fault.c         | 3 ++-
 2 files changed, 2 insertions(+), 4 deletions(-)

-- 
2.25.1


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v4 1/2] riscv: Make VM_WRITE imply VM_READ
  2022-09-15 19:37 [PATCH v4 0/2] Make mmap() with PROT_WRITE imply PROT_READ Andrew Bresticker
@ 2022-09-15 19:37 ` Andrew Bresticker
  2022-09-15 19:37 ` [PATCH v4 2/2] riscv: Allow PROT_WRITE-only mmap() Andrew Bresticker
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Andrew Bresticker @ 2022-09-15 19:37 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Paul Walmsley, Celeste Liu, dram, Ruizhe Pan, Conor.Dooley,
	linux-riscv, linux-kernel, Andrew Bresticker, stable,
	Atish Patra

RISC-V does not presently have write-only mappings as that PTE bit pattern
is considered reserved in the privileged spec, so allow handling of read
faults in VMAs that have VM_WRITE without VM_READ in order to be consistent
with other architectures that have similar limitations.

Fixes: 2139619bcad7 ("riscv: mmap with PROT_WRITE but no PROT_READ is invalid")
Cc: <stable@vger.kernel.org> # v4.19+
Reviewed-by: Atish Patra <atishp@rivosinc.com>
Signed-off-by: Andrew Bresticker <abrestic@rivosinc.com>
---
new in v3
v3 -> v4: add Fixes tag
---
 arch/riscv/mm/fault.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c
index f2fbd1400b7c..d86f7cebd4a7 100644
--- a/arch/riscv/mm/fault.c
+++ b/arch/riscv/mm/fault.c
@@ -184,7 +184,8 @@ static inline bool access_error(unsigned long cause, struct vm_area_struct *vma)
 		}
 		break;
 	case EXC_LOAD_PAGE_FAULT:
-		if (!(vma->vm_flags & VM_READ)) {
+		/* Write implies read */
+		if (!(vma->vm_flags & (VM_READ | VM_WRITE))) {
 			return true;
 		}
 		break;
-- 
2.25.1


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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v4 2/2] riscv: Allow PROT_WRITE-only mmap()
  2022-09-15 19:37 [PATCH v4 0/2] Make mmap() with PROT_WRITE imply PROT_READ Andrew Bresticker
  2022-09-15 19:37 ` [PATCH v4 1/2] riscv: Make VM_WRITE imply VM_READ Andrew Bresticker
@ 2022-09-15 19:37 ` Andrew Bresticker
  2022-10-11 17:04 ` [PATCH v4 0/2] Make mmap() with PROT_WRITE imply PROT_READ Conor Dooley
  2022-10-13 21:01 ` Palmer Dabbelt
  3 siblings, 0 replies; 5+ messages in thread
From: Andrew Bresticker @ 2022-09-15 19:37 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Paul Walmsley, Celeste Liu, dram, Ruizhe Pan, Conor.Dooley,
	linux-riscv, linux-kernel, Andrew Bresticker, stable,
	Atish Patra

Commit 2139619bcad7 ("riscv: mmap with PROT_WRITE but no PROT_READ is
invalid") made mmap() return EINVAL if PROT_WRITE was set wihtout
PROT_READ with the justification that a write-only PTE is considered a
reserved PTE permission bit pattern in the privileged spec. This check
is unnecessary since we let VM_WRITE imply VM_READ on RISC-V, and it is
inconsistent with other architectures that don't support write-only PTEs,
creating a potential software portability issue. Just remove the check
altogether and let PROT_WRITE imply PROT_READ as is the case on other
architectures.

Note that this also allows PROT_WRITE|PROT_EXEC mappings which were
disallowed prior to the aforementioned commit; PROT_READ is implied in
such mappings as well.

Fixes: 2139619bcad7 ("riscv: mmap with PROT_WRITE but no PROT_READ is invalid")
Cc: <stable@vger.kernel.org> # v4.19+
Reviewed-by: Atish Patra <atishp@rivosinc.com>
Signed-off-by: Andrew Bresticker <abrestic@rivosinc.com>
---
v1 -> v2: Update access_error() to account for write-implies-read
v2 -> v3: Separate into two commits
---
 arch/riscv/kernel/sys_riscv.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
index 571556bb9261..5d3f2fbeb33c 100644
--- a/arch/riscv/kernel/sys_riscv.c
+++ b/arch/riscv/kernel/sys_riscv.c
@@ -18,9 +18,6 @@ static long riscv_sys_mmap(unsigned long addr, unsigned long len,
 	if (unlikely(offset & (~PAGE_MASK >> page_shift_offset)))
 		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));
 }
-- 
2.25.1


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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v4 0/2] Make mmap() with PROT_WRITE imply PROT_READ
  2022-09-15 19:37 [PATCH v4 0/2] Make mmap() with PROT_WRITE imply PROT_READ Andrew Bresticker
  2022-09-15 19:37 ` [PATCH v4 1/2] riscv: Make VM_WRITE imply VM_READ Andrew Bresticker
  2022-09-15 19:37 ` [PATCH v4 2/2] riscv: Allow PROT_WRITE-only mmap() Andrew Bresticker
@ 2022-10-11 17:04 ` Conor Dooley
  2022-10-13 21:01 ` Palmer Dabbelt
  3 siblings, 0 replies; 5+ messages in thread
From: Conor Dooley @ 2022-10-11 17:04 UTC (permalink / raw)
  To: Andrew Bresticker
  Cc: Palmer Dabbelt, Paul Walmsley, Celeste Liu, dram, Ruizhe Pan,
	Conor.Dooley, linux-riscv, linux-kernel

Hey Palmer,

On Thu, Sep 15, 2022 at 03:37:00PM -0400, Andrew Bresticker wrote:
> Commit 2139619bcad7 ("riscv: mmap with PROT_WRITE but no PROT_READ is
> invalid") made mmap() reject mappings with only PROT_WRITE set in an
> attempt to fix an observed inconsistency in behavior when attempting
> to read from a PROT_WRITE-only mapping. The root cause of this behavior
> was actually that while RISC-V's protection_map maps VM_WRITE to
> readable PTE permissions (since write-only PTEs are considered reserved
> by the privileged spec), the page fault handler considered loads from
> VM_WRITE-only VMAs illegal accesses. Fix the underlying cause by
> handling faults in VM_WRITE-only VMAs (patch 1) and then re-enable
> use of mmap(PROT_WRITE) (patch 2), making RISC-V's behavior consistent
> with all other architectures that don't support write-only PTEs.
> 
> Both patches are tagged as fixes for the aforementioned commit since that
> commit made a userspace visible change that will break any software relying
> on mmap(PROT_WRITE). (Also cc: stable since the offending commit was
> itself backported to stable).

The patch that these commits fix has hit the distros & manifests as a
userspace breakage for openJDK:
https://lore.kernel.org/linux-riscv/a69ee775-e565-3d72-eb5f-8378616694d3@gmail.com/
https://lore.kernel.org/linux-riscv/d6c9e249-08bd-4439-7dcc-371b32e7b851@canonical.com/

Eva tested these patches and reported that their problem was fixed:
https://lore.kernel.org/linux-riscv/20282242-5cad-42be-ce6c-834b0e7ef269@gmail.com/

I asked them for a T-b but I don't see one on lore etc, but it would be
from Eva Kotova <nyandarknessgirl@gmail.com> if you consider their
comments their sufficient for a T-B

Thanks,
Conor.

> 
> v1 -> v2: Allow handling of load faults in VM_WRITE VMAs
> v2 -> v3: Split into two pathces
> v3 -> v4: Fixes tags (+ this cover letter)
> 
> Andrew Bresticker (2):
>   riscv: Make VM_WRITE imply VM_READ
>   riscv: Allow PROT_WRITE-only mmap()
> 
>  arch/riscv/kernel/sys_riscv.c | 3 ---
>  arch/riscv/mm/fault.c         | 3 ++-
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v4 0/2] Make mmap() with PROT_WRITE imply PROT_READ
  2022-09-15 19:37 [PATCH v4 0/2] Make mmap() with PROT_WRITE imply PROT_READ Andrew Bresticker
                   ` (2 preceding siblings ...)
  2022-10-11 17:04 ` [PATCH v4 0/2] Make mmap() with PROT_WRITE imply PROT_READ Conor Dooley
@ 2022-10-13 21:01 ` Palmer Dabbelt
  3 siblings, 0 replies; 5+ messages in thread
From: Palmer Dabbelt @ 2022-10-13 21:01 UTC (permalink / raw)
  To: abrestic
  Cc: Paul Walmsley, coelacanthus, dramforever, c141028, Conor Dooley,
	linux-riscv, linux-kernel, abrestic

On Thu, 15 Sep 2022 12:37:00 PDT (-0700), abrestic@rivosinc.com wrote:
> Commit 2139619bcad7 ("riscv: mmap with PROT_WRITE but no PROT_READ is
> invalid") made mmap() reject mappings with only PROT_WRITE set in an
> attempt to fix an observed inconsistency in behavior when attempting
> to read from a PROT_WRITE-only mapping. The root cause of this behavior
> was actually that while RISC-V's protection_map maps VM_WRITE to
> readable PTE permissions (since write-only PTEs are considered reserved
> by the privileged spec), the page fault handler considered loads from
> VM_WRITE-only VMAs illegal accesses. Fix the underlying cause by
> handling faults in VM_WRITE-only VMAs (patch 1) and then re-enable
> use of mmap(PROT_WRITE) (patch 2), making RISC-V's behavior consistent
> with all other architectures that don't support write-only PTEs.
>
> Both patches are tagged as fixes for the aforementioned commit since that
> commit made a userspace visible change that will break any software relying
> on mmap(PROT_WRITE). (Also cc: stable since the offending commit was
> itself backported to stable).
>
> v1 -> v2: Allow handling of load faults in VM_WRITE VMAs
> v2 -> v3: Split into two pathces
> v3 -> v4: Fixes tags (+ this cover letter)
>
> Andrew Bresticker (2):
>   riscv: Make VM_WRITE imply VM_READ
>   riscv: Allow PROT_WRITE-only mmap()
>
>  arch/riscv/kernel/sys_riscv.c | 3 ---
>  arch/riscv/mm/fault.c         | 3 ++-
>  2 files changed, 2 insertions(+), 4 deletions(-)

Thanks, these are on for-next.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-10-13 21:01 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-15 19:37 [PATCH v4 0/2] Make mmap() with PROT_WRITE imply PROT_READ Andrew Bresticker
2022-09-15 19:37 ` [PATCH v4 1/2] riscv: Make VM_WRITE imply VM_READ Andrew Bresticker
2022-09-15 19:37 ` [PATCH v4 2/2] riscv: Allow PROT_WRITE-only mmap() Andrew Bresticker
2022-10-11 17:04 ` [PATCH v4 0/2] Make mmap() with PROT_WRITE imply PROT_READ Conor Dooley
2022-10-13 21:01 ` Palmer Dabbelt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).