Re: [Qemu-devel] [PATCH v2] mips: Decide to map PAGE_EXEC in map_address

[Aleksandar Markovic]

On May 16, 2019 6:31 PM, "Philippe Mathieu-Daudé" <address@hidden> wrote:
>
> Hi Jakub,
>
> On 5/16/19 3:10 PM, Jakub Jermar wrote:
> > Hi,
> >
> > On 5/3/19 12:02 PM, Jakub Jermar wrote:
> >> Hi,
> >>
> >> On 4/23/19 4:58 PM, Jakub Jermar wrote:
> >>> Hi Philippe!
> >>>
> >>> On 4/23/19 3:48 PM, Philippe Mathieu-Daudé wrote:
> >>>> Hi Jakub,
> >>>>
> >>>> On 4/23/19 1:00 PM, Jakub Jermář wrote:
> >>>>> This commit addresses QEMU Bug #1825311:
> >>>>>
> >>>>>   mips_cpu_handle_mmu_fault renders all accessed pages executable
> >>>>>
> >>>>> It allows finer-grained control over whether the accessed page
should be
> >>>>> executable by moving the decision to the underlying map_address
> >>>>> function, which has more information for this.
> >>>>>
> >>>>> As a result, pages that have the XI bit set in the TLB and are
accessed
> >>>>> for read/write, don't suddenly end up being executable.
> >>>>>
> >>>>
> >>>> Fixes: https://bugs.launchpad.net/qemu/+bug/1825311
> >>>>
> >>>>> Signed-off-by: Jakub Jermář <address@hidden>
> >>>>> ---
> >>>>>  target/mips/helper.c | 17 ++++++++++-------
> >>>>>  1 file changed, 10 insertions(+), 7 deletions(-)
> >>>>>
> >>>>> diff --git a/target/mips/helper.c b/target/mips/helper.c
> >>>>> index c44cdca3b5..132d073fbe 100644
> >>>>> --- a/target/mips/helper.c
> >>>>> +++ b/target/mips/helper.c
> >>>>> @@ -43,7 +43,7 @@ int no_mmu_map_address (CPUMIPSState *env, hwaddr
*physical, int *prot,
> >>>>>                          target_ulong address, int rw, int
access_type)
> >>>>>  {
> >>>>>      *physical = address;
> >>>>> -    *prot = PAGE_READ | PAGE_WRITE;
> >>>>> +    *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> >>>>>      return TLBRET_MATCH;
> >>>>>  }
> >>>>>
> >>>>> @@ -61,7 +61,7 @@ int fixed_mmu_map_address (CPUMIPSState *env,
hwaddr *physical, int *prot,
> >>>>>      else
> >>>>>          *physical = address;
> >>>>>
> >>>>> -    *prot = PAGE_READ | PAGE_WRITE;
> >>>>> +    *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> >>>>>      return TLBRET_MATCH;
> >>>>>  }
> >>>>>
> >>>>> @@ -101,6 +101,9 @@ int r4k_map_address (CPUMIPSState *env, hwaddr
*physical, int *prot,
> >>>>>                  *prot = PAGE_READ;
> >>>>>                  if (n ? tlb->D1 : tlb->D0)
> >>>>>                      *prot |= PAGE_WRITE;
> >>>>> +                if (!(n ? tlb->XI1 : tlb->XI0)) {
> >>>>> +                    *prot |= PAGE_EXEC;
> >>>>> +                }
> >>>>
> >>>> This was indeed missed in commit 2fb58b73746e.
>
> Aleksandar, if this patch is OK with you, can you amend this comment,
> and add the "Fixes:" tag too when applying? Thanks!

Yes, definitely, Philippe, that is not a problem.

Thanks for helping out!

I tested Jakub's scenario too, it works as expected, but I am not concerned
about it as much as about regression tests. Knowing that you have many MIPS
test kernels and images, may I ask you to test some of them WITH Jakub's
fix (so indepenently of myself anf Jakub), just to confirm that there are
no regressions?

C’est vraiment gentil de votre part.

Aleksandar

> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>
> >>>>
> >>>>>                  return TLBRET_MATCH;
> >>>>>              }
> >>>>>              return TLBRET_DIRTY;
> >>>>> @@ -182,7 +185,7 @@ static int
get_seg_physical_address(CPUMIPSState *env, hwaddr *physical,
> >>>>>      } else {
> >>>>>          /* The segment is unmapped */
> >>>>>          *physical = physical_base | (real_address & segmask);
> >>>>> -        *prot = PAGE_READ | PAGE_WRITE;
> >>>>> +        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> >>>>>          return TLBRET_MATCH;
> >>>>>      }
> >>>>>  }
> >>>>> @@ -913,8 +916,8 @@ int mips_cpu_handle_mmu_fault(CPUState *cs,
vaddr address, int size, int rw,
> >>>>>      }
> >>>>>      if (ret == TLBRET_MATCH) {
> >>>>>          tlb_set_page(cs, address & TARGET_PAGE_MASK,
> >>>>> -                     physical & TARGET_PAGE_MASK, prot | PAGE_EXEC,
> >>>>> -                     mmu_idx, TARGET_PAGE_SIZE);
> >>>>> +                     physical & TARGET_PAGE_MASK, prot, mmu_idx,
> >>>>> +                     TARGET_PAGE_SIZE);
> >>>>>          ret = 0;
> >>>>>      } else if (ret < 0)
> >>>>>  #endif
> >>>>> @@ -936,8 +939,8 @@ int mips_cpu_handle_mmu_fault(CPUState *cs,
vaddr address, int size, int rw,
> >>>>>                                             address, rw,
access_type, mmu_idx);
> >>>>>                  if (ret == TLBRET_MATCH) {
> >>>>>                      tlb_set_page(cs, address & TARGET_PAGE_MASK,
> >>>>> -                            physical & TARGET_PAGE_MASK, prot |
PAGE_EXEC,
> >>>>> -                            mmu_idx, TARGET_PAGE_SIZE);
> >>>>> +                            physical & TARGET_PAGE_MASK, prot,
mmu_idx,
> >>>>> +                            TARGET_PAGE_SIZE);
> >>>>>                      ret = 0;
> >>>>>                      return ret;
> >>>>>                  }
> >>>>>
> >>>>
> >>>> Your patch looks correct, but I'd like to test it.
> >>>> Do you have a reproducer?
> >>>> Can you describe the command line you used?
> >>>
> >>> I've just attached a reproducer image and script to the bug. It's a
> >>> 32-bit little-endian test binary running on top of the L4Re
microkernel.
>
> I can't get the "TAP" output you described on launchpad.
>
> >>> Let me know if you also need a 64-bit version.
>
> 64-bit version is welcomed.
>
> >>> I tested both 32 and 64-bit versions of the reproducer and also
checked
> >>> to see that the the other images I have lying around here (Linux
2.6.32
> >>> big endian and HelenOS master little-endian, both 32-bit for 4Kc)
> >>> continue to run without regressions.
>
> Yes, definitively an improvement:
> Tested-by: Philippe Mathieu-Daudé <address@hidden>
>
> Regards,
>
> Phil.
>