Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3652-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 09:10:06 +0200
From: Johan Hovold <johan@kernel.org>
To: quic_zijuhu <quic_zijuhu@quicinc.com>
Cc: luiz.dentz@gmail.com, marcel@holtmann.org,
	linux-bluetooth@vger.kernel.org, johan+linaro@kernel.org
Subject: Re: [PATCH] Bluetooth: qca: Fix nullptr dereference for non-serdev
 devices
Message-ID: <Zh91zq13nZvH3-Yj@hovoldconsulting.com>
References: <1713325792-17181-1-git-send-email-quic_zijuhu@quicinc.com>
 <Zh9saGbOl-fXP6SD@hovoldconsulting.com>
 <5e4e62c8-d17f-45fd-9212-088eb026dc7b@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5e4e62c8-d17f-45fd-9212-088eb026dc7b@quicinc.com>

[ Please wrap you mails at 72 columns or so and trim unnecessary context
  when replying. ]

On Wed, Apr 17, 2024 at 02:51:38PM +0800, quic_zijuhu wrote:
> On 4/17/2024 2:30 PM, Johan Hovold wrote:
> > On Wed, Apr 17, 2024 at 11:49:52AM +0800, Zijun Hu wrote:
> >> hu->serdev is nullptr and will cause nullptr dereference if qca_setup()
> >> is called by non-serdev device, fixed by null check before access.
> > 
> > No, this patch is not correct.

> i don't think so, nullptr checking for hu->serdev has been performed
> within qca_setup() everywhere when need to access serdev related
> members since this function will be called by both serdev and
> none-serdev. so suggest add such checking.

Your patch is not correct since you claim that this path can trigger a
NULL pointer dereference. As I point out below that is currently not
possible.

If you need this for some future change you need to say so in the commit
message and drop the bogus Fixes tag.

> >> Fixes: 77f45cca8bc5 ("Bluetooth: qca: fix device-address endianness")
> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

> >> @@ -1905,10 +1905,11 @@ static int qca_setup(struct hci_uart *hu)
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> -		qcadev = serdev_device_get_drvdata(hu->serdev);
> > 
> > Non-serdev controllers have type QCA_ROME (see qca_soc_type()) so will
> > never end up in this path.

> i have submitted below patches to add supports for all other
> non-serdev controllers.

> https://patchwork.kernel.org/project/bluetooth/list/?series=844120

Ok, you need it for some future changes, but I'm afraid that adding new
random vendor specific ioctls like you do in that is series is a no-go.

Why are you trying to revive the old line-discipline when we have
serdev?

In any case, a change like this one would should be included in that
series so that it's clear that it is only needed for your proposed
further changes.
 
> > I verified this when I wrote the patch and also fixed up a couple of
> > real non-serdev bugs here:
> > 
> > 	https://lore.kernel.org/lkml/20240319154611.2492-1-johan+linaro@kernel.org/

> actually, i have submitted below fix for this issue earlier.
> https://lore.kernel.org/all/1704960978-5437-1-git-send-email-quic_zijuhu@quicinc.com/

Ok.

Johan