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;
>  }