[PATCH net 0/2] net/smc: fix potential UAF in smc_pnet_add_ib

[PATCH net 0/2] net/smc: fix potential UAF in smc_pnet_add_ib

[Li Xiasong]

This series fixes potential use-after-free issues in smc_pnet_add_ib()
where the device pointer could be freed between find and apply operations.

The race occurs because smc_pnet_find_ib() and smc_pnet_find_smcd()
release the mutex before returning the device pointer. If the device is
removed (e.g., via smc_ib_remove_dev() or smcd_unregister_dev()) before
smc_pnet_apply_ib() or smc_pnet_apply_smcd() is called, the freed
pointer will be accessed.

Patch 1 fixes the issue for ib device, and patch 2 fixes the same issue
for smcd device.

Li Xiasong (2):
  net/smc: fix potential UAF in smc_pnet_add_ib for ib device
  net/smc: fix potential UAF in smc_pnet_add_ib for smcd device

 net/smc/smc_pnet.c | 121 ++++++++++++++++++++++++++++-----------------
 1 file changed, 75 insertions(+), 46 deletions(-)

-- 
2.34.1

[PATCH net 1/2] net/smc: fix potential UAF in smc_pnet_add_ib for ib device

[Li Xiasong]

smc_pnet_find_ib() returns an ib device pointer and releases the lock,
then smc_pnet_apply_ib() is called to use this pointer. The device could
be removed between these two calls, leading to a potential use-after-free
when accessing the freed ib_dev pointer.

    CPU 0                           CPU 1
    ----                            ----
    smc_pnet_add_ib()
      ib_dev = smc_pnet_find_ib()
        mutex_lock(&smc_ib_devices.mutex)
        list_for_each_entry() ...
        mutex_unlock(&smc_ib_devices.mutex)
                                    smc_ib_remove_dev()
                                      mutex_lock(&smc_ib_devices.mutex)
                                      list_del_init(&smcibdev->list)
                                      mutex_unlock(&smc_ib_devices.mutex)
                                      kfree(smcibdev)
      smc_pnet_apply_ib(ib_dev)
        ib_dev->pnetid[ib_port - 1]   <- UAF (ib_dev already freed)

Fix this by introducing smc_pnet_find_ib_apply() which performs both
find and apply under the same lock, preventing the device from being
removed in between.

Also refactor smc_pnet_apply_ib() into __smc_pnet_apply_ib() (without
lock) and smc_pnet_apply_ib() (with lock) for reuse.

Fixes: 890a2cb4a966 ("net/smc: rework pnet table")
Signed-off-by: Li Xiasong <lixiasong1@huawei.com>
---
 net/smc/smc_pnet.c | 63 ++++++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 24 deletions(-)

diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
index 63e286e2dfaa..91c0b1c473b2 100644
--- a/net/smc/smc_pnet.c
+++ b/net/smc/smc_pnet.c
@@ -245,18 +245,25 @@ static int smc_pnet_remove_by_ndev(struct net_device *ndev)
 
 /* Apply pnetid to ib device when no pnetid is set.
  */
-static bool smc_pnet_apply_ib(struct smc_ib_device *ib_dev, u8 ib_port,
-			      char *pnet_name)
+static bool __smc_pnet_apply_ib(struct smc_ib_device *ib_dev, u8 ib_port,
+				char *pnet_name)
 {
-	bool applied = false;
-
-	mutex_lock(&smc_ib_devices.mutex);
 	if (!smc_pnet_is_pnetid_set(ib_dev->pnetid[ib_port - 1])) {
 		memcpy(ib_dev->pnetid[ib_port - 1], pnet_name,
 		       SMC_MAX_PNETID_LEN);
 		ib_dev->pnetid_by_user[ib_port - 1] = true;
-		applied = true;
+		return true;
 	}
+	return false;
+}
+
+static bool smc_pnet_apply_ib(struct smc_ib_device *ib_dev, u8 ib_port,
+			      char *pnet_name)
+{
+	bool applied;
+
+	mutex_lock(&smc_ib_devices.mutex);
+	applied = __smc_pnet_apply_ib(ib_dev, ib_port, pnet_name);
 	mutex_unlock(&smc_ib_devices.mutex);
 	return applied;
 }
@@ -305,24 +312,42 @@ static bool smc_pnetid_valid(const char *pnet_name, char *pnetid)
 }
 
 /* Find an infiniband device by a given name. The device might not exist. */
-static struct smc_ib_device *smc_pnet_find_ib(char *ib_name)
+static struct smc_ib_device *__smc_pnet_find_ib(char *ib_name)
 {
 	struct smc_ib_device *ibdev;
 
-	mutex_lock(&smc_ib_devices.mutex);
 	list_for_each_entry(ibdev, &smc_ib_devices.list, list) {
 		if (!strncmp(ibdev->ibdev->name, ib_name,
 			     sizeof(ibdev->ibdev->name)) ||
 		    (ibdev->ibdev->dev.parent &&
 		     !strncmp(dev_name(ibdev->ibdev->dev.parent), ib_name,
 			     IB_DEVICE_NAME_MAX - 1))) {
-			goto out;
+			return ibdev;
 		}
 	}
-	ibdev = NULL;
-out:
+	return NULL;
+}
+
+/* Find an ib device by name and apply pnetid under lock. */
+static bool smc_pnet_find_ib_apply(char *ib_name, u8 ib_port, char *pnet_name)
+{
+	struct smc_ib_device *ibdev;
+	bool rc = true;
+
+	mutex_lock(&smc_ib_devices.mutex);
+	ibdev = __smc_pnet_find_ib(ib_name);
+	if (ibdev) {
+		if (!__smc_pnet_apply_ib(ibdev, ib_port, pnet_name))
+			rc = false;
+		else
+			pr_warn_ratelimited("smc: ib device %s ibport %d "
+					    "applied user defined pnetid "
+					    "%.16s\n", ibdev->ibdev->name,
+					    ib_port,
+					    ibdev->pnetid[ib_port - 1]);
+	}
 	mutex_unlock(&smc_ib_devices.mutex);
-	return ibdev;
+	return rc;
 }
 
 /* Find an smcd device by a given name. The device might not exist. */
@@ -412,23 +437,13 @@ static int smc_pnet_add_ib(struct smc_pnettable *pnettable, char *ib_name,
 			   u8 ib_port, char *pnet_name)
 {
 	struct smc_pnetentry *tmp_pe, *new_pe;
-	struct smc_ib_device *ib_dev;
 	bool smcddev_applied = true;
-	bool ibdev_applied = true;
+	bool ibdev_applied;
 	struct smcd_dev *smcd;
 	bool new_ibdev;
 
 	/* try to apply the pnetid to active devices */
-	ib_dev = smc_pnet_find_ib(ib_name);
-	if (ib_dev) {
-		ibdev_applied = smc_pnet_apply_ib(ib_dev, ib_port, pnet_name);
-		if (ibdev_applied)
-			pr_warn_ratelimited("smc: ib device %s ibport %d "
-					    "applied user defined pnetid "
-					    "%.16s\n", ib_dev->ibdev->name,
-					    ib_port,
-					    ib_dev->pnetid[ib_port - 1]);
-	}
+	ibdev_applied = smc_pnet_find_ib_apply(ib_name, ib_port, pnet_name);
 	smcd = smc_pnet_find_smcd(ib_name);
 	if (smcd) {
 		smcddev_applied = smc_pnet_apply_smcd(smcd, pnet_name);
-- 
2.34.1

[PATCH net 2/2] net/smc: fix potential UAF in smc_pnet_add_ib for smcd device

[Li Xiasong]

smc_pnet_find_smcd() returns an smcd device pointer and releases the
lock, then smc_pnet_apply_smcd() is called to use this pointer. The
device could be removed between these two calls, leading to a potential
use-after-free when accessing the freed smcd pointer.

    CPU 0                           CPU 1
    ----                            ----
    smc_pnet_add_ib()
      smcd = smc_pnet_find_smcd()
        mutex_lock(&smcd_dev_list.mutex)
        list_for_each_entry() ...
        mutex_unlock(&smcd_dev_list.mutex)
                                    smcd_unregister_dev()
                                      mutex_lock(&smcd_dev_list.mutex)
                                      list_del_init(&smcd->list)
                                      mutex_unlock(&smcd_dev_list.mutex)
                                      kfree(smcd)
      smc_pnet_apply_smcd(smcd)
        smcd->pnetid                   <- UAF (smcd already freed)

Fix this by introducing smc_pnet_find_smcd_apply() which performs both
find and apply under the same lock, preventing the device from being
removed in between.

Also refactor smc_pnet_apply_smcd() into __smc_pnet_apply_smcd()
(without lock) and smc_pnet_apply_smcd() (with lock) for reuse.

Fixes: 69baaac9361e ("dibs: Define dibs_client_ops and dibs_dev_ops")
Signed-off-by: Li Xiasong <lixiasong1@huawei.com>
---
 net/smc/smc_pnet.c | 58 ++++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 22 deletions(-)

diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
index 91c0b1c473b2..a1603740167b 100644
--- a/net/smc/smc_pnet.c
+++ b/net/smc/smc_pnet.c
@@ -270,16 +270,22 @@ static bool smc_pnet_apply_ib(struct smc_ib_device *ib_dev, u8 ib_port,
 
 /* Apply pnetid to smcd device when no pnetid is set.
  */
-static bool smc_pnet_apply_smcd(struct smcd_dev *smcd_dev, char *pnet_name)
+static bool __smc_pnet_apply_smcd(struct smcd_dev *smcd_dev, char *pnet_name)
 {
-	bool applied = false;
-
-	mutex_lock(&smcd_dev_list.mutex);
 	if (!smc_pnet_is_pnetid_set(smcd_dev->pnetid)) {
 		memcpy(smcd_dev->pnetid, pnet_name, SMC_MAX_PNETID_LEN);
 		smcd_dev->pnetid_by_user = true;
-		applied = true;
+		return true;
 	}
+	return false;
+}
+
+static bool smc_pnet_apply_smcd(struct smcd_dev *smcd_dev, char *pnet_name)
+{
+	bool applied;
+
+	mutex_lock(&smcd_dev_list.mutex);
+	applied = __smc_pnet_apply_smcd(smcd_dev, pnet_name);
 	mutex_unlock(&smcd_dev_list.mutex);
 	return applied;
 }
@@ -351,23 +357,39 @@ static bool smc_pnet_find_ib_apply(char *ib_name, u8 ib_port, char *pnet_name)
 }
 
 /* Find an smcd device by a given name. The device might not exist. */
-static struct smcd_dev *smc_pnet_find_smcd(char *smcd_name)
+static struct smcd_dev *__smc_pnet_find_smcd(char *smcd_name)
 {
 	struct smcd_dev *smcd_dev;
 
-	mutex_lock(&smcd_dev_list.mutex);
 	list_for_each_entry(smcd_dev, &smcd_dev_list.list, list) {
 		if (!strncmp(dev_name(&smcd_dev->dibs->dev), smcd_name,
 			     IB_DEVICE_NAME_MAX - 1) ||
 		    (smcd_dev->dibs->dev.parent &&
 		     !strncmp(dev_name(smcd_dev->dibs->dev.parent), smcd_name,
 			      IB_DEVICE_NAME_MAX - 1)))
-			goto out;
+			return smcd_dev;
+	}
+	return NULL;
+}
+
+/* Find an smcd device by name and apply pnetid under lock. */
+static bool smc_pnet_find_smcd_apply(char *smcd_name, char *pnet_name)
+{
+	struct smcd_dev *smcd_dev;
+	bool rc = true;
+
+	mutex_lock(&smcd_dev_list.mutex);
+	smcd_dev = __smc_pnet_find_smcd(smcd_name);
+	if (smcd_dev) {
+		if (!__smc_pnet_apply_smcd(smcd_dev, pnet_name))
+			rc = false;
+		else
+			pr_warn_ratelimited("smc: smcd device %s applied user defined pnetid %.16s\n",
+					    dev_name(&smcd_dev->dibs->dev),
+					    smcd_dev->pnetid);
 	}
-	smcd_dev = NULL;
-out:
 	mutex_unlock(&smcd_dev_list.mutex);
-	return smcd_dev;
+	return rc;
 }
 
 static int smc_pnet_add_eth(struct smc_pnettable *pnettable, struct net *net,
@@ -437,22 +459,14 @@ static int smc_pnet_add_ib(struct smc_pnettable *pnettable, char *ib_name,
 			   u8 ib_port, char *pnet_name)
 {
 	struct smc_pnetentry *tmp_pe, *new_pe;
-	bool smcddev_applied = true;
+	bool smcddev_applied;
 	bool ibdev_applied;
-	struct smcd_dev *smcd;
 	bool new_ibdev;
 
 	/* try to apply the pnetid to active devices */
 	ibdev_applied = smc_pnet_find_ib_apply(ib_name, ib_port, pnet_name);
-	smcd = smc_pnet_find_smcd(ib_name);
-	if (smcd) {
-		smcddev_applied = smc_pnet_apply_smcd(smcd, pnet_name);
-		if (smcddev_applied) {
-			pr_warn_ratelimited("smc: smcd device %s applied user defined pnetid %.16s\n",
-					    dev_name(&smcd->dibs->dev),
-					    smcd->pnetid);
-		}
-	}
+	smcddev_applied = smc_pnet_find_smcd_apply(ib_name, pnet_name);
+
 	/* Apply fails when a device has a hardware-defined pnetid set, do not
 	 * add a pnet table entry in that case.
 	 */
-- 
2.34.1

Re: [PATCH net 1/2] net/smc: fix potential UAF in smc_pnet_add_ib for ib device

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
No issues found.
-- 
pw-bot: cr

Re: [PATCH net 2/2] net/smc: fix potential UAF in smc_pnet_add_ib for smcd device

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/smc: fix potential UAF in smc_pnet_add_ib for smcd device

This commit fixes a potential use-after-free when accessing an smcd device
in smc_pnet_add_ib(). It resolves the issue by combining the lookup and
apply operations into a single function that holds the smcd_dev_list.mutex
for the duration of both operations.

> diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
> index 91c0b1c473b2..a1603740167b 100644
> --- a/net/smc/smc_pnet.c
> +++ b/net/smc/smc_pnet.c

[ ... ]

> @@ -351,23 +357,39 @@ static bool smc_pnet_find_ib_apply(char *ib_name, u8 ib_port, char *pnet_name)
>  }
>  
>  /* Find an smcd device by a given name. The device might not exist. */
> -static struct smcd_dev *smc_pnet_find_smcd(char *smcd_name)
> +static struct smcd_dev *__smc_pnet_find_smcd(char *smcd_name)
>  {
>  	struct smcd_dev *smcd_dev;
>  
> -	mutex_lock(&smcd_dev_list.mutex);
>  	list_for_each_entry(smcd_dev, &smcd_dev_list.list, list) {
>  		if (!strncmp(dev_name(&smcd_dev->dibs->dev), smcd_name,
>  			     IB_DEVICE_NAME_MAX - 1) ||
>  		    (smcd_dev->dibs->dev.parent &&
>  		     !strncmp(dev_name(smcd_dev->dibs->dev.parent), smcd_name,
>  			      IB_DEVICE_NAME_MAX - 1)))
> -			goto out;
> +			return smcd_dev;
> +	}
> +	return NULL;
> +}
> +
> +/* Find an smcd device by name and apply pnetid under lock. */
> +static bool smc_pnet_find_smcd_apply(char *smcd_name, char *pnet_name)
> +{
> +	struct smcd_dev *smcd_dev;
> +	bool rc = true;
> +
> +	mutex_lock(&smcd_dev_list.mutex);
> +	smcd_dev = __smc_pnet_find_smcd(smcd_name);
> +	if (smcd_dev) {
> +		if (!__smc_pnet_apply_smcd(smcd_dev, pnet_name))

While this prevents the use-after-free in smc_pnet_add_ib() by expanding the
mutex coverage, does the same vulnerability pattern still exist in the
connection handshake paths?

Neither struct smcd_dev nor struct smc_ib_device appear to use reference
counting. During a connection handshake, functions like
smc_pnet_find_ism_resource() and _smc_pnet_find_roce_by_pnetid() find a
device under the mutex, store the raw pointer (e.g., into ini->ism_dev[0]),
and immediately release the mutex.

If a concurrent hot-unplug event triggers smcd_unregister_dev() or
smc_ib_remove_dev() while the handshake process is accessing the device
without holding a lock or reference, could it lead to another use-after-free?

Would a more complete fix require introducing reference counting for both
device structures to ensure they remain valid while pointed to by the
handshake structures?

> +			rc = false;
> +		else
> +			pr_warn_ratelimited("smc: smcd device %s applied user defined pnetid %.16s\n",
> +					    dev_name(&smcd_dev->dibs->dev),
> +					    smcd_dev->pnetid);
>  	}
> -	smcd_dev = NULL;
> -out:
>  	mutex_unlock(&smcd_dev_list.mutex);
> -	return smcd_dev;
> +	return rc;
>  }