linux-kernel - Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 13 Nov 2015 11:58:53 -0800
From:	Davidlohr Bueso <dave@...olabs.net>
To:	"Kirill A. Shutemov" <kirill@...temov.name>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Manfred Spraul <manfred@...orfullife.com>
Subject: Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in
 shm_mmap()

On Fri, 13 Nov 2015, Bueso wrote:


>So considering EINVAL, even your approach to bumping up nattach by calling
>_shm_open earlier isn't enough. Races exposed to user called rmid can still
>occur between dropping the lock and doing ->mmap(). Ultimately this leads to
>all ipc_valid_object() checks, as we totally ignore SHM_DEST segments nowadays
>since we forbid mapping previously removed segments.
>
>I think this is the first thing we must decide before going forward with this
>mess. ipc currently defines invalid objects by merely checking the deleted flag.

Particularly something like this, which we could then add to the vma validity
check, thus saving the lookup as well.

diff --git a/ipc/shm.c b/ipc/shm.c
index 4178727..d9b2fb1 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -64,6 +64,20 @@ static const struct vm_operations_struct shm_vm_ops;
  #define shm_unlock(shp)			\
  	ipc_unlock(&(shp)->shm_perm)
  
+/*
+ * shm object validity is different than the rest of ipc
+ * as shm needs to deal with segments previously marked
+ * for deletion, which can occur at any time via user calls.
+ */
+static inline int shm_invalid_object(struct kern_ipc_perm *perm)
+{
+	if (perm->mode & SHM_DEST)
+		return -EINVAL;
+	if (ipc_valid_object(perm))
+		return -EIDRM;
+	return 0; /* yay */
+}
+
  static int newseg(struct ipc_namespace *, struct ipc_params *);
  static void shm_open(struct vm_area_struct *vma);
  static void shm_close(struct vm_area_struct *vma);
@@ -985,11 +999,9 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
  
  		ipc_lock_object(&shp->shm_perm);
  
-		/* check if shm_destroy() is tearing down shp */
-		if (!ipc_valid_object(&shp->shm_perm)) {
-			err = -EIDRM;
+		err = shm_invalid_object(&shp->shm_perm);
+		if (err)
  			goto out_unlock0;
-		}
  
  		if (!ns_capable(ns->user_ns, CAP_IPC_LOCK)) {
  			kuid_t euid = current_euid();
@@ -1124,10 +1136,9 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
  
  	ipc_lock_object(&shp->shm_perm);
  
-	/* check if shm_destroy() is tearing down shp */
-	if (!ipc_valid_object(&shp->shm_perm)) {
+	err = shm_invalid_object(&shp->shm_perm);
+	if (err) {
  		ipc_unlock_object(&shp->shm_perm);
-		err = -EIDRM;
  		goto out_unlock;
  	}
  

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/