Re: [PATCH] Bluetooth: qca: Fix nullptr dereference for non-serdev devices

[quic_zijuhu <quic_zijuhu@quicinc.com>]

On 4/17/2024 3:10 PM, Johan Hovold wrote:
> [ 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.
> 
will add this change in to btattach support serials.
>>>> 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.
> 
it is a generic ioctl, for QCA, it is used to specific soc_type. it maybe be used
by other vendor driver to set user specified info.
 
> Why are you trying to revive the old line-discipline when we have
> serdev?
> 

we usually need to use tool btattach which will generate non-serdev devices to attach a BT module to generic PC to
complete various development and verification, customer also have requirements
to use tool btattach as explained by below link
https://patchwork.kernel.org/project/bluetooth/patch/1712939188-25529-5-git-send-email-quic_zijuhu@quicinc.com/

> 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.
>  okay, it has been included in the updated patch serials, will update the serials to include change
in question.
https://patchwork.kernel.org/project/bluetooth/list/?series=844120
>>> 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