[PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Rik van Riel]

When manipulating just one semaphore with semop, sem_lock only takes that
single semaphore's lock. This creates a problem during initialization of
the semaphore array, when the data structures used by sem_lock have not
been set up yet. The sma->lock is already held by newary, and we just
have to make sure everything else waits on that lock during initialization.

Luckily it is easy to make sem_lock wait on the sma->lock, by pretending
there is a complex operation in progress while the sma is being initialized.

The newary function already zeroes sma->complex_count before unlocking
the sma->lock.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 ipc/sem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ipc/sem.c b/ipc/sem.c
index 454f6c6..1823160 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -507,6 +507,9 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
 		return retval;
 	}
 
+	/* Ensures sem_lock waits on &sma->lock until sma is ready. */
+	sma->complex_count = 1;
+
 	id = ipc_addid(&sem_ids(ns), &sma->sem_perm, ns->sc_semmni);
 	if (id < 0) {
 		ipc_rcu_putref(sma, sem_rcu_free);

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Rafael Aquini]

On Fri, Nov 21, 2014 at 02:52:26PM -0500, Rik van Riel wrote:
> When manipulating just one semaphore with semop, sem_lock only takes that
> single semaphore's lock. This creates a problem during initialization of
> the semaphore array, when the data structures used by sem_lock have not
> been set up yet. The sma->lock is already held by newary, and we just
> have to make sure everything else waits on that lock during initialization.
> 
> Luckily it is easy to make sem_lock wait on the sma->lock, by pretending
> there is a complex operation in progress while the sma is being initialized.
> 
> The newary function already zeroes sma->complex_count before unlocking
> the sma->lock.
> 
> Signed-off-by: Rik van Riel <riel@redhat.com>
> ---
>  ipc/sem.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 454f6c6..1823160 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -507,6 +507,9 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
>  		return retval;
>  	}
>  
> +	/* Ensures sem_lock waits on &sma->lock until sma is ready. */
> +	sma->complex_count = 1;
> +
>  	id = ipc_addid(&sem_ids(ns), &sma->sem_perm, ns->sc_semmni);
>  	if (id < 0) {
>  		ipc_rcu_putref(sma, sem_rcu_free);

Acked-by: Rafael Aquini <aquini@redhat.com>

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Andrew Morton]

On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel <riel@redhat.com> wrote:

> When manipulating just one semaphore with semop, sem_lock only takes that
> single semaphore's lock. This creates a problem during initialization of
> the semaphore array, when the data structures used by sem_lock have not
> been set up yet. The sma->lock is already held by newary, and we just
> have to make sure everything else waits on that lock during initialization.
> 
> Luckily it is easy to make sem_lock wait on the sma->lock, by pretending
> there is a complex operation in progress while the sma is being initialized.
> 
> The newary function already zeroes sma->complex_count before unlocking
> the sma->lock.

What are the runtime effects of the bug?

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/21/2014 03:09 PM, Andrew Morton wrote:
> On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel <riel@redhat.com>
> wrote:
> 
>> When manipulating just one semaphore with semop, sem_lock only
>> takes that single semaphore's lock. This creates a problem during
>> initialization of the semaphore array, when the data structures
>> used by sem_lock have not been set up yet. The sma->lock is
>> already held by newary, and we just have to make sure everything
>> else waits on that lock during initialization.
>> 
>> Luckily it is easy to make sem_lock wait on the sma->lock, by
>> pretending there is a complex operation in progress while the sma
>> is being initialized.
>> 
>> The newary function already zeroes sma->complex_count before
>> unlocking the sma->lock.
> 
> What are the runtime effects of the bug?
> 

NULL pointer dereference in spin_lock from sem_lock,
if it is called before sma->sem_base has been pointed
somewhere valid.

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUb6CnAAoJEM553pKExN6Dh8oH/iFVqwrukMkZp7ViFTC84DVK
m8rw6CWk76kEvi6BWx977nT26e7ZfiOCxrhyy/gETOnUDJMgQrn7cFMFd6Ja/2yG
uGCq5WcvVLDiLw7ij9Rqu4C6aHcICserzgfXwWV0XAa5TZOEqvg6FKZgCUHN6sxM
ek0TV0oq/VQvRwAQk/MFDOv38PydH2LH1Z3wXk7JVlhEMX062a4EoxTAe8Teed2p
X5+mTOl4jezog2rFJxFe0Cp8PxpqAi4f1kDugQKohZ3TpUFqH4VKZYmTtvHvpNDH
oeHjnRv632N8KuU2lvIi7EGJGu0Y+ReyOr+NQtozlRYCTPuY/rezkbBmgVwu4iY=
=CDvy
-----END PGP SIGNATURE-----

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Andrew Morton]

On Fri, 21 Nov 2014 15:29:27 -0500 Rik van Riel <riel@redhat.com> wrote:

> On 11/21/2014 03:09 PM, Andrew Morton wrote:
> > On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel <riel@redhat.com>
> > wrote:
> > 
> >> When manipulating just one semaphore with semop, sem_lock only
> >> takes that single semaphore's lock. This creates a problem during
> >> initialization of the semaphore array, when the data structures
> >> used by sem_lock have not been set up yet. The sma->lock is
> >> already held by newary, and we just have to make sure everything
> >> else waits on that lock during initialization.
> >> 
> >> Luckily it is easy to make sem_lock wait on the sma->lock, by
> >> pretending there is a complex operation in progress while the sma
> >> is being initialized.
> >> 
> >> The newary function already zeroes sma->complex_count before
> >> unlocking the sma->lock.
> > 
> > What are the runtime effects of the bug?
> > 
> 
> NULL pointer dereference in spin_lock from sem_lock,
> if it is called before sma->sem_base has been pointed
> somewhere valid.

Help us out here.  People need to use this description to work out
which kernel versions need the patch and whether to backport the fix
into their various kernels.  Other people will be starting at this
changelog wondering "will this fix the bug my customer has reported".

Is there some bug report people can look at?

What userspace actions trigger this bug?

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/21/2014 03:42 PM, Andrew Morton wrote:
> On Fri, 21 Nov 2014 15:29:27 -0500 Rik van Riel <riel@redhat.com>
> wrote:
> 
>> On 11/21/2014 03:09 PM, Andrew Morton wrote:
>>> On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel
>>> <riel@redhat.com> wrote:
>>> 
>>>> When manipulating just one semaphore with semop, sem_lock
>>>> only takes that single semaphore's lock. This creates a
>>>> problem during initialization of the semaphore array, when
>>>> the data structures used by sem_lock have not been set up
>>>> yet. The sma->lock is already held by newary, and we just
>>>> have to make sure everything else waits on that lock during
>>>> initialization.
>>>> 
>>>> Luckily it is easy to make sem_lock wait on the sma->lock,
>>>> by pretending there is a complex operation in progress while
>>>> the sma is being initialized.
>>>> 
>>>> The newary function already zeroes sma->complex_count before 
>>>> unlocking the sma->lock.
>>> 
>>> What are the runtime effects of the bug?
>>> 
>> 
>> NULL pointer dereference in spin_lock from sem_lock, if it is
>> called before sma->sem_base has been pointed somewhere valid.
> 
> Help us out here.  People need to use this description to work out 
> which kernel versions need the patch and whether to backport the
> fix into their various kernels.  Other people will be starting at
> this changelog wondering "will this fix the bug my customer has
> reported".
> 
> Is there some bug report people can look at?
> 
> What userspace actions trigger this bug?

The reason the bug took almost two years to get noticed is that
it takes one task doing a semop on a semaphore in an array that
is still getting instantiated by newary (getsem) from another
task.

In other words, if you try to use a semaphore array before
getsem returns, you can oops the task that calls semop.

It should not cause any damage to long-living kernel data
structures.

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUb8TZAAoJEM553pKExN6DzJUH/RYSovikk+36KH0uFQN44txj
ZkEM6BsT7I6W9zBiK4OCPpwYCr5gy2xsXH7bLzCgzRV/YmjLFdw20DhDfSo14GO/
1ByYcsUcsZ+lPJZ+g4IKi57VW4T+NLa1T4CoJ84+1QVGKYlpc7mlwc8suTGBhKvQ
5Eq1o1KOE9ZtAG5Go8OYH7frwalkrYE0YJbGN9PW0pUvZ7FilEiMJIkznIetRS6K
WK05dK52DMKeXFxzuxVhSRcCZb2+bHZn3qFOmon6kHbMqgzRZCKMcdydtoIvcFq7
cA5eTt6V6je3XVhc4lsSfP9cHraLDZZIjkaJ856fBpgJ30ypsHcpVY6UKTbFSHo=
=u1Vg
-----END PGP SIGNATURE-----

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Davidlohr Bueso]

On Fri, 2014-11-21 at 18:03 -0500, Rik van Riel wrote:
> On 11/21/2014 03:42 PM, Andrew Morton wrote:
> > On Fri, 21 Nov 2014 15:29:27 -0500 Rik van Riel <riel@redhat.com>
> > wrote:
> > 
> >> On 11/21/2014 03:09 PM, Andrew Morton wrote:
> >>> On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel
> >>> <riel@redhat.com> wrote:
> >>> 
> >>>> When manipulating just one semaphore with semop, sem_lock
> >>>> only takes that single semaphore's lock. This creates a
> >>>> problem during initialization of the semaphore array, when
> >>>> the data structures used by sem_lock have not been set up
> >>>> yet. The sma->lock is already held by newary, and we just
> >>>> have to make sure everything else waits on that lock during
> >>>> initialization.
> >>>> 
> >>>> Luckily it is easy to make sem_lock wait on the sma->lock,
> >>>> by pretending there is a complex operation in progress while
> >>>> the sma is being initialized.
> >>>> 
> >>>> The newary function already zeroes sma->complex_count before 
> >>>> unlocking the sma->lock.
> >>> 
> >>> What are the runtime effects of the bug?
> >>> 
> >> 
> >> NULL pointer dereference in spin_lock from sem_lock, if it is
> >> called before sma->sem_base has been pointed somewhere valid.
> > 
> > Help us out here.  People need to use this description to work out 
> > which kernel versions need the patch and whether to backport the
> > fix into their various kernels.  Other people will be starting at
> > this changelog wondering "will this fix the bug my customer has
> > reported".
> > 
> > Is there some bug report people can look at?

This would be nice for the changelog...

> > 
> > What userspace actions trigger this bug?
> 
> The reason the bug took almost two years to get noticed is that
> it takes one task doing a semop on a semaphore in an array that
> is still getting instantiated by newary (getsem) from another
> task.
> 
> In other words, if you try to use a semaphore array before
> getsem returns, you can oops the task that calls semop.

This seems bogus from an application level: how can you call semop if
you don't have the semid yet returned from semget? And the fact that the
race is with newary, means that the call is in fact creating a *new*
set, as opposed to plugging into an already existing set.

The fix in newary() being before the actual creation of the id seems
even stranger:

	sma->complex_count = 1;
	id = ipc_addid(&sem_ids(ns), &sma->sem_perm, ns->sc_semmni);

As for semtimedop() before even getting to sem_lock(), we first call:

	sma = sem_obtain_object_check(ns, semid);

So shouldn't that fail anyway before we even consider acquiring the lock?

Thanks,
Davidlohr

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/21/2014 07:56 PM, Davidlohr Bueso wrote:
> On Fri, 2014-11-21 at 18:03 -0500, Rik van Riel wrote:

>> In other words, if you try to use a semaphore array before getsem
>> returns, you can oops the task that calls semop.
> 
> This seems bogus from an application level: how can you call semop
> if you don't have the semid yet returned from semget? And the fact
> that the race is with newary, means that the call is in fact
> creating a *new* set, as opposed to plugging into an already
> existing set.

Agreed, this is bogus from userspace.

However, userspace doing bogus things should not lead to a
kernel crash.

> The fix in newary() being before the actual creation of the id
> seems even stranger:
> 
> sma->complex_count = 1; id = ipc_addid(&sem_ids(ns),
> &sma->sem_perm, ns->sc_semmni);
> 
> As for semtimedop() before even getting to sem_lock(), we first
> call:
> 
> sma = sem_obtain_object_check(ns, semid);
> 
> So shouldn't that fail anyway before we even consider acquiring the
> lock?

newary initializes a bunch of things after the call to
ipc_addid, however some things are initialized inside
ipc_addid as well

Looking closer at newary, I suppose that it should be
possible to move those other initializations before
the call to ipc_addid.  That would likely get rid of
the problem, too.

However, I also see this line in newary, and I have
no idea what protects that data:

        ns->used_sems += nsems;

I don't see any locking around ns->used_sems for
simultaneous getsem & RMID...

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUcAWfAAoJEM553pKExN6D4n4H/jogtT4f/cWvMI4be3MlfE2x
sAIuC0Z6Fqqzm60XB2OB4/yIAZU1JDmsUrmUVqwh3R/G2mQygpkrM9ZKW4dkxtyd
MZ0IWtx74OSb376mDcmhk8vI8xh5/j/bWTx2oxP7IFZf4imVFGeZmlG/YLKGSnLS
lO9ehr9wkyzoyo1wgpuWhKdxDTEaeZd8C6Ij6bVylWybuWVripN9eX13vWyDmKJ8
P754efTIDu+PWCaEdNA7eKTMlydkXqjPwUpSnSE/bs2ngFhlAkZqkWmTEu54Wc32
yoyEqFNdMvAV8QCHLeR8Uqf53PNhncz7S7RfX58wgdQ5bKO3ATuJ8jbTT5ZXVZ8=
=xg+y
-----END PGP SIGNATURE-----

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Manfred Spraul]

Hi Rik,

good catch - I completely forgot to check the initialization

On 11/22/2014 04:40 AM, Rik van Riel wrote:
>
> newary initializes a bunch of things after the call to
> ipc_addid, however some things are initialized inside
> ipc_addid as well
>
> Looking closer at newary, I suppose that it should be
> possible to move those other initializations before
> the call to ipc_addid.  That would likely get rid of
> the problem, too.
>
> However, I also see this line in newary, and I have
> no idea what protects that data:
>
>          ns->used_sems += nsems;
It should be sem_ids.rwsem, and at least according to the documentation 
both freeary() and newary() hold it.


--
     Manfred

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/22/2014 08:56 AM, Manfred Spraul wrote:
> Hi Rik,
> 
> good catch - I completely forgot to check the initialization
> 
> On 11/22/2014 04:40 AM, Rik van Riel wrote:
>> 
>> newary initializes a bunch of things after the call to ipc_addid,
>> however some things are initialized inside ipc_addid as well
>> 
>> Looking closer at newary, I suppose that it should be possible to
>> move those other initializations before the call to ipc_addid.
>> That would likely get rid of the problem, too.
>> 
>> However, I also see this line in newary, and I have no idea what
>> protects that data:
>> 
>> ns->used_sems += nsems;
> It should be sem_ids.rwsem, and at least according to the
> documentation both freeary() and newary() hold it.

You're right, that is properly protected already.

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUcLFwAAoJEM553pKExN6DoGwIAJVr0SJWq1sxkRr6quw03OlU
8GUNm8Mz4tVpt1PYSe5r3SIGeT0btEhXjRrqp3IOw7z85Iw5Ed2CwzuLS3aunmpg
hZag9qyArxKBQkbnhtqiM/0AEHbRiju3+sFNC1cQxsdRvLV6QkRveoP5kKvv4v6d
PCMJBHuxL01tPXe5eQC2WjLmI2YPgWWh3fXhDcLt2XTVBI0vwyBiVFc/CtSPERo0
sBF01l/l5KRfOAHduW2kj7KrSF004IgLU7KN5fLG0BzvwlowcHjXgzX7mMCbZrm2
8d8bdEtRJxDZqs4Z9XjDrjXfSdWH/H/5CnfAmvgXA4pswb1lLDLPFbgmt7NUtxk=
=vUfd
-----END PGP SIGNATURE-----

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Manfred Spraul]

On 11/21/2014 09:29 PM, Rik van Riel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 11/21/2014 03:09 PM, Andrew Morton wrote:
>> On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel <riel@redhat.com>
>> wrote:
>>
>>> When manipulating just one semaphore with semop, sem_lock only
>>> takes that single semaphore's lock. This creates a problem during
>>> initialization of the semaphore array, when the data structures
>>> used by sem_lock have not been set up yet. The sma->lock is
>>> already held by newary, and we just have to make sure everything
>>> else waits on that lock during initialization.
>>>
>>> Luckily it is easy to make sem_lock wait on the sma->lock, by
>>> pretending there is a complex operation in progress while the sma
>>> is being initialized.
>>>
>>> The newary function already zeroes sma->complex_count before
>>> unlocking the sma->lock.
>> What are the runtime effects of the bug?
>>
> NULL pointer dereference in spin_lock from sem_lock,
> if it is called before sma->sem_base has been pointed
> somewhere valid.
No, this can't happen:
- sma is initialized to 0 with memset()
- sma->sem_nsems is set last.
- semtimedop() contains a "max >= sma->sem_nsems".

with sma->sem_nsems==0, this will always fail and therefore sem_lock() 
can't be reached.

The only misbehavior (apart from returning -EFBIG) is that 
find_alloc_undo() could allocate a wrong-sized undo structure.
Would cause random memory corruptions - but not NULL pointer dereference.

Which which kernel version have you seen the NULL pointer dereference?

--
     Manfred

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/22/2014 02:14 PM, Manfred Spraul wrote:
> On 11/21/2014 09:29 PM, Rik van Riel wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>> 
>> On 11/21/2014 03:09 PM, Andrew Morton wrote:
>>> On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel
>>> <riel@redhat.com> wrote:
>>> 
>>>> When manipulating just one semaphore with semop, sem_lock
>>>> only takes that single semaphore's lock. This creates a
>>>> problem during initialization of the semaphore array, when
>>>> the data structures used by sem_lock have not been set up
>>>> yet. The sma->lock is already held by newary, and we just
>>>> have to make sure everything else waits on that lock during
>>>> initialization.
>>>> 
>>>> Luckily it is easy to make sem_lock wait on the sma->lock,
>>>> by pretending there is a complex operation in progress while
>>>> the sma is being initialized.
>>>> 
>>>> The newary function already zeroes sma->complex_count before 
>>>> unlocking the sma->lock.
>>> What are the runtime effects of the bug?
>>> 
>> NULL pointer dereference in spin_lock from sem_lock, if it is
>> called before sma->sem_base has been pointed somewhere valid.
> No, this can't happen: - sma is initialized to 0 with memset() -
> sma->sem_nsems is set last. - semtimedop() contains a "max >=
> sma->sem_nsems".
> 
> with sma->sem_nsems==0, this will always fail and therefore
> sem_lock() can't be reached.

You're right. The reported race must have been semop vs RMID.

The kernel tree in question was missing this changeset:

commit 6e224f94597842c5eb17f1fc2208d20b6f7f7d49
Author: Manfred Spraul <manfred@colorfullife.com>
Date:   Wed Oct 16 13:46:45 2013 -0700

    ipc/sem.c: synchronize semop and semctl with IPC_RMID


- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEbBAEBAgAGBQJUcO6rAAoJEM553pKExN6DPXkH+Ot5H94no3DJ6b1WdhOhDMUM
sQaWErEcSJ2dxzVES4WUMzqnnEZPokG2uK4z2PVUWjE+YA1U7hGfctLg/Eabr5tV
tD+uZhrbSbJVT7HiS5wyqmyzCV5eUV+2Am19pqwa6gyfB30cAYA/GtYfnMhKRGR0
l9hcvyzhci59d/2V2/Y5cGrxvQaWued33JZYfjp2TCl1GDpPD1bocptc3BO0DbwO
iHMZBcWfjR5t/EJ2Pg9gwu8X4C7amHsaNM58yTU6o93dE4bpS//A7WtwlLHJ/WEE
tD9zoOMnv7o8B5AHl3UDUJJ+JjieQU498AC3IganXQE8WrsZMJWZXo1OZtQP7A==
=vZEa
-----END PGP SIGNATURE-----

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Manfred Spraul]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

Hi Rik,

On 11/21/2014 08:52 PM, Rik van Riel wrote:
> When manipulating just one semaphore with semop, sem_lock only takes that
> single semaphore's lock. This creates a problem during initialization of
> the semaphore array, when the data structures used by sem_lock have not
> been set up yet. The sma->lock is already held by newary, and we just
> have to make sure everything else waits on that lock during initialization.
>
> Luckily it is easy to make sem_lock wait on the sma->lock, by pretending
> there is a complex operation in progress while the sma is being initialized.
That's not sufficient, as sma->sem_nsems is accessed before calling 
sem_lock(), both within find_alloc_undo() and within semtimedop().

The root problem is that sma->sem_nsems and sma->sem_base are accessed 
without any locks, this conflicts with the approach that sma starts to 
exist as not yet initialized but locked and is unlocked after the 
initialization is completed.

Attached is an idea. It did pass a few short tests.
What do you think?

With regards to affected kernels:
- wrong -EFBIG are possible since 3.10 (test for sma->sem_nsems moved 
before taking the lock)
- kernel memory corruptions with 0-sized undo buffer allocation is 
possible since 3.10, too.
   (sem_lock before accessing sma->sem_nsems replaced with 
sem_obtain_object_check).


--
     Manfred

[-- Attachment #2: 0001-ipc-sem.c-Fully-initialize-sem_array-before-making-i.patch --]
[-- Type: text/x-patch, Size: 1642 bytes --]

>From fa928cdd6b5e032006f100f9689a5a4167c086e8 Mon Sep 17 00:00:00 2001
From: Manfred Spraul <manfred@colorfullife.com>
Date: Sun, 23 Nov 2014 19:08:57 +0100
Subject: [PATCH] ipc/sem.c: Fully initialize sem_array before making it
 visible

ipc_addid() makes a new ipc identifier visible to everyone.
New objects start as locked, so that the caller can complete
the initialization after the call.
Within struct sem_array, at least sma->sem_base and sma->sem_nsems
are accessed without any locks, therefore this approach doesn't work.

Thus: Move the ipc_addid() to the end of the initialization.

Reported-by: Rik van Riel <riel@redhat.com>
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/sem.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 454f6c6..53c3310 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -507,13 +507,6 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
 		return retval;
 	}
 
-	id = ipc_addid(&sem_ids(ns), &sma->sem_perm, ns->sc_semmni);
-	if (id < 0) {
-		ipc_rcu_putref(sma, sem_rcu_free);
-		return id;
-	}
-	ns->used_sems += nsems;
-
 	sma->sem_base = (struct sem *) &sma[1];
 
 	for (i = 0; i < nsems; i++) {
@@ -528,6 +521,14 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
 	INIT_LIST_HEAD(&sma->list_id);
 	sma->sem_nsems = nsems;
 	sma->sem_ctime = get_seconds();
+
+	id = ipc_addid(&sem_ids(ns), &sma->sem_perm, ns->sc_semmni);
+	if (id < 0) {
+		ipc_rcu_putref(sma, sem_rcu_free);
+		return id;
+	}
+	ns->used_sems += nsems;
+
 	sem_unlock(sma, -1);
 	rcu_read_unlock();
 
-- 
1.9.3

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/23/2014 01:23 PM, Manfred Spraul wrote:
> Hi Rik,
> 
> On 11/21/2014 08:52 PM, Rik van Riel wrote:
>> When manipulating just one semaphore with semop, sem_lock only
>> takes that single semaphore's lock. This creates a problem during
>> initialization of the semaphore array, when the data structures
>> used by sem_lock have not been set up yet. The sma->lock is
>> already held by newary, and we just have to make sure everything
>> else waits on that lock during initialization.
>> 
>> Luckily it is easy to make sem_lock wait on the sma->lock, by
>> pretending there is a complex operation in progress while the sma
>> is being initialized.
> That's not sufficient, as sma->sem_nsems is accessed before
> calling sem_lock(), both within find_alloc_undo() and within
> semtimedop().
> 
> The root problem is that sma->sem_nsems and sma->sem_base are
> accessed without any locks, this conflicts with the approach that
> sma starts to exist as not yet initialized but locked and is
> unlocked after the initialization is completed.
> 
> Attached is an idea. It did pass a few short tests. What do you
> think?

This was my other idea for fixing the issue; unfortunately
I didn't think of it until after I sent the first patch :)

You are right that without that change, we can return the
wrong error codes to userspace.

I will give the patch a try, though I have so far been unable
to reproduce the bug that the customer reported, so I am unlikely
to give much in the way of useful testing results...

Andrew, feel free to give Manfred's patch my

Acked-by: Rik van Riel <riel@redhat.com>

It closes off more things than my patch did, in a similar way
(doing something before ipc_addid)

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUckuKAAoJEM553pKExN6DfRoH/jjcBpLwKzrId8aFtuSWYgv3
I7LNzoGozb6Lvn1D1lt/sREdl74KIQptaVClQzphIubiAkQPJVgzqe43f01mqaPu
mSjGKarjfKXwUNfa+Q3pW7b2hnbdfTfyQDsTr6JhSpy0ynsmmNb1J2S7E2E2B40r
KbOETKeKRTq4vYCk4Em5S4OJg31AoXNoXMVwciOB0M/QDFCSB+4JdKrRsiz6Hm1o
+JlRtgwUY6m047Ur65MKeN1bkx1cgqK8tzayMpXW+PTI9cVs6153tlWXmQoXP+Co
lHrODmKkhOhJfOr3q0YvrwnaZOVKliKLVV0YkQ+6pi4SAqSjH7pV0jWr/FsaLIA=
=u6vO
-----END PGP SIGNATURE-----

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Davidlohr Bueso]

On Sun, 2014-11-23 at 16:03 -0500, Rik van Riel wrote:
> On 11/23/2014 01:23 PM, Manfred Spraul wrote:
> > Hi Rik,
> > 
> > On 11/21/2014 08:52 PM, Rik van Riel wrote:
> >> When manipulating just one semaphore with semop, sem_lock only
> >> takes that single semaphore's lock. This creates a problem during
> >> initialization of the semaphore array, when the data structures
> >> used by sem_lock have not been set up yet. The sma->lock is
> >> already held by newary, and we just have to make sure everything
> >> else waits on that lock during initialization.
> >> 
> >> Luckily it is easy to make sem_lock wait on the sma->lock, by
> >> pretending there is a complex operation in progress while the sma
> >> is being initialized.
> > That's not sufficient, as sma->sem_nsems is accessed before
> > calling sem_lock(), both within find_alloc_undo() and within
> > semtimedop().
> > 
> > The root problem is that sma->sem_nsems and sma->sem_base are
> > accessed without any locks, this conflicts with the approach that
> > sma starts to exist as not yet initialized but locked and is
> > unlocked after the initialization is completed.
> > 
> > Attached is an idea. It did pass a few short tests. What do you
> > think?
> 
> This was my other idea for fixing the issue; unfortunately
> I didn't think of it until after I sent the first patch :)

Yep, this is what I was mentioning as well.

> You are right that without that change, we can return the
> wrong error codes to userspace.
> 
> I will give the patch a try, though I have so far been unable
> to reproduce the bug that the customer reported, so I am unlikely
> to give much in the way of useful testing results...
> 
> Andrew, feel free to give Manfred's patch my
> 
> Acked-by: Rik van Riel <riel@redhat.com>

Acked-by: Davidlohr Bueso <dave@stgolabs.net>

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Rafael Aquini]

On Sun, Nov 23, 2014 at 01:36:51PM -0800, Davidlohr Bueso wrote:
> On Sun, 2014-11-23 at 16:03 -0500, Rik van Riel wrote:
> > On 11/23/2014 01:23 PM, Manfred Spraul wrote:
> > > Hi Rik,
> > > 
> > > On 11/21/2014 08:52 PM, Rik van Riel wrote:
> > >> When manipulating just one semaphore with semop, sem_lock only
> > >> takes that single semaphore's lock. This creates a problem during
> > >> initialization of the semaphore array, when the data structures
> > >> used by sem_lock have not been set up yet. The sma->lock is
> > >> already held by newary, and we just have to make sure everything
> > >> else waits on that lock during initialization.
> > >> 
> > >> Luckily it is easy to make sem_lock wait on the sma->lock, by
> > >> pretending there is a complex operation in progress while the sma
> > >> is being initialized.
> > > That's not sufficient, as sma->sem_nsems is accessed before
> > > calling sem_lock(), both within find_alloc_undo() and within
> > > semtimedop().
> > > 
> > > The root problem is that sma->sem_nsems and sma->sem_base are
> > > accessed without any locks, this conflicts with the approach that
> > > sma starts to exist as not yet initialized but locked and is
> > > unlocked after the initialization is completed.
> > > 
> > > Attached is an idea. It did pass a few short tests. What do you
> > > think?
> > 
> > This was my other idea for fixing the issue; unfortunately
> > I didn't think of it until after I sent the first patch :)
> 
> Yep, this is what I was mentioning as well.
> 
> > You are right that without that change, we can return the
> > wrong error codes to userspace.
> > 
> > I will give the patch a try, though I have so far been unable
> > to reproduce the bug that the customer reported, so I am unlikely
> > to give much in the way of useful testing results...
> > 
> > Andrew, feel free to give Manfred's patch my
> > 
> > Acked-by: Rik van Riel <riel@redhat.com>
> 
> Acked-by: Davidlohr Bueso <dave@stgolabs.net>
> 
Acked-by: Rafael Aquini <aquini@redhat.com>

Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Andrew Morton]

On Sun, 23 Nov 2014 19:23:53 +0100 Manfred Spraul <manfred@colorfullife.com> wrote:

> Subject: [PATCH] ipc/sem.c: Fully initialize sem_array before making it
>  visible
> 
> ipc_addid() makes a new ipc identifier visible to everyone.
> New objects start as locked, so that the caller can complete
> the initialization after the call.
> Within struct sem_array, at least sma->sem_base and sma->sem_nsems
> are accessed without any locks, therefore this approach doesn't work.
> 
> Thus: Move the ipc_addid() to the end of the initialization.

Any thoughts on which kernel version(s) need the patch?  I'm still
rather fuzzy on the end-user impact of this bug.