[PATCH v7 05/20] linux-user: Clear translations and tb_jmp_cache on mprotect()

Richard Henderson posted 20 patches 3 years, 5 months ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Riku Voipio <riku.voipio@iki.fi>, Laurent Vivier <laurent@vivier.eu>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, Peter Maydell <peter.maydell@linaro.org>, Michael Rolnik <mrolnik@gmail.com>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Taylor Simpson <tsimpson@quicinc.com>, Eduardo Habkost <eduardo@habkost.net>, Song Gao <gaosong@loongson.cn>, Xiaojuan Yang <yangxiaojuan@loongson.cn>, Aurelien Jarno <aurelien@aurel32.net>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Chris Wulff <crwulff@gmail.com>, Marek Vasut <marex@denx.de>, Stafford Horne <shorne@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Greg Kurz <groug@kaod.org>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Yoshinori Sato <ysato@users.sourceforge.jp>, Cornelia Huck <cohuck@redhat.com>, Thomas Huth <thuth@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Max Filippov <jcmvbkbc@gmail.com>, "Alex Bennée" <alex.bennee@linaro.org>
[PATCH v7 05/20] linux-user: Clear translations and tb_jmp_cache on mprotect()
Posted by Richard Henderson 3 years, 5 months ago
From: Ilya Leoshkevich <iii@linux.ibm.com>

Currently it's possible to execute pages that do not have PAGE_EXEC
if there is an existing translation block. Fix by clearing tb_jmp_cache
and invalidating TBs, which forces recheck of permission bits.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Message-Id: <20220817150506.592862-2-iii@linux.ibm.com>
[rth: Invalidate is required -- e.g. riscv fallthrough cross test]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

fixup mprotect
---
 linux-user/mmap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 048c4135af..e9dc8848be 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -115,6 +115,7 @@ int target_mprotect(abi_ulong start, abi_ulong len, int target_prot)
 {
     abi_ulong end, host_start, host_end, addr;
     int prot1, ret, page_flags, host_prot;
+    CPUState *cpu;
 
     trace_target_mprotect(start, len, target_prot);
 
@@ -177,7 +178,14 @@ int target_mprotect(abi_ulong start, abi_ulong len, int target_prot)
             goto error;
         }
     }
+
     page_set_flags(start, start + len, page_flags);
+    tb_invalidate_phys_range(start, start + len);
+
+    CPU_FOREACH(cpu) {
+        cpu_tb_jmp_cache_clear(cpu);
+    }
+
     mmap_unlock();
     return 0;
 error:
-- 
2.34.1
Re: [PATCH v7 05/20] linux-user: Clear translations and tb_jmp_cache on mprotect()
Posted by Ilya Leoshkevich 3 years, 5 months ago
On Tue, 2022-08-23 at 15:05 -0700, Richard Henderson wrote:
> From: Ilya Leoshkevich <iii@linux.ibm.com>
> 
> Currently it's possible to execute pages that do not have PAGE_EXEC
> if there is an existing translation block. Fix by clearing
> tb_jmp_cache
> and invalidating TBs, which forces recheck of permission bits.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Message-Id: <20220817150506.592862-2-iii@linux.ibm.com>
> [rth: Invalidate is required -- e.g. riscv fallthrough cross test]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> fixup mprotect
> ---
>  linux-user/mmap.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index 048c4135af..e9dc8848be 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -115,6 +115,7 @@ int target_mprotect(abi_ulong start, abi_ulong
> len, int target_prot)
>  {
>      abi_ulong end, host_start, host_end, addr;
>      int prot1, ret, page_flags, host_prot;
> +    CPUState *cpu;
>  
>      trace_target_mprotect(start, len, target_prot);
>  
> @@ -177,7 +178,14 @@ int target_mprotect(abi_ulong start, abi_ulong
> len, int target_prot)
>              goto error;
>          }
>      }
> +
>      page_set_flags(start, start + len, page_flags);
> +    tb_invalidate_phys_range(start, start + len);
> +
> +    CPU_FOREACH(cpu) {
> +        cpu_tb_jmp_cache_clear(cpu);
> +    }
> +
>      mmap_unlock();
>      return 0;
>  error:

I think adding tb_invalidate_phys_range() obviates the need for
cpu_tb_jmp_cache_clear()? The lookup may still find an invalidated tb,
but it will have CF_INVALID set.

The following worked for me:

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index e9dc8848bed..b58e3eeb198 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -115,7 +115,6 @@ int target_mprotect(abi_ulong start, abi_ulong len,
int target_prot)
 {
     abi_ulong end, host_start, host_end, addr;
     int prot1, ret, page_flags, host_prot;
-    CPUState *cpu;
 
     trace_target_mprotect(start, len, target_prot);
 
@@ -182,10 +181,6 @@ int target_mprotect(abi_ulong start, abi_ulong
len, int target_prot)
     page_set_flags(start, start + len, page_flags);
     tb_invalidate_phys_range(start, start + len);
 
-    CPU_FOREACH(cpu) {
-        cpu_tb_jmp_cache_clear(cpu);
-    }
-
     mmap_unlock();
     return 0;
 error:


Re: [PATCH v7 05/20] linux-user: Clear translations and tb_jmp_cache on mprotect()
Posted by Richard Henderson 3 years, 5 months ago
On 8/31/22 00:17, Ilya Leoshkevich wrote:
>>       page_set_flags(start, start + len, page_flags);
>> +    tb_invalidate_phys_range(start, start + len);
>> +
>> +    CPU_FOREACH(cpu) {
>> +        cpu_tb_jmp_cache_clear(cpu);
>> +    }
>> +
>>       mmap_unlock();
>>       return 0;
>>   error:
> 
> I think adding tb_invalidate_phys_range() obviates the need for
> cpu_tb_jmp_cache_clear()? The lookup may still find an invalidated tb,
> but it will have CF_INVALID set.

Quite right.  And we definitely don't want to have to touch a list of all threads if its 
not necessary.


r~