linux-mips.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] MIPS: Grant pte read permission, even if vma only have VM_WRITE permission.
@ 2020-07-17  9:55 Lichao Liu
  2020-07-22  9:36 ` Thomas Bogendoerfer
  0 siblings, 1 reply; 5+ messages in thread
From: Lichao Liu @ 2020-07-17  9:55 UTC (permalink / raw)
  To: tsbogend; +Cc: linux-mips, Lichao Liu

If a page can be written, then it can definitely be read.

Signed-off-by: Lichao Liu <liulichao@loongson.cn>
---
 arch/mips/mm/cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
index ad6df1cea866..72b60c44a962 100644
--- a/arch/mips/mm/cache.c
+++ b/arch/mips/mm/cache.c
@@ -160,7 +160,7 @@ static inline void setup_protection_map(void)
 	if (cpu_has_rixi) {
 		protection_map[0]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
 		protection_map[1]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
-		protection_map[2]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
+		protection_map[2]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
 		protection_map[3]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
 		protection_map[4]  = __pgprot(_page_cachable_default | _PAGE_PRESENT);
 		protection_map[5]  = __pgprot(_page_cachable_default | _PAGE_PRESENT);
@@ -169,7 +169,7 @@ static inline void setup_protection_map(void)
 
 		protection_map[8]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
 		protection_map[9]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
-		protection_map[10] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE | _PAGE_NO_READ);
+		protection_map[10] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE);
 		protection_map[11] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE);
 		protection_map[12] = __pgprot(_page_cachable_default | _PAGE_PRESENT);
 		protection_map[13] = __pgprot(_page_cachable_default | _PAGE_PRESENT);
-- 
2.25.1


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

* Re: [PATCH] MIPS: Grant pte read permission, even if vma only have VM_WRITE permission.
  2020-07-17  9:55 [PATCH] MIPS: Grant pte read permission, even if vma only have VM_WRITE permission Lichao Liu
@ 2020-07-22  9:36 ` Thomas Bogendoerfer
  2020-07-24  1:50   ` Lichao Liu
  0 siblings, 1 reply; 5+ messages in thread
From: Thomas Bogendoerfer @ 2020-07-22  9:36 UTC (permalink / raw)
  To: Lichao Liu; +Cc: linux-mips

On Fri, Jul 17, 2020 at 05:55:36PM +0800, Lichao Liu wrote:
> If a page can be written, then it can definitely be read.

IMHO it's exactly the point of RIXI enabled CPUs to support a
writeonly mapping even if most of other archs aren't able to
support it. So if there is no real good reason to change this,
I'm going to leave it this way.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

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

* Re: [PATCH] MIPS: Grant pte read permission, even if vma only have VM_WRITE permission.
  2020-07-22  9:36 ` Thomas Bogendoerfer
@ 2020-07-24  1:50   ` Lichao Liu
  2020-07-31 22:37     ` Maciej W. Rozycki
  0 siblings, 1 reply; 5+ messages in thread
From: Lichao Liu @ 2020-07-24  1:50 UTC (permalink / raw)
  To: Thomas Bogendoerfer; +Cc: linux-mips



在 2020/7/22 下午5:36, Thomas Bogendoerfer 写道:
> On Fri, Jul 17, 2020 at 05:55:36PM +0800, Lichao Liu wrote:
>> If a page can be written, then it can definitely be read.
> IMHO it's exactly the point of RIXI enabled CPUs to support a
> writeonly mapping even if most of other archs aren't able to
> support it. So if there is no real good reason to change this,
> I'm going to leave it this way.
>
> Thomas.
>

If a vma only have VM_WRITE permission, the vma->vm_page_prot will
set _PAGE_NO_READ. In general case, someone read the vma will trigger
RI exception, then do_page_fault will handle it.

But in the following scene, program will hang.

futex_wake_op() will read uaddr, which is passed from user space.
If a program mmap a vma, which only have VM_WRITE permission,
then exec futex syscall, and use an address belonging to the vma as uaddr
argument.

futex_wake_op() will disable pagefault and set correct __ex_table(return -14 directly),
then try to access the address in atomic,
do_page_fault will find the correct __ex_table, and then return -14.

Then futex_wake_op() will try to fixup this error by call
fault_in_user_writeable(),  the fixup will success.
But, because the pte only write permission,
so after fault_in_user_writeable(), 
the corresponding pte and tlb will set RI bit.

The program will deadloop:
do_page_fault(RI) -> find __ex_table success -> return -14;
futex_wake_op -> call fault_in_user_writeable() to fix the error -> success -> retry;
do_page_fault(RI) -> find __ex_table success -> return -14;
futex_wake_op -> call fault_in_user_writeable() to fix the error -> success -> retry;

I think there are have two solutions to the problem:
1)modify fault_in_user_writeable(), 
  must claim read permission when claiming write permission.
2)Grant pte read permission, even if vma only have VM_WRITE permission.

But not sure which one is more suitable.

Thank you.


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

* Re: [PATCH] MIPS: Grant pte read permission, even if vma only have VM_WRITE permission.
  2020-07-24  1:50   ` Lichao Liu
@ 2020-07-31 22:37     ` Maciej W. Rozycki
  0 siblings, 0 replies; 5+ messages in thread
From: Maciej W. Rozycki @ 2020-07-31 22:37 UTC (permalink / raw)
  To: Lichao Liu; +Cc: Thomas Bogendoerfer, linux-mips, Maciej W. Rozycki

On Fri, 24 Jul 2020, Lichao Liu wrote:

> > IMHO it's exactly the point of RIXI enabled CPUs to support a
> > writeonly mapping even if most of other archs aren't able to
> > support it. So if there is no real good reason to change this,
> > I'm going to leave it this way.
[...]
> I think there are have two solutions to the problem:
> 1)modify fault_in_user_writeable(), 
>   must claim read permission when claiming write permission.
> 2)Grant pte read permission, even if vma only have VM_WRITE permission.
> 
> But not sure which one is more suitable.

 Well, the internal documentation is clear:

 * fault_in_user_writeable() - Fault in user address and verify RW access

so if it does only verify W rather than RW access, then it has to be fixed 
and verify both kinds of access at a time.  Presumably:

	mmap_read_lock(mm);
	ret = fixup_user_fault(current, mm, (unsigned long)uaddr, 0, NULL);
	if (!ret)
		ret = fixup_user_fault(current, mm, (unsigned long)uaddr,
				       FAULT_FLAG_WRITE, NULL);
        mmap_read_unlock(mm);

at the minimum or perhaps by expanding the interface of `fixup_user_fault' 
to also support FAULT_FLAG_RW so as to avoid the double call.

 As Thomas says silently expanding access permissions beyond what has been 
granted would be a security breach.

  Maciej

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

* [PATCH] MIPS: Grant pte read permission, even if vma only have VM_WRITE permission.
@ 2020-06-30  0:58 Lichao Liu
  0 siblings, 0 replies; 5+ messages in thread
From: Lichao Liu @ 2020-06-30  0:58 UTC (permalink / raw)
  To: tsbogend, yuanjunqing, chenhc, jiaxun.yang
  Cc: linux-mips, linux-kernel, Lichao Liu

Background:
a cpu have RIXI feature.

Now, if a vma only have VM_WRITE permission, the vma->vm_page_prot will
set _PAGE_NO_READ. In general case, someone read the vma will trigger
RI exception, then do_page_fault will handle it.

But in the following scene, program will hang.

example scene(a trinity test case):
futex_wake_op() will read uaddr, which is passed from user space.
If a program mmap a vma, which only have VM_WRITE permission,
then call futex, and use an address belonging to the vma as uaddr
argument. futex_wake_op() will read the address after disable
pagefault and set correct __ex_table(return -14 directly),
do_page_fault will find the correct __ex_table, and then return -14.
Then futex_wake_op() will try to fixup this error by call
fault_in_user_writeable(), because the pte have write permission,
so handle_mm_fault will do nothing, and return success.
But the RI bit in pte and tlb entry still exsits.
The program will deadloop:
do_page_fault -> find __ex_table success -> return -14;
futex_wake_op -> call fault_in_user_writeable() to fix the error -> retry;
do_page_fault -> find __ex_table success -> return -14;
futex_wake_op -> call fault_in_user_writeable() to fix the error -> retry;
.....

The first perspective of root cause:
Futex think a pte have write permission will have read permission.
When page fault, it only try to fixup with FAULT_FLAG_WRITE.

The second perspective of root cause:
MIPS platform doesn't grant pte read permission, if vma only have
VM_WRITE permission.But X86 and arm64 will.

Most of the architecture will grant pte read permission, even if
the vma only have VM_WRITE permission.
And if the cpu doesn't have RIXI feature, MIPS platform will
grant pte read permission by set _PAGE_READ.
So I think we should fixup thix problem by grant pte read permission,
even if vma only have VM_WRITE permission.

Signed-off-by: Lichao Liu <liulichao@loongson.cn>
---
 arch/mips/mm/cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
index ad6df1cea866..72b60c44a962 100644
--- a/arch/mips/mm/cache.c
+++ b/arch/mips/mm/cache.c
@@ -160,7 +160,7 @@ static inline void setup_protection_map(void)
 	if (cpu_has_rixi) {
 		protection_map[0]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
 		protection_map[1]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
-		protection_map[2]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
+		protection_map[2]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
 		protection_map[3]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
 		protection_map[4]  = __pgprot(_page_cachable_default | _PAGE_PRESENT);
 		protection_map[5]  = __pgprot(_page_cachable_default | _PAGE_PRESENT);
@@ -169,7 +169,7 @@ static inline void setup_protection_map(void)
 
 		protection_map[8]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
 		protection_map[9]  = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
-		protection_map[10] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE | _PAGE_NO_READ);
+		protection_map[10] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE);
 		protection_map[11] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE);
 		protection_map[12] = __pgprot(_page_cachable_default | _PAGE_PRESENT);
 		protection_map[13] = __pgprot(_page_cachable_default | _PAGE_PRESENT);
-- 
2.25.1


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

end of thread, other threads:[~2020-07-31 22:37 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17  9:55 [PATCH] MIPS: Grant pte read permission, even if vma only have VM_WRITE permission Lichao Liu
2020-07-22  9:36 ` Thomas Bogendoerfer
2020-07-24  1:50   ` Lichao Liu
2020-07-31 22:37     ` Maciej W. Rozycki
  -- strict thread matches above, loose matches on Subject: below --
2020-06-30  0:58 Lichao Liu

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).