Received-SPF: pass (google.com: domain of linux-kernel+bounces-149264-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 17 Apr 2024 14:39:14 -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: <ZiBBgiQYVAEEaeZi@hu-bjorande-lv.qualcomm.com>
References: <20240327083214.29443-1-quic_vdadhani@quicinc.com>
 <ZhgIHHP1FUeCr+vx@hu-bjorande-lv.qualcomm.com>
 <720e1ee0-79b0-4d30-b1b8-a90676057161@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: <720e1ee0-79b0-4d30-b1b8-a90676057161@quicinc.com>

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