This is a note to let you know that I've just added the patch titled
mctp: prevent double key removal and unref
to the 5.19-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
mctp-prevent-double-key-removal-and-unref.patch
and it can be found in the queue-5.19 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@xxxxxxxxxxxxxxx> know about it.
>From 3a732b46736cd8a29092e4b0b1a9ba83e672bf89 Mon Sep 17 00:00:00 2001
From: Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx>
Date: Wed, 12 Oct 2022 10:08:51 +0800
Subject: mctp: prevent double key removal and unref
From: Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx>
commit 3a732b46736cd8a29092e4b0b1a9ba83e672bf89 upstream.
Currently, we have a bug where a simultaneous DROPTAG ioctl and socket
close may race, as we attempt to remove a key from lists twice, and
perform an unref for each removal operation. This may result in a uaf
when we attempt the second unref.
This change fixes the race by making __mctp_key_remove tolerant to being
called on a key that has already been removed from the socket/net lists,
and only performs the unref when we do the actual remove. We also need
to hold the list lock on the ioctl cleanup path.
This fix is based on a bug report and comprehensive analysis from
butt3rflyh4ck <butterflyhuangxx@xxxxxxxxx>, found via syzkaller.
Cc: stable@xxxxxxxxxxxxxxx
Fixes: 63ed1aab3d40 ("mctp: Add SIOCMCTP{ALLOC,DROP}TAG ioctls for tag control")
Reported-by: butt3rflyh4ck <butterflyhuangxx@xxxxxxxxx>
Signed-off-by: Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
net/mctp/af_mctp.c | 23 ++++++++++++++++-------
net/mctp/route.c | 10 +++++-----
2 files changed, 21 insertions(+), 12 deletions(-)
--- a/net/mctp/af_mctp.c
+++ b/net/mctp/af_mctp.c
@@ -295,11 +295,12 @@ __must_hold(&net->mctp.keys_lock)
mctp_dev_release_key(key->dev, key);
spin_unlock_irqrestore(&key->lock, flags);
- hlist_del(&key->hlist);
- hlist_del(&key->sklist);
-
- /* unref for the lists */
- mctp_key_unref(key);
+ if (!hlist_unhashed(&key->hlist)) {
+ hlist_del_init(&key->hlist);
+ hlist_del_init(&key->sklist);
+ /* unref for the lists */
+ mctp_key_unref(key);
+ }
kfree_skb(skb);
}
@@ -373,9 +374,17 @@ static int mctp_ioctl_alloctag(struct mc
ctl.tag = tag | MCTP_TAG_OWNER | MCTP_TAG_PREALLOC;
if (copy_to_user((void __user *)arg, &ctl, sizeof(ctl))) {
- spin_lock_irqsave(&key->lock, flags);
- __mctp_key_remove(key, net, flags, MCTP_TRACE_KEY_DROPPED);
+ unsigned long fl2;
+ /* Unwind our key allocation: the keys list lock needs to be
+ * taken before the individual key locks, and we need a valid
+ * flags value (fl2) to pass to __mctp_key_remove, hence the
+ * second spin_lock_irqsave() rather than a plain spin_lock().
+ */
+ spin_lock_irqsave(&net->mctp.keys_lock, flags);
+ spin_lock_irqsave(&key->lock, fl2);
+ __mctp_key_remove(key, net, fl2, MCTP_TRACE_KEY_DROPPED);
mctp_key_unref(key);
+ spin_unlock_irqrestore(&net->mctp.keys_lock, flags);
return -EFAULT;
}
--- a/net/mctp/route.c
+++ b/net/mctp/route.c
@@ -228,12 +228,12 @@ __releases(&key->lock)
if (!key->manual_alloc) {
spin_lock_irqsave(&net->mctp.keys_lock, flags);
- hlist_del(&key->hlist);
- hlist_del(&key->sklist);
+ if (!hlist_unhashed(&key->hlist)) {
+ hlist_del_init(&key->hlist);
+ hlist_del_init(&key->sklist);
+ mctp_key_unref(key);
+ }
spin_unlock_irqrestore(&net->mctp.keys_lock, flags);
-
- /* unref for the lists */
- mctp_key_unref(key);
}
/* and one for the local reference */
Patches currently in stable-queue which might be from jk@xxxxxxxxxxxxxxxxxxxx are
queue-5.19/mctp-prevent-double-key-removal-and-unref.patch
