lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 9 Sep 2022 11:52:01 -0700 From: Atish Patra <atishp@...shpatra.org> To: Andrew Bresticker <abrestic@...osinc.com> Cc: Palmer Dabbelt <palmer@...belt.com>, Paul Walmsley <paul.walmsley@...ive.com>, Celeste Liu <coelacanthus@...look.com>, dram <dramforever@...e.com>, Ruizhe Pan <c141028@...il.com>, linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH v2] riscv: Make mmap() with PROT_WRITE imply PROT_READ On Thu, Sep 8, 2022 at 11:50 AM Andrew Bresticker <abrestic@...osinc.com> wrote: > > 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 RISC-V defines its protection_map such that PROT_WRITE > maps to the same PTE permissions as PROT_WRITE|PROT_READ, 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") > Signed-off-by: Andrew Bresticker <abrestic@...osinc.com> > --- > v1 -> v2: Update access_error() to account for write-implies-read > --- > arch/riscv/kernel/sys_riscv.c | 3 --- > arch/riscv/mm/fault.c | 3 ++- > 2 files changed, 2 insertions(+), 4 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)); > } > 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; This should be a separate patch with commit text about VMA permissions. > -- > 2.25.1 > Otherwise, lgtm. Reviewed-by: Atish Patra <atishp@...osinc.com> -- Regards, Atish
Powered by blists - more mailing lists