[v3] linux-user: Fix siginfo_t contents when jumping to non-readable pages

[PATCH v3 1/4] accel/tcg: Invalidate translations when clearing PAGE_EXEC

Posted by Ilya Leoshkevich 3 years, 6 months ago

After mprotect(addr, PROT_NONE), addr can still be executed if there
are cached translations. Drop them.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 accel/tcg/translate-all.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index ef62a199c7..32ea5f0adf 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2295,12 +2295,19 @@ void page_set_flags(target_ulong start, target_ulong end, int flags)
          len != 0;
          len -= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
         PageDesc *p = page_find_alloc(addr >> TARGET_PAGE_BITS, 1);
+        bool write_set, exec_cleared;
 
-        /* If the write protection bit is set, then we invalidate
-           the code inside.  */
-        if (!(p->flags & PAGE_WRITE) &&
-            (flags & PAGE_WRITE) &&
-            p->first_tb) {
+        /*
+         * If the write protection bit is set, then we invalidate the code
+         * inside.
+         */
+        write_set = !(p->flags & PAGE_WRITE) && (flags & PAGE_WRITE);
+        /*
+         * If PAGE_EXEC is cleared, we also need to invalidate the code in
+         * order to force a fault when trying to run it.
+         */
+        exec_cleared = (p->flags & PAGE_EXEC) && !(flags & PAGE_EXEC);
+        if ((write_set || exec_cleared) && p->first_tb) {
             tb_invalidate_phys_page(addr, 0);
         }
         if (reset_target_data) {
-- 
2.37.1

Re: [PATCH v3 1/4] accel/tcg: Invalidate translations when clearing PAGE_EXEC

Posted by Richard Henderson 3 years, 6 months ago

On 8/8/22 10:10, Ilya Leoshkevich wrote:
> After mprotect(addr, PROT_NONE), addr can still be executed if there
> are cached translations. Drop them.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   accel/tcg/translate-all.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index ef62a199c7..32ea5f0adf 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -2295,12 +2295,19 @@ void page_set_flags(target_ulong start, target_ulong end, int flags)
>            len != 0;
>            len -= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
>           PageDesc *p = page_find_alloc(addr >> TARGET_PAGE_BITS, 1);
> +        bool write_set, exec_cleared;
>   
> -        /* If the write protection bit is set, then we invalidate
> -           the code inside.  */
> -        if (!(p->flags & PAGE_WRITE) &&
> -            (flags & PAGE_WRITE) &&
> -            p->first_tb) {
> +        /*
> +         * If the write protection bit is set, then we invalidate the code
> +         * inside.
> +         */
> +        write_set = !(p->flags & PAGE_WRITE) && (flags & PAGE_WRITE);
> +        /*
> +         * If PAGE_EXEC is cleared, we also need to invalidate the code in
> +         * order to force a fault when trying to run it.
> +         */
> +        exec_cleared = (p->flags & PAGE_EXEC) && !(flags & PAGE_EXEC);
> +        if ((write_set || exec_cleared) && p->first_tb) {

I believe the bug you're trying to fix is in get_page_addr_code, which for USER_ONLY is 
currently a no-op.  It ought to be checking the page permissions there, as we do for softmmu.

I have a patch for get_page_addr_code in the works, because I was working on pther stuff 
in the area.


r~

Re: [PATCH v3 1/4] accel/tcg: Invalidate translations when clearing PAGE_EXEC

Posted by Ilya Leoshkevich 3 years, 5 months ago

On Wed, 2022-08-10 at 13:29 -0700, Richard Henderson wrote:
> On 8/8/22 10:10, Ilya Leoshkevich wrote:
> > After mprotect(addr, PROT_NONE), addr can still be executed if
> > there
> > are cached translations. Drop them.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   accel/tcg/translate-all.c | 17 ++++++++++++-----
> >   1 file changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> > index ef62a199c7..32ea5f0adf 100644
> > --- a/accel/tcg/translate-all.c
> > +++ b/accel/tcg/translate-all.c
> > @@ -2295,12 +2295,19 @@ void page_set_flags(target_ulong start,
> > target_ulong end, int flags)
> >            len != 0;
> >            len -= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
> >           PageDesc *p = page_find_alloc(addr >> TARGET_PAGE_BITS,
> > 1);
> > +        bool write_set, exec_cleared;
> >   
> > -        /* If the write protection bit is set, then we invalidate
> > -           the code inside.  */
> > -        if (!(p->flags & PAGE_WRITE) &&
> > -            (flags & PAGE_WRITE) &&
> > -            p->first_tb) {
> > +        /*
> > +         * If the write protection bit is set, then we invalidate
> > the code
> > +         * inside.
> > +         */
> > +        write_set = !(p->flags & PAGE_WRITE) && (flags &
> > PAGE_WRITE);
> > +        /*
> > +         * If PAGE_EXEC is cleared, we also need to invalidate the
> > code in
> > +         * order to force a fault when trying to run it.
> > +         */
> > +        exec_cleared = (p->flags & PAGE_EXEC) && !(flags &
> > PAGE_EXEC);
> > +        if ((write_set || exec_cleared) && p->first_tb) {
> 
> I believe the bug you're trying to fix is in get_page_addr_code,
> which for USER_ONLY is 
> currently a no-op.  It ought to be checking the page permissions
> there, as we do for softmmu.
> 
> I have a patch for get_page_addr_code in the works, because I was
> working on pther stuff 
> in the area.

How is qemu-user's get_page_addr_code() involved here?

I tried to experiment with it, and while I agree that it looks buggy,
it's called only from translation code paths. If we already have a
translation block, these code paths are not used.

Re: [PATCH v3 1/4] accel/tcg: Invalidate translations when clearing PAGE_EXEC

Posted by Richard Henderson 3 years, 5 months ago

On 8/11/22 02:28, Ilya Leoshkevich wrote:
> How is qemu-user's get_page_addr_code() involved here?
> 
> I tried to experiment with it, and while I agree that it looks buggy,
> it's called only from translation code paths. If we already have a
> translation block, these code paths are not used.

It's called from tb_lookup too, when we're trying to find an existing TB.


r~

Re: [PATCH v3 1/4] accel/tcg: Invalidate translations when clearing PAGE_EXEC

Posted by Ilya Leoshkevich 3 years, 5 months ago

On Thu, 2022-08-11 at 08:42 -0700, Richard Henderson wrote:
> On 8/11/22 02:28, Ilya Leoshkevich wrote:
> > How is qemu-user's get_page_addr_code() involved here?
> > 
> > I tried to experiment with it, and while I agree that it looks
> > buggy,
> > it's called only from translation code paths. If we already have a
> > translation block, these code paths are not used.
> 
> It's called from tb_lookup too, when we're trying to find an existing
> TB.
> 
> 
> r~
> 

Oh, I see. I was first worried about direct block chaining with
goto_tb, but it turned out that translator_use_goto_tb() prevented it.

tb_lookup() skips get_page_addr_code() if tb is found in tb_jmp_cache.
I assume it's a bug?

Re: [PATCH v3 1/4] accel/tcg: Invalidate translations when clearing PAGE_EXEC

Posted by Richard Henderson 3 years, 5 months ago

On 8/12/22 08:02, Ilya Leoshkevich wrote:
> tb_lookup() skips get_page_addr_code() if tb is found in tb_jmp_cache.
> I assume it's a bug?

Yes, I think so.  I've rearranged that for other reasons, and so may have inadvertently 
fix this.  I'll post the in-progress work in a moment.


r~