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

From: Ilya Leoshkevich <iii@linux.ibm.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "Laurent Vivier" <laurent@vivier.eu>,
	"Eduardo Habkost" <eduardo@habkost.net>,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Yanan Wang" <wangyanan55@huawei.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"David Hildenbrand" <david@redhat.com>,
	qemu-devel@nongnu.org, qemu-s390x@nongnu.org,
	"Christian Borntraeger" <borntraeger@linux.ibm.com>
Subject: Re: [PATCH 1/2] linux-user: Fix siginfo_t contents when jumping to non-readable pages
Date: Fri, 05 Aug 2022 12:28:26 +0200	[thread overview]
Message-ID: <6aafa461732e7c98670c0e9c765cc95a5d88e8f0.camel@linux.ibm.com> (raw)
In-Reply-To: <CAFEAcA9FG+b4=-QNujG5Prx_me8uw7YTWjk-mqr3_X1Wb0wHzg@mail.gmail.com>

On Fri, 2022-08-05 at 09:50 +0100, Peter Maydell wrote:
> On Thu, 4 Aug 2022 at 19:50, Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > When the first instruction of a translation block is located in a
> > non-readable page, qemu-user fills siginfo_t correctly. For the
> > other
> > instructions the result is as if it were the first instruction,
> > which
> > is not correct.
> > 
> > The reason is that the current logic expects translate_insn() hook
> > to
> > stop at the page boundary. This way only the first instruction can
> > cause a SEGV. However, this is quite difficult to properly
> > implement
> > when the problematic instruction crosses a page boundary, and
> > indeed
> > the actual implementations do not do this. Note that this can also
> > break self-modifying code detection when only bytes on the second
> > page
> > are modified, but this is outside of the scope of this patch.
> 
> 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.
> 
> thanks
> -- PMM

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

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.