[PATCH v1 RESEND] slimbus: stream: Add null pointer check for client functions
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);
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);
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);
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
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
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
participants (3)
-
Bjorn Andersson -
Srinivas Kandagatla -
Viken Dadhaniya