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