Questions about commit "ipc/shm: Fix shmat mmap nil-page protection"

Questions about commit "ipc/shm: Fix shmat mmap nil-page protection"

[Joe Lawrence]

Hi Davidlohr,

I was looking into backporting commit 95e91b831f87 ("ipc/shm: Fix shmat
mmap nil-page protection") to a distro kernel and Andrea brought up some
interesting questions about that change.

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


Is it possible that the original report noticed that shmat allowed
attach to an address of 1, and it was assumed that somehow mmap_min_addr
protections were circumvented?  Then commit 95e91b831f87 modified the
rounding in do_shmat() so that shmat would fail on similar input (but
for apparently different reasons)?

I didn't see any discussion when looking up the original commit in the
list archives, so any explanations or pointers would be very helpful.

[1]
https://github.com/linux-test-project/ltp/blob/master/testcases/cve/cve-2017-5669.c

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

Regards,

-- Joe

Re: Questions about commit "ipc/shm: Fix shmat mmap nil-page protection"

[Andrea Arcangeli]

Hello,

On Mon, Sep 25, 2017 at 03:38:07PM -0400, Joe Lawrence wrote:
> Hi Davidlohr,
> 
> I was looking into backporting commit 95e91b831f87 ("ipc/shm: Fix shmat
> mmap nil-page protection") to a distro kernel and Andrea brought up some
> interesting questions about that change.
> 
> 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

Yes, thanks for the accurate summary Joe.

> Is it possible that the original report noticed that shmat allowed
> attach to an address of 1, and it was assumed that somehow mmap_min_addr
> protections were circumvented?  Then commit 95e91b831f87 modified the
> rounding in do_shmat() so that shmat would fail on similar input (but
> for apparently different reasons)?
> 
> I didn't see any discussion when looking up the original commit in the
> list archives, so any explanations or pointers would be very helpful.

We identified only one positive side effect to such change, it is
about the semantics of SHM_REMAP when addr < shmlba (and != 0). Before
the patch SHM_REMAP was erroneously implicit for that virtual
range. However that's not security related either, and there's no
mention of SHM_REMAP in the commit message.

So then we wondered what this CVE is about in the first place, it
looks a invalid CVE for a not existent security issue. The testcase at
least shows no malfunction, mapping addr 0 is fine to succeed with
CAP_SYS_RAWIO.

>From the commit message, testcase and CVE I couldn't get what this
commit is about.

Last but not the least, if there was a security problem in calling
do_mmap_pgoff with addr=0, flags=MAP_FIXED|MAP_SHARED the fix would
better be moved to do_mmap_pgoff, not in ipc/shm.c.

Thanks,
Andrea

Re: Questions about commit "ipc/shm: Fix shmat mmap nil-page protection"

[Andrea Arcangeli]

Hello,

On Mon, Sep 25, 2017 at 03:38:07PM -0400, Joe Lawrence wrote:
> Hi Davidlohr,
> 
> I was looking into backporting commit 95e91b831f87 ("ipc/shm: Fix shmat
> mmap nil-page protection") to a distro kernel and Andrea brought up some
> interesting questions about that change.
> 
> 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

Yes, thanks for the accurate summary Joe.

> Is it possible that the original report noticed that shmat allowed
> attach to an address of 1, and it was assumed that somehow mmap_min_addr
> protections were circumvented?  Then commit 95e91b831f87 modified the
> rounding in do_shmat() so that shmat would fail on similar input (but
> for apparently different reasons)?
> 
> I didn't see any discussion when looking up the original commit in the
> list archives, so any explanations or pointers would be very helpful.

We identified only one positive side effect to such change, it is
about the semantics of SHM_REMAP when addr < shmlba (and != 0). Before
the patch SHM_REMAP was erroneously implicit for that virtual
range. However that's not security related either, and there's no
mention of SHM_REMAP in the commit message.

So then we wondered what this CVE is about in the first place, it
looks a invalid CVE for a not existent security issue. The testcase at
least shows no malfunction, mapping addr 0 is fine to succeed with
CAP_SYS_RAWIO.

>From the commit message, testcase and CVE I couldn't get what this
commit is about.

Last but not the least, if there was a security problem in calling
do_mmap_pgoff with addr=0, flags=MAP_FIXED|MAP_SHARED the fix would
better be moved to do_mmap_pgoff, not in ipc/shm.c.

Thanks,
Andrea

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Questions about commit "ipc/shm: Fix shmat mmap nil-page protection"

[Joe Lawrence]

On 09/25/2017 05:44 PM, Andrea Arcangeli wrote:
> Hello,
> 
> On Mon, Sep 25, 2017 at 03:38:07PM -0400, Joe Lawrence wrote:
>> Hi Davidlohr,
>>
>> I was looking into backporting commit 95e91b831f87 ("ipc/shm: Fix shmat
>> mmap nil-page protection") to a distro kernel and Andrea brought up some
>> interesting questions about that change.
>>
>> 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
> 
> Yes, thanks for the accurate summary Joe.
> 
>> Is it possible that the original report noticed that shmat allowed
>> attach to an address of 1, and it was assumed that somehow mmap_min_addr
>> protections were circumvented?  Then commit 95e91b831f87 modified the
>> rounding in do_shmat() so that shmat would fail on similar input (but
>> for apparently different reasons)?
>>
>> I didn't see any discussion when looking up the original commit in the
>> list archives, so any explanations or pointers would be very helpful.
> 
> We identified only one positive side effect to such change, it is
> about the semantics of SHM_REMAP when addr < shmlba (and != 0). Before
> the patch SHM_REMAP was erroneously implicit for that virtual
> range. However that's not security related either, and there's no
> mention of SHM_REMAP in the commit message.
> 
> So then we wondered what this CVE is about in the first place, it
> looks a invalid CVE for a not existent security issue. The testcase at
> least shows no malfunction, mapping addr 0 is fine to succeed with
> CAP_SYS_RAWIO.
> 
> From the commit message, testcase and CVE I couldn't get what this
> commit is about.
> 
> Last but not the least, if there was a security problem in calling
> do_mmap_pgoff with addr=0, flags=MAP_FIXED|MAP_SHARED the fix would
> better be moved to do_mmap_pgoff, not in ipc/shm.c.

Gentle ping.

-- Joe

Re: Questions about commit "ipc/shm: Fix shmat mmap nil-page protection"

[Joe Lawrence]

On 09/25/2017 05:44 PM, Andrea Arcangeli wrote:
> Hello,
> 
> On Mon, Sep 25, 2017 at 03:38:07PM -0400, Joe Lawrence wrote:
>> Hi Davidlohr,
>>
>> I was looking into backporting commit 95e91b831f87 ("ipc/shm: Fix shmat
>> mmap nil-page protection") to a distro kernel and Andrea brought up some
>> interesting questions about that change.
>>
>> 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
> 
> Yes, thanks for the accurate summary Joe.
> 
>> Is it possible that the original report noticed that shmat allowed
>> attach to an address of 1, and it was assumed that somehow mmap_min_addr
>> protections were circumvented?  Then commit 95e91b831f87 modified the
>> rounding in do_shmat() so that shmat would fail on similar input (but
>> for apparently different reasons)?
>>
>> I didn't see any discussion when looking up the original commit in the
>> list archives, so any explanations or pointers would be very helpful.
> 
> We identified only one positive side effect to such change, it is
> about the semantics of SHM_REMAP when addr < shmlba (and != 0). Before
> the patch SHM_REMAP was erroneously implicit for that virtual
> range. However that's not security related either, and there's no
> mention of SHM_REMAP in the commit message.
> 
> So then we wondered what this CVE is about in the first place, it
> looks a invalid CVE for a not existent security issue. The testcase at
> least shows no malfunction, mapping addr 0 is fine to succeed with
> CAP_SYS_RAWIO.
> 
> From the commit message, testcase and CVE I couldn't get what this
> commit is about.
> 
> Last but not the least, if there was a security problem in calling
> do_mmap_pgoff with addr=0, flags=MAP_FIXED|MAP_SHARED the fix would
> better be moved to do_mmap_pgoff, not in ipc/shm.c.

Gentle ping.

-- Joe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Questions about commit "ipc/shm: Fix shmat mmap nil-page protection"

[Davidlohr Bueso]

On Mon, 25 Sep 2017, Andrea Arcangeli wrote:

Sorry this took so long guys. I had forgotten about this until it recently
resurfaced.

>Hello,
>
>On Mon, Sep 25, 2017 at 03:38:07PM -0400, Joe Lawrence wrote:
>> Hi Davidlohr,
>>
>> I was looking into backporting commit 95e91b831f87 ("ipc/shm: Fix shmat
>> mmap nil-page protection") to a distro kernel and Andrea brought up some
>> interesting questions about that change.
>>
>> 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
>
>Yes, thanks for the accurate summary Joe.
>
>> Is it possible that the original report noticed that shmat allowed
>> attach to an address of 1, and it was assumed that somehow mmap_min_addr
>> protections were circumvented?  Then commit 95e91b831f87 modified the
>> rounding in do_shmat() so that shmat would fail on similar input (but
>> for apparently different reasons)?
>>
>> I didn't see any discussion when looking up the original commit in the
>> list archives, so any explanations or pointers would be very helpful.
>
>We identified only one positive side effect to such change, it is
>about the semantics of SHM_REMAP when addr < shmlba (and != 0). Before
>the patch SHM_REMAP was erroneously implicit for that virtual
>range. However that's not security related either, and there's no
>mention of SHM_REMAP in the commit message.

Coincidence. I didn't notice the SHM_REMAP, but after looking at it
you appear to be right. I'll send a patch along with the revert
(see below).

>
>So then we wondered what this CVE is about in the first place, it
>looks a invalid CVE for a not existent security issue. The testcase at
>least shows no malfunction, mapping addr 0 is fine to succeed with
>CAP_SYS_RAWIO.

This is exactly the issue. I thought mapping addr=0 with MAP_FIXED
was an issue, including for root. Hence avoiding the round off from
1 to 0. If this is legal, then this commit needs reverted.

In fact, X11[1] seems to rely on this _exact_ case; and this change
breaks semantics.

>
>From the commit message, testcase and CVE I couldn't get what this
>commit is about.
>
>Last but not the least, if there was a security problem in calling
>do_mmap_pgoff with addr=0, flags=MAP_FIXED|MAP_SHARED the fix would
>better be moved to do_mmap_pgoff, not in ipc/shm.c.

Yeah at the time, akpm and I wondered why this was special to security.

[1] https://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/os-support/linux/int10/linux.c#n347

Thanks,
Davidlohr