* Re: Questions about commit "ipc/shm: Fix shmat mmap nil-page protection"
@ 2017-09-25 21:44 ` Andrea Arcangeli
0 siblings, 0 replies; 6+ messages in thread
From: Andrea Arcangeli @ 2017-09-25 21:44 UTC (permalink / raw)
To: Joe Lawrence; +Cc: linux-kernel@vger.kernel.org, Davidlohr Bueso, linux-mm
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Questions about commit "ipc/shm: Fix shmat mmap nil-page protection"
2017-09-25 21:44 ` Andrea Arcangeli
@ 2017-10-10 18:11 ` Joe Lawrence
-1 siblings, 0 replies; 6+ messages in thread
From: Joe Lawrence @ 2017-10-10 18:11 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: Andrea Arcangeli, linux-kernel@vger.kernel.org, linux-mm
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Questions about commit "ipc/shm: Fix shmat mmap nil-page protection"
@ 2017-10-10 18:11 ` Joe Lawrence
0 siblings, 0 replies; 6+ messages in thread
From: Joe Lawrence @ 2017-10-10 18:11 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: Andrea Arcangeli, linux-kernel@vger.kernel.org, linux-mm
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Questions about commit "ipc/shm: Fix shmat mmap nil-page protection"
2017-09-25 21:44 ` Andrea Arcangeli
(?)
(?)
@ 2018-04-30 17:21 ` Davidlohr Bueso
-1 siblings, 0 replies; 6+ messages in thread
From: Davidlohr Bueso @ 2018-04-30 17:21 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Joe Lawrence, akpm, gareth.evans, linux-kernel@vger.kernel.org,
linux-mm
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
^ permalink raw reply [flat|nested] 6+ messages in thread