[SRU][Trusty][PATCH 0/1] Fix for CVE-2017-5669

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][Trusty][PATCH 0/1] Fix for CVE-2017-5669

Kleber Souza
Clean cherry-pick for Trusty, the other supported series are
either not affected or have already been fixed.

Tested with the proof of concept code attached to
https://bugzilla.kernel.org/show_bug.cgi?id=192931.

Davidlohr Bueso (1):
  ipc/shm: Fix shmat mmap nil-page protection

 ipc/shm.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

--
2.14.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Trusty][PATCH 1/1] ipc/shm: Fix shmat mmap nil-page protection

Kleber Souza
From: Davidlohr Bueso <[hidden email]>

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@...
Signed-off-by: Davidlohr Bueso <[hidden email]>
Reported-by: Gareth Evans <[hidden email]>
Cc: Manfred Spraul <[hidden email]>
Cc: Michael Kerrisk <[hidden email]>
Cc: <[hidden email]>
Signed-off-by: Andrew Morton <[hidden email]>
Signed-off-by: Linus Torvalds <[hidden email]>

CVE-2017-5669
(cherry picked from commit 95e91b831f87ac8e1f8ed50c14d709089b4e01b8)
Signed-off-by: Kleber Sacilotto de Souza <[hidden email]>
---
 ipc/shm.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index d7805acb44fd..06ea9ef7f54a 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1091,8 +1091,8 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
  * "raddr" thing points to kernel space, and there has to be a wrapper around
  * this.
  */
-long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
-      unsigned long shmlba)
+long do_shmat(int shmid, char __user *shmaddr, int shmflg,
+      ulong *raddr, unsigned long shmlba)
 {
  struct shmid_kernel *shp;
  unsigned long addr;
@@ -1113,8 +1113,13 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
  goto out;
  else if ((addr = (ulong)shmaddr)) {
  if (addr & (shmlba - 1)) {
- if (shmflg & SHM_RND)
- addr &= ~(shmlba - 1);   /* round down */
+ /*
+ * Round down to the nearest multiple of shmlba.
+ * For sane do_mmap_pgoff() parameters, avoid
+ * round downs that trigger nil-page and MAP_FIXED.
+ */
+ if ((shmflg & SHM_RND) && addr >= shmlba)
+ addr &= ~(shmlba - 1);
  else
 #ifndef __ARCH_FORCE_SHMLBA
  if (addr & ~PAGE_MASK)
--
2.14.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][Trusty][PATCH 1/1] ipc/shm: Fix shmat mmap nil-page protection

Colin King
On 30/11/17 17:43, Kleber Sacilotto de Souza wrote:

> From: Davidlohr Bueso <[hidden email]>
>
> 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@...
> Signed-off-by: Davidlohr Bueso <[hidden email]>
> Reported-by: Gareth Evans <[hidden email]>
> Cc: Manfred Spraul <[hidden email]>
> Cc: Michael Kerrisk <[hidden email]>
> Cc: <[hidden email]>
> Signed-off-by: Andrew Morton <[hidden email]>
> Signed-off-by: Linus Torvalds <[hidden email]>
>
> CVE-2017-5669
> (cherry picked from commit 95e91b831f87ac8e1f8ed50c14d709089b4e01b8)
> Signed-off-by: Kleber Sacilotto de Souza <[hidden email]>
> ---
>  ipc/shm.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index d7805acb44fd..06ea9ef7f54a 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1091,8 +1091,8 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
>   * "raddr" thing points to kernel space, and there has to be a wrapper around
>   * this.
>   */
> -long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
> -      unsigned long shmlba)
> +long do_shmat(int shmid, char __user *shmaddr, int shmflg,
> +      ulong *raddr, unsigned long shmlba)
>  {
>   struct shmid_kernel *shp;
>   unsigned long addr;
> @@ -1113,8 +1113,13 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>   goto out;
>   else if ((addr = (ulong)shmaddr)) {
>   if (addr & (shmlba - 1)) {
> - if (shmflg & SHM_RND)
> - addr &= ~(shmlba - 1);   /* round down */
> + /*
> + * Round down to the nearest multiple of shmlba.
> + * For sane do_mmap_pgoff() parameters, avoid
> + * round downs that trigger nil-page and MAP_FIXED.
> + */
> + if ((shmflg & SHM_RND) && addr >= shmlba)
> + addr &= ~(shmlba - 1);
>   else
>  #ifndef __ARCH_FORCE_SHMLBA
>   if (addr & ~PAGE_MASK)
>
Clean upstream cherry pick, looks sane to me.

Acked-by: Colin Ian King <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team