linux-kernel - Re: [RFC PATCH v2 12/22] sound: usb: card: Introduce USB SND platform op callbacks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <63ef99b7-c6c4-497d-da08-a3966291de9f@quicinc.com>
Date:   Mon, 30 Jan 2023 15:00:32 -0800
From:   Wesley Cheng <quic_wcheng@...cinc.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        <srinivas.kandagatla@...aro.org>, <mathias.nyman@...el.com>,
        <perex@...ex.cz>, <lgirdwood@...il.com>, <andersson@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <gregkh@...uxfoundation.org>,
        <Thinh.Nguyen@...opsys.com>, <broonie@...nel.org>,
        <bgoswami@...cinc.com>, <tiwai@...e.com>, <robh+dt@...nel.org>,
        <agross@...nel.org>
CC:     <devicetree@...r.kernel.org>, <alsa-devel@...a-project.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_jackp@...cinc.com>,
        <quic_plai@...cinc.com>
Subject: Re: [RFC PATCH v2 12/22] sound: usb: card: Introduce USB SND platform
 op callbacks

Hi Pierre,

On 1/26/2023 7:50 AM, Pierre-Louis Bossart wrote:
> 
> 
> 
>> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops)
>> +{
>> +	if (platform_ops)
>> +		return -EEXIST;
>> +
>> +	platform_ops = ops;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops);
>> +
>> +int snd_usb_unregister_platform_ops(void)
>> +{
>> +	platform_ops = NULL;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops);
> 
> I find this super-racy.
> 
> If the this function is called just before ...
> 
>>   
>>   /*
>>    * disconnect streams
>> @@ -910,6 +928,10 @@ static int usb_audio_probe(struct usb_interface *intf,
>>   	usb_set_intfdata(intf, chip);
>>   	atomic_dec(&chip->active);
>>   	mutex_unlock(&register_mutex);
>> +
>> +	if (platform_ops->connect_cb)
>> +		platform_ops->connect_cb(intf, chip);
>> +
> 
> ... this, then you have a risk of using a dandling pointer.
> 
> You also didn't test that the platform_ops != NULL, so there's a risk of
> dereferencing a NULL pointer.
> 
> Not so good, eh?
> 
> It's a classic (I've had the same sort of issues with SoundWire), when
> you export ops from one driver than can be removed, then additional
> protection is needed when using those callbacks.
> 
> 

Yep, will take a look at this a bit more to improve it.

Thanks
Wesley Cheng