printsiginfo: decode si_pkey field

[bacher09]

This adds decoding of si_pkey field which is set on SIGSEGV in case
of memory access violation on some modern CPUs (these have pku flag in /proc/cpuinfo).

Here's few sample outputs:

pkey_alloc(0, 0)                        = 3
pkey_mprotect(0x7f3f20662000, 4096, PROT_READ|PROT_WRITE, 3) = 0
--- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_PKUERR, si_addr=0x7f3f20662000, si_pkey=3} ---

or another case:

mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f137374b000
mprotect(0x7f137374b000, 4096, PROT_EXEC) = 0
--- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_PKUERR, si_addr=0x7f137374b000, si_pkey=1} ---

[codecov]

Codecov Report

Merging #210 (1ffe04f) into master (dadd6b4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #210   +/-   ##
=======================================
  Coverage   89.95%   89.95%           
=======================================
  Files         287      287           
  Lines       24043    24049    +6     
=======================================
+ Hits        21628    21634    +6     
  Misses       2415     2415           

Impacted Files	Coverage Δ
src/printsiginfo.c	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dadd6b4...1ffe04f. Read the comment docs.

[ldv-alt]

On Wed, Feb 02, 2022 at 03:03:45PM +0000, codecov[bot] wrote: > The diff coverage is `33.33%`. | [src/printsiginfo.c](https://codecov.io/gh/strace/strace/pull/210/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=strace#diff-c3JjL3ByaW50c2lnaW5mby5j) | `98.27% <33.33%> (-1.73%)` | ⬇️ |

I wonder whether the new code could be tested?

[ldv-alt]

commit message: please change

* src/printsiginfo.c: decode si_pkey field.

to

* src/printsiginfo.c (print_si_info) [HAVE_SIGINFO_T_SI_PKEY]: Decode
si_pkey field.

[ldv-alt]

Please make sure each sentence ends with a dot.

If you have a look at the NEWS file, you'll see that almost every
"Improvements" section ends with "Updated lists of constants" line followed by
"Updated lists of ioctl commands" line.
Let's keep this tradition, please add your line before rather than after
these lines.

[ldv-alt]

Apparently, siginfo_t.si_pkey is not the only field that could be printed in case of SIGSEGV or SIGBUS,
so I'd suggest to separate case SIGSEG and case SIGBUS, and use a switch on sip->si_code instead of an if statement.

[bacher09]

I wonder whether the new code could be tested?

Behavior is different on different platforms. On platforms that have pku and ospke in /proc/cpuinfo this code would crash with SIGSEGV (and si_pkey=1) :

#define _GNU_SOURCE
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <sys/prctl.h>

int main(void) {
   prctl(PR_SET_DUMPABLE, 0);
   int *buf = mmap(NULL, getpagesize(), PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);

   if (buf == MAP_FAILED) {
       perror("mmap");
       exit(EXIT_FAILURE);
   }
   if (*buf) {
       asm volatile("":::"memory");
   }
   puts("PKU disabled, so it isn't crashed");
}

while on other platforms it will succeed (since these allow read access to PROT_EXEC regions).

I can try to make conditional test, that would be executed only of pku is present in /proc/cpuinfo.

[ldv-alt]

On Thu, Feb 03, 2022 at 12:14:15PM -0800, Slava wrote: > I wonder whether the new code could be tested? Behavior is different on different platforms.

That's OK for such platform-specific code.

puts("PKU disabled, so it isn't crashed");

Adding ``` error_msg_and_skip("PKU seems to be disabled"); ``` would make the test executable exit with code 77, this could be used to skip the whole test.

[bacher09]

Hey, wanted to ask what do you think about tests like this:

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <sys/prctl.h>

int main(void)
{
        prctl(PR_SET_DUMPABLE, 0);
        int *buf = mmap(NULL, getpagesize(), PROT_EXEC,
                                        MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);

        if (buf == MAP_FAILED) {
           perror("mmap");
           exit(EXIT_FAILURE);
        }
        asm volatile("":: "r" (*buf));
        puts("SIGSEGV did not happen");
        return 0;
}

and script like this:

#
# Check -i option.
#
# Copyright (c) 2015-2022 The strace developers.
# All rights reserved.
#
# SPDX-License-Identifier: GPL-2.0-or-later

. "${srcdir=.}/init.sh"

check_prog grep
check_prog sed


if [ ! "/proc/cpuinfo" ]; then
        exit 0
fi

if ! grep -E -e '^flags.*pku' /proc/cpuinfo | grep ospke > /dev/null 2>&1; then
        exit 0
fi

# this test works only if pku is present
set -- "../$NAME"
$STRACE -e trace=none "$@" > "$LOG"  2>&1 |:
addr="$(sed -r -n 's/^--- SIGSEGV \{si_signo=SIGSEGV, si_code=SEGV_PKUERR, si_addr=(0x[[:xdigit:]]+),.*/\1/p' $LOG)" &&
[ -n "$addr" ] || dump_log_and_fail_with
cat > "$EXP" << __EOF__
--- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_PKUERR, si_addr=${addr}, si_pkey=1} ---
+++ killed by SIGSEGV +++
__EOF__

match_diff "$LOG" "$EXP"

I was going to improve it a bit, so it would check cases with different si_pkey.

[bacher09]

I've pushed my changes into another branch (to avoid overwriting your changes), you can check it here. If it's fine I can force push into this branch.

[bacher09]

Hey, I've replaced:

if (*p)
    __asm__ volatile("":::"memory");

with

__asm__ volatile("":: "r" (*p));

which is IMHO a bit simpler and generates a single mov instruction (in x86 assembly), unlike trick with if (which produces 2 instructions).

[ldv-alt]

On Sun, Feb 06, 2022 at 07:49:14AM -0800, Slava wrote: Hey, I've replaced: ``` if (*p) __asm__ volatile("":::"memory"); ``` with ``` __asm__ volatile("":: "r" (*p)); ``` which is IMHO a bit simpler and generates a single `mov` instruction (in x86 assembly), unlike trick with if (which produces 2 instructions).

Thanks. It's clarity that's more important in the test than the number of instructions.

[ldv-alt]

On Sat, Feb 05, 2022 at 02:48:31PM -0800, Slava wrote: I've pushed my changes into another branch (to avoid overwriting your changes), you can check it [here](bacher09@a76c1d3). If it's fine I can force push into this branch.

I'm sorry, but github is not the place where we prefer to discuss things, see README.md

[ldv-alt]

On Sat, Feb 05, 2022 at 05:10:02PM -0800, Slava wrote: Hey, wanted to ask what do you think about tests like this: ```#define _GNU_SOURCE #include <stdio.h> #include <unistd.h> #include <stdlib.h> #include <sys/mman.h> #include <sys/prctl.h> int main(void) { prctl(PR_SET_DUMPABLE, 0); int *buf = mmap(NULL, getpagesize(), PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); if (buf == MAP_FAILED) { perror("mmap"); exit(EXIT_FAILURE); } asm volatile("":: "r" (*buf)); puts("SIGSEGV did not happen"); return 0; } ``` and script like this: ```#!/bin/sh # # Check -i option.

Really? :)

# # Copyright (c) 2015-2022 The strace developers. # All rights reserved. # # SPDX-License-Identifier: GPL-2.0-or-later . "${srcdir=.}/init.sh" check_prog grep check_prog sed if [ ! "/proc/cpuinfo" ]; then exit 0 fi if ! grep -E -e '^flags.*pku' /proc/cpuinfo | grep ospke > /dev/null 2>&1; then exit 0 fi

No, /proc/cpuinfo is not reliable in this respect, there are architectures like ppc64 and ppc64le that do not have any pku mentioned in /proc/cpuinfo at all.

# this test works only if pku is present set -- "../$NAME" $STRACE -e trace=none "$@" > "$LOG" 2>&1 |: addr="$(sed -r -n 's/^--- SIGSEGV \{si_signo=SIGSEGV, si_code=SEGV_PKUERR, si_addr=(0x[[:xdigit:]]+),.*/\1/p' $LOG)" && [ -n "$addr" ] || dump_log_and_fail_with cat > "$EXP" << __EOF__ --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_PKUERR, si_addr=${addr}, si_pkey=1} --- +++ killed by SIGSEGV +++ __EOF__ match_diff "$LOG" "$EXP" ```

Yet again, this won't play well with other architectures. For example, on ppc64 and ppc64le for some reason it's si_pkey=2 instead of si_pkey=1, and on mips and riscv it's SEGV_ACCERR instead of SEGV_PKUERR. I'd rather stick with the version of this test that does all necessary checks inside the executable.