Re: [PATCH 1/2] linux-user: Fix siginfo_t contents when jumping to non-readable pages

[Peter Maydell]

On Fri, 5 Aug 2022 at 11:28, Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> On Fri, 2022-08-05 at 09:50 +0100, Peter Maydell wrote:
> > Which guests do you observe this on ? I think we should indeed
> > fix this in the translators. More specifically, I think we should
> > get this correct already on Arm, and I would expect it to work
> > correctly on all the fixed-insn-width architectures, which can't
> > have page-crossing-insns to start with. x86 probably gets this wrong.

> I first discovered this on s390x, and then realized x86_64 is broken as
> well. Fixing this in translators means adding page boundary checks to
> all code loads. Actually, on s390x it doesn't look as nasty as I
> thought it would, since we quickly figure out the length and load
> everything in one place:
>
> $ grep ld.*code target/s390x/tcg/translate.c | wc -l
> 6

Yes, it depends a bit on the translator and the architecture
how painful it is to get this right. Note that it's OK to
be pessimistic about whether an insn might cross the page
boundary (you end up with a 1-insn TB for it, which is a bit
inefficient but not wrong behaviour). For Arm we check this kind
of thing in insn_crosses_page() (original fix in commit
541ebcd401ee in 2015, cleaned up to be a bit more efficient later).

I think also that this bug is not specific to linux-user.
In a case with a TB like:

 load/store insn that should fault
 other insn that spans page boundary into a non-executable page

ie where the translator failed to break the TB before the
page-boundary-spanning instruction, we will report the fault for
the execution on the non-executable page, when we should have
reported the fault for the load/store, and this happens in system
mode as well as linux-user.

> On x86_64 it's as bad as expected:
>
> $ grep x86_ld.*code target/i386/tcg/translate.c | wc -l
> 96
>
> Implementing this there would mean changing x86_ldub_code() and friends
> to macros, and then making sure we undo everything that we did since
> the start of the instruction. E.g. bt/bts/btr/btc mix parsing and
> op emission. There might be something that touches DisasContext as
> well. Therefore I thought that the generic approach from this patch
> would be more reliable.

No surprise that x86 is a mess :-)

-- PMM