linux-kernel - Re: [PATCH] selftests/proc: Fix proc-pid-vm for vsyscall=xonly.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 17 Jun 2022 11:45:43 -0700
From:   Dylan Hatch <dylanbhatch@...gle.com>
To:     Shuah Khan <skhan@...uxfoundation.org>
Cc:     Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH] selftests/proc: Fix proc-pid-vm for vsyscall=xonly.

On Thu, Jun 16, 2022 at 4:01 PM Shuah Khan <skhan@...uxfoundation.org> wrote:
>
> On 6/16/22 3:10 PM, Dylan Hatch wrote:
> > This test would erroneously fail the /proc/$PID/maps case if
> > vsyscall=xonly since the existing probe of the vsyscall page only
> > succeeds if the process has read permissions. Fix this by checking for
> > either no vsyscall mapping OR an execute-only vsyscall mapping in the
> > case were probing the vsyscall page segfaults.
> >
>
> Does this fix include skipping the test with a clear message that
> says why test is skipped?
>
> > Signed-off-by: Dylan Hatch <dylanbhatch@...gle.com>
> > ---
> >   tools/testing/selftests/proc/proc-pid-vm.c | 20 +++++++++++++++-----
> >   1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/testing/selftests/proc/proc-pid-vm.c b/tools/testing/selftests/proc/proc-pid-vm.c
> > index 28604c9f805c..5ca85520131f 100644
> > --- a/tools/testing/selftests/proc/proc-pid-vm.c
> > +++ b/tools/testing/selftests/proc/proc-pid-vm.c
> > @@ -213,9 +213,12 @@ static int make_exe(const uint8_t *payload, size_t len)
> >
> >   static bool g_vsyscall = false;
> >
> > -static const char str_vsyscall[] =
> > +static const char str_vsyscall_rx[] =
> >   "ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]\n";
> >
> > +static const char str_vsyscall_x[] =
> > +"ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0                  [vsyscall]\n";
> > +
> >   #ifdef __x86_64__
> >   static void sigaction_SIGSEGV(int _, siginfo_t *__, void *___)
> >   {
> > @@ -261,6 +264,7 @@ int main(void)
> >       int exec_fd;
> >
> >       vsyscall();
> > +     const char *str_vsyscall = g_vsyscall ? str_vsyscall_rx : str_vsyscall_x;
> >
> >       atexit(ate);
> >
> > @@ -314,7 +318,8 @@ int main(void)
> >
> >       /* Test /proc/$PID/maps */
> >       {
> > -             const size_t len = strlen(buf0) + (g_vsyscall ? strlen(str_vsyscall) : 0);
> > +             const size_t len_buf0 = strlen(buf0);
> > +             const size_t len_vsys = strlen(str_vsyscall);
> >               char buf[256];
> >               ssize_t rv;
> >               int fd;
> > @@ -325,11 +330,16 @@ int main(void)
> >                       return 1;
> >               }
> >               rv = read(fd, buf, sizeof(buf));
> > -             assert(rv == len);
> > -             assert(memcmp(buf, buf0, strlen(buf0)) == 0);
> >               if (g_vsyscall) {
> > -                     assert(memcmp(buf + strlen(buf0), str_vsyscall, strlen(str_vsyscall)) == 0);
> > +                     assert(rv == len_buf0 + len_vsys);
> > +             } else {
> > +                     /* If vsyscall isn't readable, it's either x-only or not mapped at all */
> > +                     assert(rv == len_buf0 + len_vsys || rv == len_buf0);
> >               }
> > +             assert(memcmp(buf, buf0, len_buf0) == 0);
> > +             /* Check for vsyscall mapping if buf is long enough */
> > +             if (rv == len_buf0 + len_vsys)
> > +                     assert(memcmp(buf + len_buf0, str_vsyscall, len_vsys) == 0);
> >       }
> >
> >       /* Test /proc/$PID/smaps */
> >
>
> The change looks good to me. Doesn't look like it skips the test though?

Instead of skipping the test, it changes the passing condition to
accept both cases of an unmapped vsyscall page and an x-only vsyscall
page. Differentiating between these two cases without relying on
/proc/$PID/maps would involve both checking the kernel command line
for vsyscall=xonly and having a special ifdef block for
CONFIG_VSYSCALL_XONLY, so accepting both as passing conditions seems
like a simpler solution.

Thanks,
Dylan