Skip to contentSkip to conversation

ipc/msg: fix race around refcount

PATCH Applied Konstantin Khlebnikov 3 participants 3 messages Mar 26, 2014 – Apr 6, 2014 Stable branch

lore
Share & tools
lore

Konstantin Khlebnikov

author
#0 lore raw

PATCH 1/1 ipc/msg: fix race around refcount

In older kernels (before v3.10) ipc_rcu_hdr->refcount was non-atomic int. There was possuble double-free bug: do_msgsnd() calls ipc_rcu_putref() under msq->q_perm->lock and RCU, while freequeue() calls it while it holds only 'rw_mutex', so there is no sinchronization between them. Two function decrements '2' non-atomically, they both can get '0' as result.

do_msgsnd() freequeue()

msq = msg_lock_check(ns, msqid);
...
ipc_rcu_getref(msq);
msg_unlock(msq);

schedule();
						(caller locks spinlock)
						expunge_all(msq, -EIDRM);
						ss_wakeup(&msq->q_senders, 1);
						msg_rmid(ns, msq);
						msg_unlock(msq);

ipc_lock_by_ptr(&msq->q_perm);
ipc_rcu_putref(msq); ipc_rcu_putref(msq);
< both may get get --(...)->refcount == 0 >

This patch locks ipc_lock and RCU around ipc_rcu_putref in freequeue. ( RCU protects memory for spin_unlock() )

Similar bugs might be in other users of ipc_rcu_putref().

In the mainline this has been fixed in v3.10 indirectly in commmit 6062a8dc0517bce23e3c2f7d2fea5e22411269a3
("ipc,sem: fine grained locking for semtimedop") by Rik van Riel. That commit optimized locking and converted refcount into atomic.

I'm not sure that anybody should care about this bug: it's very-very unlikely and no longer exists in actual mainline. I've found this just by looking into the code, probably this never happens in real life.

Signed-off-by Konstantin Khlebnikov <[email protected]>
---
 ipc/msg.c |    2 ++
 1 file changed, 2 insertions(+)
ipc/msg.c +2 -0
index 7385de2..25f1a61 100644
@@ -296,7 +296,9 @@ static void freeque(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp)
 	}
 	atomic_sub(msq->q_cbytes, &ns->msg_bytes);
 	security_msg_queue_free(msq);
+	ipc_lock_by_ptr(&msq->q_perm);
 	ipc_rcu_putref(msq);
+	ipc_unlock(&msq->q_perm);
 }
 
 /*
#1 lore raw
Quoted 32 lines · Konstantin Khlebnikov

In older kernels (before v3.10) ipc_rcu_hdr->refcount was non-atomic int. There was possuble double-free bug: do_msgsnd() calls ipc_rcu_putref() under msq->q_perm->lock and RCU, while freequeue() calls it while it holds only 'rw_mutex', so there is no sinchronization between them. Two function decrements '2' non-atomically, they both can get '0' as result.

do_msgsnd() freequeue()

msq = msg_lock_check(ns, msqid);
...
ipc_rcu_getref(msq);
msg_unlock(msq);

schedule();
						(caller locks spinlock)
						expunge_all(msq, -EIDRM);
						ss_wakeup(&msq->q_senders, 1);
						msg_rmid(ns, msq);
						msg_unlock(msq);

ipc_lock_by_ptr(&msq->q_perm);
ipc_rcu_putref(msq); ipc_rcu_putref(msq);
< both may get get --(...)->refcount == 0 >

This patch locks ipc_lock and RCU around ipc_rcu_putref in freequeue. ( RCU protects memory for spin_unlock() )

Similar bugs might be in other users of ipc_rcu_putref().

In the mainline this has been fixed in v3.10 indirectly in commmit 6062a8dc0517bce23e3c2f7d2fea5e22411269a3
("ipc,sem: fine grained locking for semtimedop") by Rik van Riel. That commit optimized locking and converted refcount into atomic.

Quoted 5 lines

I'm not sure that anybody should care about this bug: it's very-very unlikely and no longer exists in actual mainline. I've found this just by looking into the code, probably this never happens in real life.

Signed-off-by Konstantin Khlebnikov <[email protected]>

Thank you, I'm queuing this for the 3.5 kernel.

Cheers,
--
Lu�s

Quoted 23 lines
---
 ipc/msg.c |    2 ++
 1 file changed, 2 insertions(+)
ipc/msg.c +2 -1
index 7385de2..25f1a61 100644
@@ -296,7 +296,9 @@ static void freeque(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp)
 	}
 	atomic_sub(msq->q_cbytes, &ns->msg_bytes);
 	security_msg_queue_free(msq);
+	ipc_lock_by_ptr(&msq->q_perm);
 	ipc_rcu_putref(msq);
+	ipc_unlock(&msq->q_perm);
 }
 
 /*
 
--

To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Applied
#2 lore raw
In reply to Konstantin Khlebnikov
Quoted 5 lines · Konstantin Khlebnikov

In older kernels (before v3.10) ipc_rcu_hdr->refcount was non-atomic int. There was possuble double-free bug: do_msgsnd() calls ipc_rcu_putref() under msq->q_perm->lock and RCU, while freequeue() calls it while it holds only 'rw_mutex', so there is no sinchronization between them. Two function decrements '2' non-atomically, they both can get '0' as result.

[...]

Quoted 8 lines

In the mainline this has been fixed in v3.10 indirectly in commmit 6062a8dc0517bce23e3c2f7d2fea5e22411269a3
("ipc,sem: fine grained locking for semtimedop") by Rik van Riel. That commit optimized locking and converted refcount into atomic.

I'm not sure that anybody should care about this bug: it's very-very unlikely and no longer exists in actual mainline. I've found this just by looking into the code, probably this never happens in real life.

Queued up for 3.2, thanks.

Ben.

Quoted 19 lines
Signed-off-by Konstantin Khlebnikov <[email protected]>
---
 ipc/msg.c |    2 ++
 1 file changed, 2 insertions(+)
ipc/msg.c +2 -0
index 7385de2..25f1a61 100644
@@ -296,7 +296,9 @@ static void freeque(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp)
 	}
 	atomic_sub(msq->q_cbytes, &ns->msg_bytes);
 	security_msg_queue_free(msq);
+	ipc_lock_by_ptr(&msq->q_perm);
 	ipc_rcu_putref(msq);
+	ipc_unlock(&msq->q_perm);
 }
 
 /*
Signature · 2 lines
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.