[PATCH v1 RESEND] slimbus: stream: Add null pointer check for client functions

[PATCH v1 RESEND] slimbus: stream: Add null pointer check for client functions

[Viken Dadhaniya]

There is a possible scenario where client driver is calling
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.

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__);
+		return -EINVAL;
+	}
+
+	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

Re: [PATCH v1 RESEND] slimbus: stream: Add null pointer check for client functions

[Srinivas Kandagatla]

Thanks Viken for the patch,

On 27/03/2024 08:32, Viken Dadhaniya wrote:
> There is a possible scenario where client driver is calling
> 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.
> 
> 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__);

Please use dev_err where possible


--srini
> +		return -EINVAL;
> +	}
> +
> +	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);

Re: [PATCH v1 RESEND] slimbus: stream: Add null pointer check for client functions

[Viken Dadhaniya]

On 4/11/2024 3:19 PM, Srinivas Kandagatla wrote:
> Thanks Viken for the patch,
> 
> On 27/03/2024 08:32, Viken Dadhaniya wrote:
>> There is a possible scenario where client driver is calling
>> 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.
>>
>> 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__);
> 
> Please use dev_err where possible
> 
> 
> --srini


For error scenario, we don't have valid dev to be used in dev_err 
argument. this log will help for debug.
Please let us know if any concern with pr_err.


>> +        return -EINVAL;
>> +    }
>> +
>> +    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);

Re: [PATCH v1 RESEND] slimbus: stream: Add null pointer check for client functions

[Bjorn Andersson]

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
> 

Re: [PATCH v1 RESEND] slimbus: stream: Add null pointer check for client functions

[Viken Dadhaniya]

On 4/11/2024 9:26 PM, Bjorn Andersson wrote:
> 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.

Ok. Updated commit log in v2

> 
>> 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.

Let me correct myself there. Not a fix but consider an improvement where 
preventing a crash due to client following the incorrect sequence.

> 
>> 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__.

For error scenario, we don't have valid dev to be used in dev_err argument.
this log will help for debug. Please let us know any concern with pr_err

> 
>> +		return -EINVAL;
> 
> Is this expected to happen during normal operation, or is this a sign of
> a bug?
> 

It's a scenario where client doesn't follow the proper sequence and 
slimbus driver can crash if not checked against NULL.

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

we expect client to check return value of framework APIs.

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

Agree. it is sequence mismatch from client driver, and they should take 
care. it is leading to NULL pointer access in slimbus APIs, so prevent 
crash by adding check.

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

Re: [PATCH v1 RESEND] slimbus: stream: Add null pointer check for client functions

[Bjorn Andersson]

On Wed, Apr 17, 2024 at 03:12:38PM +0530, Viken Dadhaniya wrote:
> 
> 
> On 4/11/2024 9:26 PM, Bjorn Andersson wrote:
> > 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.
> 
> Ok. Updated commit log in v2
> 
> > 
> > > 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.
> 
> Let me correct myself there. Not a fix but consider an improvement where
> preventing a crash due to client following the incorrect sequence.
> 

This is C, this is the Linux kernel. We do not account for clients
calling random functions in random order.

If it happens because there are race conditions, then fix the client
driver (there's probably other bugs hidden there). If it's a problem
that can happen during bringup due to some misconfiguration, let it go
to kernel panic so we can catch it quickly.

If there is a valid scenario where this can happen, then clearly
describe this in the commit message.

> > 
> > > 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__.
> 
> For error scenario, we don't have valid dev to be used in dev_err argument.
> this log will help for debug. Please let us know any concern with pr_err
> 

Yes, I have a concern with this.

You will print a line in the log and carry on as if nothing happened.
Most likely this will go unnoticed during testing, or you will have bug
reports that are extremely hard to take action on.

> > 
> > > +		return -EINVAL;
> > 
> > Is this expected to happen during normal operation, or is this a sign of
> > a bug?
> > 
> 
> It's a scenario where client doesn't follow the proper sequence and slimbus
> driver can crash if not checked against NULL.
> 
> > 
> > Neither of the two callers of this function checks the return value, so
> > is this really going to result in a good system state?
> > 
> 
> we expect client to check return value of framework APIs.
> 

Please send bug fixes for these.

> > 
> > 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
> > 
> 
> Agree. it is sequence mismatch from client driver, and they should take
> care. it is leading to NULL pointer access in slimbus APIs, so prevent crash
> by adding check.
> 

You're not just preventing a crash, you're introducing a unexpected
(currently undefined, due to lack of error propagation) behavior by just
returning an error here.

Regards,
Bjorn