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: <ef83036f-6605-1db3-d962-ac28a10711ac@quicinc.com>
Date: Tue, 6 Feb 2024 16:08: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/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.

Thanks
Wesley Cheng