linux-kernel - Re: [PATCH v13 35/53] ALSA: usb-audio: Prevent starting of audio stream if in use

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: <0cb39613-ec01-50aa-807f-b537f201dac0@quicinc.com>
Date: Wed, 7 Feb 2024 17:12:00 -0800
From: Wesley Cheng <quic_wcheng@...cinc.com>
To: Takashi Iwai <tiwai@...e.de>
CC: <srinivas.kandagatla@...aro.org>, <mathias.nyman@...el.com>,
        <perex@...ex.cz>, <conor+dt@...nel.org>, <corbet@....net>,
        <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>,
        <konrad.dybcio@...aro.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-sound@...r.kernel.org>,
        <linux-usb@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-doc@...r.kernel.org>, <alsa-devel@...a-project.org>
Subject: Re: [PATCH v13 35/53] ALSA: usb-audio: Prevent starting of audio
 stream if in use

Hi Takashi,

On 2/7/2024 4:02 PM, Wesley Cheng wrote:
> Hi Takashi,
> 
> On 2/6/2024 11:05 PM, Takashi Iwai wrote:
>> On Wed, 07 Feb 2024 01:08:00 +0100,
>> Wesley Cheng wrote:
>>>
>>> Hi Takashi,
>>>
>>> On 2/6/2024 5:07 AM, Takashi Iwai wrote:
>>>> On Sat, 03 Feb 2024 03:36:27 +0100,
>>>> Wesley Cheng wrote:
>>>>>
>>>>> With USB audio offloading, an audio session is started from the ASoC
>>>>> platform sound card and PCM devices.  Likewise, the USB SND path is 
>>>>> still
>>>>> readily available for use, in case the non-offload path is 
>>>>> desired.  In
>>>>> order to prevent the two entities from attempting to use the USB bus,
>>>>> introduce a flag that determines when either paths are in use.
>>>>>
>>>>> If a PCM device is already in use, the check will return an error to
>>>>> userspace notifying that the stream is currently busy.  This 
>>>>> ensures that
>>>>> only one path is using the USB substream.
>>>>>
>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
>>>>
>>>> Hm, I'm not sure whether it's safe to hold chip->mutex there for the
>>>> long code path.  It even kicks off the auto-resume, which may call
>>>> various functions at resuming, and some of them may re-hold
>>>> chip->mutex.
>>>>
>>>
>>> That's a good point.
>>>
>>>> If it's only about the open flag, protect only the flag access with
>>>> the mutex, not covering the all open function.  At least the re-entry
>>>> can be avoided by that.
>>>>
>>>
>>> Sure, let me re-order the check/assignment and the mutex locking.
>>> Since this is now checked here in USB PCM and the QC offload driver,
>>> we want to make sure that if there was some application attempting to
>>> open both at the same time, we prevent any possible races.
>>>
>>> I think the best way to address this would be something like:
>>>
>>> static int snd_usb_pcm_open(struct snd_pcm_substream *substream)
>>> {
>>> ...
>>>     mutex_lock(&chip->mutex);
>>>     if (subs->opened) {
>>>         mutex_unlock(&chip->mutex);
>>>         return -EBUSY;
>>>     }
>>>     subs->opened = 1;
>>>     mutex_unlock(&chip->mutex);
>>>
>>> //Execute bulk of PCM open routine
>>> ...
>>>     return 0;
>>>
>>> // If any errors are seen, unwind
>>> err_resume:
>>>     snd_usb_autosuspend(subs->stream->chip);
>>> err_open:
>>>     mutex_lock(&chip->mutex);
>>>     subs->opened = 0;
>>>     mutex_unlock(&chip->mutex);
>>>
>>>     return ret;
>>> }
>>>
>>> Set the opened flag first, so that if QC offload checks it, it can
>>> exit early and vice versa.  Otherwise, if we set the opened flag at
>>> the same position as the previous patch, we may be calling the other
>>> routines in parallel to the QC offload enable stream routine.  The
>>> only thing with this patch is that we'd need some error handling
>>> unwinding.
>>
>> The above is what I had in mind.
>>
>> But, thinking on this again, you might be able to get the same result
>> by using the ALSA PCM core substream open_mutex and hw_opened flag.
>> This is already held and set at snd_pcm_core() (the hw_opened flag is
>> set after open callback, though).  The offload driver can use those
>> instead of the own lock and flag, too, although it's not really
>> well-mannered behavior (hence you need proper comments).
>>
> 
> I think I had looked into this as well previously, and it was difficult 
> to achieve, because from the USB offloading perspective, we don't ever 
> call: snd_usb_pcm_open()
> 
> This is actually where we populate the pcm_substream parameter within 
> struct snd_usb_substream based on when userspace opens the USB SND PCM 
> device (which is not the case for offloading).  So the offload driver 
> doesn't have a way to fetch the struct snd_pcm that is allocated to the 
> PCM device created by the USB SND card.
> 

Sorry, took a look at it again, and found a way.  Although not pretty, 
we can access it using:
subs->stream->pcm->streams[direction].substream->hw_opened

Thanks
Wesley Cheng