Received-SPF: pass (google.com: domain of linux-kernel+bounces-141242-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 11 Apr 2024 08:56:12 -0700
From: Bjorn Andersson <quic_bjorande@quicinc.com>
To: Viken Dadhaniya <quic_vdadhani@quicinc.com>
CC: <andersson@kernel.org>, <konrad.dybcio@linaro.org>,
        <srinivas.kandagatla@linaro.org>, <linux-arm-msm@vger.kernel.org>,
        <alsa-devel@alsa-project.org>, <linux-kernel@vger.kernel.org>,
        <quic_msavaliy@quicinc.com>, <quic_vtanuku@quicinc.com>,
        <quic_anupkulk@quicinc.com>, <quic_cchiluve@quicinc.com>
Subject: Re: [PATCH v1 RESEND] slimbus: stream: Add null pointer check for
 client functions
Message-ID: <ZhgIHHP1FUeCr+vx@hu-bjorande-lv.qualcomm.com>
References: <20240327083214.29443-1-quic_vdadhani@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240327083214.29443-1-quic_vdadhani@quicinc.com>

On Wed, Mar 27, 2024 at 02:02:14PM +0530, Viken Dadhaniya wrote:
> There is a possible scenario where client driver is calling

How can we asses the validity or the risk of this problem?
How would I know if this matches e.g. a bug report reported by a user?

Describe the problem such that the reviewer can asses the validity and
severity of your bug fixes.

> slimbus stream APIs in incorrect sequence and it might lead to
> invalid null access of the stream pointer in slimbus
> enable/disable/prepare/unprepare/free function.
> 
> Fix this by checking validity of the stream before accessing in
> all function API’s exposed to client.
> 

You use the work "fix" a few time, are you fixing an actual bug? Are you
just guarding the driver from incorrect usage?

If it's a fix, then add Fixes and Cc: stable here.

> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
> ---
>  drivers/slimbus/stream.c | 37 +++++++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/slimbus/stream.c b/drivers/slimbus/stream.c
> index 1d6b38657917..c5a436fd0952 100644
> --- a/drivers/slimbus/stream.c
> +++ b/drivers/slimbus/stream.c
> @@ -202,10 +202,16 @@ static int slim_get_prate_code(int rate)
>  int slim_stream_prepare(struct slim_stream_runtime *rt,
>  			struct slim_stream_config *cfg)
>  {
> -	struct slim_controller *ctrl = rt->dev->ctrl;
> +	struct slim_controller *ctrl;
>  	struct slim_port *port;
>  	int num_ports, i, port_id, prrate;
>  
> +	if (!rt || !cfg) {
> +		pr_err("%s: Stream or cfg is NULL, Check from client side\n", __func__);

Use dev_err() and write your error messages such that they make sense
without the use of __func__.

> +		return -EINVAL;

Is this expected to happen during normal operation, or is this a sign of
a bug?


Neither of the two callers of this function checks the return value, so
is this really going to result in a good system state?


It would make sense to require the client to pass valid rt and cfg
pointers, and if you have an issue in the client driver in which we
might end up with invalid points, then those drivers should be fixed -
rather than relying on chance and swipe it under the rug here.

Regards,
Bjorn

> +	}
> +
> +	ctrl = rt->dev->ctrl;
>  	if (rt->ports) {
>  		dev_err(&rt->dev->dev, "Stream already Prepared\n");
>  		return -EINVAL;
> @@ -358,9 +364,15 @@ int slim_stream_enable(struct slim_stream_runtime *stream)
>  {
>  	DEFINE_SLIM_BCAST_TXN(txn, SLIM_MSG_MC_BEGIN_RECONFIGURATION,
>  				3, SLIM_LA_MANAGER, NULL);
> -	struct slim_controller *ctrl = stream->dev->ctrl;
> +	struct slim_controller *ctrl;
>  	int ret, i;
>  
> +	if (!stream) {
> +		pr_err("%s: Stream is NULL, Check from client side\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	ctrl = stream->dev->ctrl;
>  	if (ctrl->enable_stream) {
>  		ret = ctrl->enable_stream(stream);
>  		if (ret)
> @@ -411,12 +423,18 @@ int slim_stream_disable(struct slim_stream_runtime *stream)
>  {
>  	DEFINE_SLIM_BCAST_TXN(txn, SLIM_MSG_MC_BEGIN_RECONFIGURATION,
>  				3, SLIM_LA_MANAGER, NULL);
> -	struct slim_controller *ctrl = stream->dev->ctrl;
> +	struct slim_controller *ctrl;
>  	int ret, i;
>  
> +	if (!stream) {
> +		pr_err("%s: Stream is NULL, Check from client side\n", __func__);
> +		return -EINVAL;
> +	}
> +
>  	if (!stream->ports || !stream->num_ports)
>  		return -EINVAL;
>  
> +	ctrl = stream->dev->ctrl;
>  	if (ctrl->disable_stream)
>  		ctrl->disable_stream(stream);
>  
> @@ -448,6 +466,11 @@ int slim_stream_unprepare(struct slim_stream_runtime *stream)
>  {
>  	int i;
>  
> +	if (!stream) {
> +		pr_err("%s: Stream is NULL, Check from client side\n", __func__);
> +		return -EINVAL;
> +	}
> +
>  	if (!stream->ports || !stream->num_ports)
>  		return -EINVAL;
>  
> @@ -476,8 +499,14 @@ EXPORT_SYMBOL_GPL(slim_stream_unprepare);
>   */
>  int slim_stream_free(struct slim_stream_runtime *stream)
>  {
> -	struct slim_device *sdev = stream->dev;
> +	struct slim_device *sdev;
> +
> +	if (!stream) {
> +		pr_err("%s: Stream is NULL, Check from client side\n", __func__);
> +		return -EINVAL;
> +	}
>  
> +	sdev = stream->dev;
>  	spin_lock(&sdev->stream_list_lock);
>  	list_del(&stream->node);
>  	spin_unlock(&sdev->stream_list_lock);
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
> of Code Aurora Forum, hosted by The Linux Foundation
>