linux-kernel - Re: [PATCH] Don't set sempid in semctl syscall.

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]

Message-ID: <20160229212253.GA23467@linux-uzut.site>
Date:	Mon, 29 Feb 2016 13:22:53 -0800
From:	Davidlohr Bueso <dave@...olabs.net>
To:	Michael Kerrisk <mtk.manpages@...il.com>
Cc:	PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>,
	Manfred Spraul <manfred@...orfullife.com>,
	Andrew Morton <akpm@...ux-foundation.org>, herton@...hat.com,
	penguin-kernel@...ove.sakura.ne.jp, rientjes@...gle.com,
	joe@...ches.com, Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Don't set sempid in semctl syscall.

On Sun, 28 Feb 2016, Michael Kerrisk wrote:

>       Linux  also  updates sempid for SETVAL operations and semaphore
>       adjustments.  However, somewhat  inconsistently,  it  does  not
>       update sempid for SETALL operations.  While the SETALL behavior
>       might be viewed as a bug, the behavior is longstanding, and  is
>       probably unlikely to change.

I'm actually in favor of Linux setting sempid for SETALL, obviously as
long as we don't break things. afaik there is no documentation about this,
and we have a chance to do the right thing, given that we already admit to
setting sempid for semctl().  Furthermore, having this exception for SETALL
makes the whole situation weird and even more so ad-hoc. How about we just
fix this and document it in the manpage for whatever version it lands in?

Related, it would be good to add some sort of sempid test to ltp. I've taken
a look at it and there are some clear Linux-specific things being done when
dealing with GETPID assuming only semop modifies the field.

Thanks,
Davidlohr

8<-----------------------------------------------------------------------
Subject: [PATCH] ipc/sem: make semctl setting sempid consistent

As indicated by bug#112271, Linux sets the sempid value upon
semctl, and not only for semop calls. However, within semctl
we only do this for SETVAL, leaving SETALL without updating
the field, and therefore rather inconsistent behavior when
compared to other Unices.

There is really no documentation regarding this and therefore
users should not make assumptions. With this patch, along with
updating semctl.2 manpages, this scenario should become less
ambiguous As such, set sempid on SETALL cmd.

Also update some in-code documentation, specifying where the
sempid is set.

Signed-off-by: Davidlohr Bueso <dbueso@...e.de>
---
Passes ltp and custom testcase where a child (fork) does SETALL
to the set.

  ipc/sem.c | 13 +++++++++++--
  1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index cddd5b5..b3757ea 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -92,7 +92,14 @@
  /* One semaphore structure for each semaphore in the system. */
  struct sem {
  	int	semval;		/* current value */
-	int	sempid;		/* pid of last operation */
+	/*
+	 * PID of the process that last modified the semaphore. For
+	 * Linux, specifically these are:
+	 *  - semop
+	 *  - semctl, via SETVAL and SETALL.
+	 *  - at task exit when performing undo adjustments (see exit_sem).
+	 */
+	int	sempid;
  	spinlock_t	lock;	/* spinlock for fine-grained semtimedop */
  	struct list_head pending_alter; /* pending single-sop operations */
  					/* that alter the semaphore */
@@ -1444,8 +1451,10 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
  			goto out_unlock;
  		}

-		for (i = 0; i < nsems; i++)
+		for (i = 0; i < nsems; i++) {
  			sma->sem_base[i].semval = sem_io[i];
+			sma->sem_base[i].sempid = task_tgid_vnr(current);
+		}

  		ipc_assert_locked_object(&sma->sem_perm);
  		list_for_each_entry(un, &sma->list_id, list_id) {
-- 
2.1.4