[PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages

Ilya Leoshkevich posted 4 patches 1 year, 7 months ago
There is a newer version of this series
linux-user/mmap.c                |  14 +++
target/i386/tcg/translate.c      |  25 +++++-
target/s390x/tcg/translate.c     |  15 +++-
tests/tcg/multiarch/noexec.h     | 114 ++++++++++++++++++++++++
tests/tcg/s390x/Makefile.target  |   1 +
tests/tcg/s390x/noexec.c         | 145 +++++++++++++++++++++++++++++++
tests/tcg/x86_64/Makefile.target |   3 +-
tests/tcg/x86_64/noexec.c        | 116 +++++++++++++++++++++++++
8 files changed, 427 insertions(+), 6 deletions(-)
create mode 100644 tests/tcg/multiarch/noexec.h
create mode 100644 tests/tcg/s390x/noexec.c
create mode 100644 tests/tcg/x86_64/noexec.c
[PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages
Posted by Ilya Leoshkevich 1 year, 7 months ago
Hi,

I noticed that when we get a SEGV due to jumping to non-readable
memory, sometimes si_addr and program counter in siginfo_t are slightly
off. I tracked this down to the assumption that translators stop before
the end of a page, while in reality they may stop right after it.

Patch 1 fixes an invalidation issue, which may prevent SEGV from
happening altogether.
Patches 2-3 fix the main issue on x86_64 and s390x. Many other
architectures have fixed-size instructions and are not affected.
Patch 4 adds tests.

Note: this series depends on [1].

Best regards,
Ilya

v1: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg00822.html
v1 -> v2: Fix individual translators instead of translator_loop
          (Peter).

v2: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01079.html
v2 -> v3: Peek at the next instruction on s390x (Richard).
          Undo more on i386 (Richard).
          Check PAGE_EXEC, not PAGE_READ (Peter, Richard).

v3: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01306.html
v3 -> v4: Improve the commit message in patch 1 to better reflect what
          exactly is being fixed there.
          Factor out the is_same_page() patch (Richard).
          Do not touch the common code in the i386 fix (Richard).

v4: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01747.html
v4 -> v5: Drop patch 2.
          Use a different fix for the invalidation issue based on
          discussion with Richard [2].

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02472.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02556.html

Ilya Leoshkevich (4):
  linux-user: Clear tb_jmp_cache on mprotect()
  target/s390x: Make translator stop before the end of a page
  target/i386: Make translator stop before the end of a page
  tests/tcg: Test siginfo_t contents when jumping to non-readable pages

 linux-user/mmap.c                |  14 +++
 target/i386/tcg/translate.c      |  25 +++++-
 target/s390x/tcg/translate.c     |  15 +++-
 tests/tcg/multiarch/noexec.h     | 114 ++++++++++++++++++++++++
 tests/tcg/s390x/Makefile.target  |   1 +
 tests/tcg/s390x/noexec.c         | 145 +++++++++++++++++++++++++++++++
 tests/tcg/x86_64/Makefile.target |   3 +-
 tests/tcg/x86_64/noexec.c        | 116 +++++++++++++++++++++++++
 8 files changed, 427 insertions(+), 6 deletions(-)
 create mode 100644 tests/tcg/multiarch/noexec.h
 create mode 100644 tests/tcg/s390x/noexec.c
 create mode 100644 tests/tcg/x86_64/noexec.c

-- 
2.37.1
Re: [PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages
Posted by Vivian Wang 1 year, 7 months ago
On 8/17/22 23:05, Ilya Leoshkevich wrote:
> Hi,
>
> I noticed that when we get a SEGV due to jumping to non-readable
> memory, sometimes si_addr and program counter in siginfo_t are slightly
> off. I tracked this down to the assumption that translators stop before
> the end of a page, while in reality they may stop right after it.

Hi,

Could this be related to issue 1155 [1]? On RISC-V, I'm getting
incorrect [m|s]tval/[m|s]epc combinations for page faults in system
emulation and incorrect si_addr and program counter on SIGSEGV in user
emulation. Since it seems to only affect instructions that cross page
boundaries, and RISC-V also has variable length instructions, it seems
that I've run into the same problem as what is fixed here.

Could this fix be extended be extended to targets/riscv?

dram

[1]: https://gitlab.com/qemu-project/qemu/-/issues/1155

> Patch 1 fixes an invalidation issue, which may prevent SEGV from
> happening altogether.
> Patches 2-3 fix the main issue on x86_64 and s390x. Many other
> architectures have fixed-size instructions and are not affected.
> Patch 4 adds tests.
>
> Note: this series depends on [1].
>
> Best regards,
> Ilya
>
> v1: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg00822.html
> v1 -> v2: Fix individual translators instead of translator_loop
>           (Peter).
>
> v2: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01079.html
> v2 -> v3: Peek at the next instruction on s390x (Richard).
>           Undo more on i386 (Richard).
>           Check PAGE_EXEC, not PAGE_READ (Peter, Richard).
>
> v3: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01306.html
> v3 -> v4: Improve the commit message in patch 1 to better reflect what
>           exactly is being fixed there.
>           Factor out the is_same_page() patch (Richard).
>           Do not touch the common code in the i386 fix (Richard).
>
> v4: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01747.html
> v4 -> v5: Drop patch 2.
>           Use a different fix for the invalidation issue based on
>           discussion with Richard [2].
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02472.html
> [2] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02556.html
>
> Ilya Leoshkevich (4):
>   linux-user: Clear tb_jmp_cache on mprotect()
>   target/s390x: Make translator stop before the end of a page
>   target/i386: Make translator stop before the end of a page
>   tests/tcg: Test siginfo_t contents when jumping to non-readable pages
>
>  linux-user/mmap.c                |  14 +++
>  target/i386/tcg/translate.c      |  25 +++++-
>  target/s390x/tcg/translate.c     |  15 +++-
>  tests/tcg/multiarch/noexec.h     | 114 ++++++++++++++++++++++++
>  tests/tcg/s390x/Makefile.target  |   1 +
>  tests/tcg/s390x/noexec.c         | 145 +++++++++++++++++++++++++++++++
>  tests/tcg/x86_64/Makefile.target |   3 +-
>  tests/tcg/x86_64/noexec.c        | 116 +++++++++++++++++++++++++
>  8 files changed, 427 insertions(+), 6 deletions(-)
>  create mode 100644 tests/tcg/multiarch/noexec.h
>  create mode 100644 tests/tcg/s390x/noexec.c
>  create mode 100644 tests/tcg/x86_64/noexec.c
>
Re: [PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages
Posted by Richard Henderson 1 year, 7 months ago
On 8/18/22 09:55, Vivian Wang wrote:
> On 8/17/22 23:05, Ilya Leoshkevich wrote:
>> Hi,
>>
>> I noticed that when we get a SEGV due to jumping to non-readable
>> memory, sometimes si_addr and program counter in siginfo_t are slightly
>> off. I tracked this down to the assumption that translators stop before
>> the end of a page, while in reality they may stop right after it.
> 
> Hi,
> 
> Could this be related to issue 1155 [1]? On RISC-V, I'm getting incorrect 
> [m|s]tval/[m|s]epc combinations for page faults in system emulation and incorrect si_addr 
> and program counter on SIGSEGV in user emulation. Since it seems to only affect 
> instructions that cross page boundaries, and RISC-V also has variable length instructions, 
> it seems that I've run into the same problem as what is fixed here.

It seems likely, and the code at the end of riscv_tr_translate_insn is wrong.

> Could this fix be extended be extended to targets/riscv?

I'll write up something.


r~
Re: [PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages
Posted by Ilya Leoshkevich 1 year, 7 months ago
On Fri, 2022-08-19 at 00:55 +0800, Vivian Wang wrote:
> Hi,
> Could this be related to issue 1155 [1]? On RISC-V, I'm getting
> incorrect [m|s]tval/[m|s]epc combinations for page faults in system
> emulation and incorrect si_addr and program counter on SIGSEGV in
> user emulation. Since it seems to only affect instructions that cross
> page boundaries, and RISC-V also has variable length instructions, it
> seems that I've run into the same problem as what is fixed here.
> Could this fix be extended be extended to targets/riscv?
> dram
> [1]: https://gitlab.com/qemu-project/qemu/-/issues/1155

Yes, this looks quite similar.
I'm not too familiar with riscv, but I just googled [1].
If the following is correct:

---
However, the instruction set reserves enough opcode space to make it
possible to differentiate between 16-bit, 32-bit, 48-bit, and 64-bit
instructions.  Instructions that start with binary 11 (in the lowest
bit position of the instruction) are 32-bit sized instructions (but one
pattern is reserved: so they cannot start with 11111).  The compact
instructions use 00, 01, and 10 in that same position.  48-bit
instructions use starting sequence 011111, and 64-bit instructions
start with 0111111.
---

then we can fix this the same way s390x is being fixed here.

[1]
https://stackoverflow.com/questions/56874101/how-does-risc-v-variable-length-of-instruction-work-in-detail
Re: [PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages
Posted by Richard Henderson 1 year, 7 months ago
On 8/17/22 10:05, Ilya Leoshkevich wrote:
> Hi,
> 
> I noticed that when we get a SEGV due to jumping to non-readable
> memory, sometimes si_addr and program counter in siginfo_t are slightly
> off. I tracked this down to the assumption that translators stop before
> the end of a page, while in reality they may stop right after it.
> 
> Patch 1 fixes an invalidation issue, which may prevent SEGV from
> happening altogether.
> Patches 2-3 fix the main issue on x86_64 and s390x. Many other
> architectures have fixed-size instructions and are not affected.
> Patch 4 adds tests.
> 
> Note: this series depends on [1].

Hah.  I was just thinking that I should queue your patch set to tcg-next-7.2, and then 
rebase my stuff off of that.  It would ensure that I have your test cases in tree so that 
I don't keep regressing them on you.  :-)

I'll cherry pick the one patch you're depending on.


r~
Re: [PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages
Posted by Ilya Leoshkevich 1 year, 7 months ago
On Wed, 2022-08-17 at 11:23 -0500, Richard Henderson wrote:
> On 8/17/22 10:05, Ilya Leoshkevich wrote:
> > Hi,
> > 
> > I noticed that when we get a SEGV due to jumping to non-readable
> > memory, sometimes si_addr and program counter in siginfo_t are
> > slightly
> > off. I tracked this down to the assumption that translators stop
> > before
> > the end of a page, while in reality they may stop right after it.
> > 
> > Patch 1 fixes an invalidation issue, which may prevent SEGV from
> > happening altogether.
> > Patches 2-3 fix the main issue on x86_64 and s390x. Many other
> > architectures have fixed-size instructions and are not affected.
> > Patch 4 adds tests.
> > 
> > Note: this series depends on [1].
> 
> Hah.  I was just thinking that I should queue your patch set to tcg-
> next-7.2, and then 
> rebase my stuff off of that.  It would ensure that I have your test
> cases in tree so that 
> I don't keep regressing them on you.  :-)
> 
> I'll cherry pick the one patch you're depending on.
> 
> 
> r~

I just checked and cherry-picking [1] and [2] before this series
should be enough.

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02462.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02461.html