Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cve-2017-5669.c needs fix for shmat() nil addresses #319

Closed
rafaeldtinoco opened this issue May 28, 2018 · 2 comments
Closed

cve-2017-5669.c needs fix for shmat() nil addresses #319

rafaeldtinoco opened this issue May 28, 2018 · 2 comments

Comments

@rafaeldtinoco
Copy link
Contributor

rafaeldtinoco commented May 28, 2018

According to upstream thread (https://lkml.org/lkml/2018/5/28/2056) , cve-2017-5669 needs to address the "new" way of handling nil addresses for shmat() when used with MAP_FIXED or SHM_REMAP flags.

  1. mapping nil-page is OK on lower addresses with MAP_FIXED (or else X11 is broken)
  2. mapping nil-page is NOT OK with SHM_REMAP on lower addresses

The (Linux-specific) SHM_REMAP flag may be specified in shmflg to indicate that the mapping of the segment should replace any existing mapping in the range starting at shmaddr and continuing for the size of the segment. (Normally an EINVAL error would result if a mapping already exists in this address range.) In this case, shmaddr must not be NULL.

Based on original discussion: https://marc.info/?i=20180430172152.nfa564pvgpk3ut7p%40linux-n805

You will find initial motivation for the patches:

>> We saw that a LTP test [1] was added some time ago to reproduce behavior
>> matching that of the original report [2].  However, Andrea and I are a
>> little confused about that original report and what the upstream commit
>> was intended to fix.  A quick summary of our offlist discussion:
>>
>> - This is only about privileged users (and no SELinux).
>>
>> - We modified the 20170119_shmat_nullpage_poc.c reproducer from [2] to
>>   include MAP_FIXED to prove (as root, no SELinux):
>>
>>     It is possible to mmap 0
>>     It is NOT possible to mmap 1
>>
>> - Andrea points out that mmap(1, ...) fails not because of any
>>   mmap_min_addr checks, but for alignment reasons.
>>
>> - He also wonders about other bogus addr values above 4k, but below
>>   mmap_min_addr and whether this change misses those values

AND

In 4.17-rc7 you will find:

commit 8f89c007b6de
Author: Davidlohr Bueso <dave@stgolabs.net>
Date:   Fri May 25 14:47:30 2018 -0700

    ipc/shm: fix shmat() nil address after round-down when remapping

    shmat()'s SHM_REMAP option forbids passing a nil address for; this is in
    fact the very first thing we check for.  Andrea reported that for
    SHM_RND|SHM_REMAP cases we can end up bypassing the initial addr check,
    but we need to check again if the address was rounded down to nil.  As
    of this patch, such cases will return -EINVAL.

    Link: http://lkml.kernel.org/r/20180503204934.kk63josdu6u53fbd@linux-n805
    Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
    Reported-by: Andrea Arcangeli <aarcange@redhat.com>
    Cc: Joe Lawrence <joe.lawrence@redhat.com>
    Cc: Manfred Spraul <manfred@colorfullife.com>
    Cc: <stable@vger.kernel.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

TODO: Make sure a call to shmat() with SHM_RND & SHM_REMAP flags can't succeed for nil addresses (-EINVAL has to be returned), changing the current test that only tests SHM_RND flag.

and

commit a73ab244f0da
Author: Davidlohr Bueso <dave@stgolabs.net>
Date:   Fri May 25 14:47:27 2018 -0700

    Revert "ipc/shm: Fix shmat mmap nil-page protection"

    Patch series "ipc/shm: shmat() fixes around nil-page".

    These patches fix two issues reported[1] a while back by Joe and Andrea
    around how shmat(2) behaves with nil-page.

    The first reverts a commit that it was incorrectly thought that mapping
    nil-page (address=0) was a no no with MAP_FIXED.  This is not the case,
    with the exception of SHM_REMAP; which is address in the second patch.

    I chose two patches because it is easier to backport and it explicitly
    reverts bogus behaviour.  Both patches ought to be in -stable and ltp
    testcases need updated (the added testcase around the cve can be
    modified to just test for SHM_RND|SHM_REMAP).

    [1] lkml.kernel.org/r/20180430172152.nfa564pvgpk3ut7p@linux-n805

    This patch (of 2):

    Commit 95e91b831f87 ("ipc/shm: Fix shmat mmap nil-page protection")
    worked on the idea that we should not be mapping as root addr=0 and
    MAP_FIXED.  However, it was reported that this scenario is in fact
    valid, thus making the patch both bogus and breaks userspace as well.

    For example X11's libint10.so relies on shmat(1, SHM_RND) for lowmem
    initialization[1].

    [1] https://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/os-support/linux/int10/linux
    Link: http://lkml.kernel.org/r/20180503203243.15045-2-dave@stgolabs.net
    Fixes: 95e91b831f87 ("ipc/shm: Fix shmat mmap nil-page protection")
    Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
    Reported-by: Joe Lawrence <joe.lawrence@redhat.com>
    Reported-by: Andrea Arcangeli <aarcange@redhat.com>
    Cc: Manfred Spraul <manfred@colorfullife.com>
    Cc: <stable@vger.kernel.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
commit 95e91b831f87
Author: Davidlohr Bueso <dave@stgolabs.net>
Date:   Mon Feb 27 14:28:24 2017 -0800

    ipc/shm: Fix shmat mmap nil-page protection

    The issue is described here, with a nice testcase:

        https://bugzilla.kernel.org/show_bug.cgi?id=192931

    The problem is that shmat() calls do_mmap_pgoff() with MAP_FIXED, and
    the address rounded down to 0.  For the regular mmap case, the
    protection mentioned above is that the kernel gets to generate the
    address -- arch_get_unmapped_area() will always check for MAP_FIXED and
    return that address.  So by the time we do security_mmap_addr(0) things
    get funky for shmat().

    The testcase itself shows that while a regular user crashes, root will
    not have a problem attaching a nil-page.  There are two possible fixes
    to this.  The first, and which this patch does, is to simply allow root
    to crash as well -- this is also regular mmap behavior, ie when hacking
    up the testcase and adding mmap(...  |MAP_FIXED).  While this approach
    is the safer option, the second alternative is to ignore SHM_RND if the
    rounded address is 0, thus only having MAP_SHARED flags.  This makes the
    behavior of shmat() identical to the mmap() case.  The downside of this
    is obviously user visible, but does make sense in that it maintains
    semantics after the round-down wrt 0 address and mmap.

    Passes shm related ltp tests.

    Link: http://lkml.kernel.org/r/1486050195-18629-1-git-send-email-dave@stgolabs.net
    Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
    Reported-by: Gareth Evans <gareth.evans@contextis.co.uk>
    Cc: Manfred Spraul <manfred@colorfullife.com>
    Cc: Michael Kerrisk <mtk.manpages@googlemail.com>
    Cc: <stable@vger.kernel.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

@rafaeldtinoco
Copy link
Contributor Author

I'm verifying:

https://github.com/inaddy/ltp/commit/e3840669d2bb87e63c6df205cea1beb9eb5539dd

With old and new LTP tests, old and new kernels to make sure test is good before merge proposal.

@rafaeldtinoco
Copy link
Contributor Author

rafaeldtinoco commented May 30, 2018

ltp tests with old kernels (~4.16) both patched and unpatched

with previous ltp and old kernel

(k)inaddy@shmatbefore:bin$ sudo ./cve-2017-5669
tst_test.c:1015: INFO: Timeout per run is 0h 05m 00s
cve-2017-5669.c:62: INFO: Attempting to attach shared memory to null page
cve-2017-5669.c:67: PASS: shmat returned EINVAL

Summary:
passed   1 <-- OKAY
failed   0
skipped  0
warnings 0

with patched ltp and previous kernel

(k)inaddy@shmatbefore:bin$ sudo ./cve-2017-5669
tst_test.c:1015: INFO: Timeout per run is 0h 05m 00s
cve-2017-5669.c:65: INFO: Attempting to attach shared memory to null page
cve-2017-5669.c:74: PASS: shmat returned EINVAL

Summary:
passed   1 <-- OKAY
failed   0
skipped  0
warnings 0

mainline kernel tests (rc-7-0044cdeb7313)

with previous ltp and mainline kernel

(k)inaddy@shmatafter:bin$ sudo ./cve-2017-5669
tst_test.c:1015: INFO: Timeout per run is 0h 05m 00s
cve-2017-5669.c:62: INFO: Attempting to attach shared memory to null page
cve-2017-5669.c:74: INFO: Mapped shared memory to (nil)
cve-2017-5669.c:78: FAIL: We have mapped a VM address within the first 64Kb
cve-2017-5669.c:84: INFO: Touching shared memory to see if anything strange happens

Summary:
passed   0
failed   1 <-- FAILS
skipped  0
warnings 0

with patched ltp and mainline kernel

(k)inaddy@shmatafter:bin$ sudo dpkg -i ~inaddy/ltp_2018-05-15~1~f7aaca128+shmat_amd64.deb
(Reading database ... 112393 files and directories currently installed.)
Preparing to unpack .../ltp_2018-05-15~1~f7aaca128+shmat_amd64.deb ...
Unpacking ltp (2018-05-15~1~f7aaca128+shmat) over (2018-05-15~0~f7aaca127) ...
Setting up ltp (2018-05-15~1~f7aaca128+shmat) ...

(k)inaddy@shmatafter:bin$ sudo ./cve-2017-5669
tst_test.c:1015: INFO: Timeout per run is 0h 05m 00s
cve-2017-5669.c:65: INFO: Attempting to attach shared memory to null page
cve-2017-5669.c:74: PASS: shmat returned EINVAL

Summary:
passed   1 <-- OKAY
failed   0
skipped  0
warnings 0

chase-qi pushed a commit to Linaro/ltp that referenced this issue Jun 6, 2018
… with REMAPs

Fixes: linux-test-project#319

According to upstream thread (https://lkml.org/lkml/2018/5/28/2056),
cve-2017-5669 needs to address the "new" way of handling nil addresses
for shmat() when used with MAP_FIXED or SHM_REMAP flags.

- mapping nil-page is OK on lower addresses with MAP_FIXED (or else X11 is broken)
- mapping nil-page is NOT OK with SHM_REMAP on lower addresses

Addresses Davidlohr Bueso's comments/changes:

commit 8f89c007b6de
Author: Davidlohr Bueso <dave@stgolabs.net>
Date:   Fri May 25 14:47:30 2018 -0700

    ipc/shm: fix shmat() nil address after round-down when remapping

commit a73ab244f0da
Author: Davidlohr Bueso <dave@stgolabs.net>
Date:   Fri May 25 14:47:27 2018 -0700

    Revert "ipc/shm: Fix shmat mmap nil-page protection"

For previously test, and now broken, made based on:

commit 95e91b831f87
Author: Davidlohr Bueso <dave@stgolabs.net>
Date:   Mon Feb 27 14:28:24 2017 -0800

    ipc/shm: Fix shmat mmap nil-page protection

Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Reviewed-by: Jan Stancek <jstancek@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant