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

[Colin Ian King]

On 30/11/17 17:43, Kleber Sacilotto de Souza wrote:
> From: Davidlohr Bueso <dave at stgolabs.net>
> 
> 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 at suse.de>
> Reported-by: Gareth Evans <gareth.evans at contextis.co.uk>
> Cc: Manfred Spraul <manfred at colorfullife.com>
> Cc: Michael Kerrisk <mtk.manpages at googlemail.com>
> Cc: <stable at vger.kernel.org>
> Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
> 
> CVE-2017-5669
> (cherry picked from commit 95e91b831f87ac8e1f8ed50c14d709089b4e01b8)
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
> ---
>  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 <colin.king at canonical.com>