Re: [PATCH 4/8] rpmsg: qcom_glink_ssr: Suppress cleanup notify between cluster siblings

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Type confusion and out-of-bounds read when `qcom_glink_ept_cluster()` downcasts a generic `rpmsg_endpoint` without checking its transport backend.
--

--- Patch [4]: [PATCH 4/8] rpmsg: qcom_glink_ssr: Suppress cleanup notify between cluster siblings ---
commit c10027dd4bbd2c7fac42077955228324a4275d43
Author: Shawn Guo <[email protected]>

rpmsg: qcom_glink_ssr: Suppress cleanup notify between cluster siblings

This commit passes the remote processor's cluster identifier down to the
GLINK edges and uses it to skip sending cleanup broadcast messages to other
members of the same cluster during start/stop events.

> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index d9d4468e4cbdf..a15d7e6a52423 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
[ ... ]
> @@ -1940,6 +1944,20 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(qcom_glink_native_probe);
>  
> +/**
> + * qcom_glink_ept_cluster() - cluster identifier of an endpoint's edge
> + * @ept:	endpoint to query
> + *
> + * Return: the opaque cluster identifier of the remote processor @ept talks to,
> + * or NULL if it is not part of a cluster.
> + */
> +void *qcom_glink_ept_cluster(struct rpmsg_endpoint *ept)
> +{
> +	struct glink_channel *channel = to_glink_channel(ept);
> +
> +	return channel->glink->cluster;
> +}

[Severity: High]
Does this downcast assume that the endpoint is always backed by the GLINK
transport without verifying it?

If a remote processor on a different bus (such as virtio_rpmsg_bus) sends a
Name Service announcement for a channel named glink_ssr, the rpmsg core
could probe the qcom_glink_ssr_driver and store a non-GLINK endpoint.

Later, when a remoteproc stops, qcom_glink_ssr_notifier_call() would pass
this non-GLINK endpoint to qcom_glink_ept_cluster():

qcom_glink_ssr_notifier_call()
  cluster = qcom_glink_ept_cluster(ssr->ept);

Could this blind cast to struct glink_channel and subsequent dereference
of channel->glink->cluster cause a kernel panic or out-of-bounds read?

> diff --git a/drivers/rpmsg/qcom_glink_ssr.c b/drivers/rpmsg/qcom_glink_ssr.c
> index e71d3716c55c5..74c671a19af03 100644
> --- a/drivers/rpmsg/qcom_glink_ssr.c
> +++ b/drivers/rpmsg/qcom_glink_ssr.c
[ ... ]
> @@ -100,18 +120,24 @@ static int qcom_glink_ssr_notifier_call(struct notifier_block *nb,
>  					void *data)
>  {
>  	struct glink_ssr *ssr = container_of(nb, struct glink_ssr, nb);
> +	struct glink_ssr_notify_data *notify_data = data;
>  	struct do_cleanup_msg msg;
> -	char *ssr_name = data;
> +	void *cluster;
>  	int ret;
>  
> +	/* Cluster siblings' firmware can't handle peer SSR notify; skip it */
> +	cluster = qcom_glink_ept_cluster(ssr->ept);
> +	if (cluster && cluster == notify_data->cluster)
> +		return NOTIFY_DONE;
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4