| Message ID | 1399646109-16362-2-git-send-email-luis.henriques@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Fri, May 09, 2014 at 03:35:09PM +0100, Luis Henriques wrote: > From: Konstantin Khlebnikov <k.khlebnikov@samsung.com> > > 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 <k.khlebnikov@samsung.com> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > (cherry picked from 91182754daa6ca26dd2e97ee0b0f6e9e37d33324 3.2.y) > CVE-2013-4483 > BugLink: http://bugs.launchpad.net/bugs/1248713 > Signed-off-by: Luis Henriques <luis.henriques@canonical.com> > --- > ipc/msg.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/ipc/msg.c b/ipc/msg.c > index 779f762..75075fc 100644 > --- a/ipc/msg.c > +++ b/ipc/msg.c > @@ -297,7 +297,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); > } > > /* Matches the fix as applied to P & Q. Acked-by: Andy Whitcroft <apw@canonical.com> -apw
On 05/09/2014 09:35 AM, Luis Henriques wrote: > From: Konstantin Khlebnikov <k.khlebnikov@samsung.com> > > 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 <k.khlebnikov@samsung.com> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > (cherry picked from 91182754daa6ca26dd2e97ee0b0f6e9e37d33324 3.2.y) > CVE-2013-4483 > BugLink: http://bugs.launchpad.net/bugs/1248713 > Signed-off-by: Luis Henriques <luis.henriques@canonical.com> > --- > ipc/msg.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/ipc/msg.c b/ipc/msg.c > index 779f762..75075fc 100644 > --- a/ipc/msg.c > +++ b/ipc/msg.c > @@ -297,7 +297,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); > } > > /* >
On Fri, May 09, 2014 at 03:35:09PM +0100, Luis Henriques wrote: > From: Konstantin Khlebnikov <k.khlebnikov@samsung.com> > > 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 <k.khlebnikov@samsung.com> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > (cherry picked from 91182754daa6ca26dd2e97ee0b0f6e9e37d33324 3.2.y) > CVE-2013-4483 > BugLink: http://bugs.launchpad.net/bugs/1248713 > Signed-off-by: Luis Henriques <luis.henriques@canonical.com> > --- > ipc/msg.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/ipc/msg.c b/ipc/msg.c > index 779f762..75075fc 100644 > --- a/ipc/msg.c > +++ b/ipc/msg.c > @@ -297,7 +297,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.9.1 > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team This fix should be also applied to Precise/lts-backport-raring (thanks for noticing Andy!). As the author of the patch stated, it applies to kernels <= 3.10, which includes Raring 3.8 :-) Cheers, -- Luís
diff --git a/ipc/msg.c b/ipc/msg.c index 779f762..75075fc 100644 --- a/ipc/msg.c +++ b/ipc/msg.c @@ -297,7 +297,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); } /*