linux-kernel - Re: [PATCH v26 26/33] 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: <f71a248b-6ff7-472d-9d29-d5a0a9dd5b8e@quicinc.com>
Date: Tue, 3 Sep 2024 16:21:16 -0700
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>, <conor+dt@...nel.org>, <dmitry.torokhov@...il.com>,
        <corbet@....net>, <broonie@...nel.org>, <lgirdwood@...il.com>,
        <tiwai@...e.com>, <krzk+dt@...nel.org>, <Thinh.Nguyen@...opsys.com>,
        <bgoswami@...cinc.com>, <robh@...nel.org>,
        <gregkh@...uxfoundation.org>
CC: <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-sound@...r.kernel.org>, <linux-input@...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 v26 26/33] ALSA: usb-audio: Prevent starting of audio
 stream if in use

Hi Pierre,

On 8/30/2024 2:45 AM, Pierre-Louis Bossart wrote:
>
> On 8/29/24 21:40, 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>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
>
> I would also move this patch earlier in the series since it has no
> dependency on USB offload really, and if somehow it breaks USB audio
> regular paths we'd want to know early for bisection.

Sure I'll re-order this earlier since I'm going to send out another rev.

Thanks

Wesley Cheng

>
>> ---
>>  sound/usb/card.h |  1 +
>>  sound/usb/pcm.c  | 29 ++++++++++++++++++++++++++---
>>  2 files changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/sound/usb/card.h b/sound/usb/card.h
>> index 15cda1730076..d8b8522e1613 100644
>> --- a/sound/usb/card.h
>> +++ b/sound/usb/card.h
>> @@ -165,6 +165,7 @@ struct snd_usb_substream {
>>  	unsigned int pkt_offset_adj;	/* Bytes to drop from beginning of packets (for non-compliant devices) */
>>  	unsigned int stream_offset_adj;	/* Bytes to drop from beginning of stream (for non-compliant devices) */
>>  
>> +	unsigned int opened:1;		/* pcm device opened */
>>  	unsigned int running: 1;	/* running status */
>>  	unsigned int period_elapsed_pending;	/* delay period handling */
>>  
>> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
>> index 18467da6fd9e..b24ee38fad72 100644
>> --- a/sound/usb/pcm.c
>> +++ b/sound/usb/pcm.c
>> @@ -1241,8 +1241,17 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream)
>>  	struct snd_usb_stream *as = snd_pcm_substream_chip(substream);
>>  	struct snd_pcm_runtime *runtime = substream->runtime;
>>  	struct snd_usb_substream *subs = &as->substream[direction];
>> +	struct snd_usb_audio *chip = subs->stream->chip;
>>  	int ret;
>>  
>> +	mutex_lock(&chip->mutex);
>> +	if (subs->opened) {
>> +		mutex_unlock(&chip->mutex);
>> +		return -EBUSY;
>> +	}
>> +	subs->opened = 1;
>> +	mutex_unlock(&chip->mutex);
>> +
>>  	runtime->hw = snd_usb_hardware;
>>  	/* need an explicit sync to catch applptr update in low-latency mode */
>>  	if (direction == SNDRV_PCM_STREAM_PLAYBACK &&
>> @@ -1259,13 +1268,23 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream)
>>  
>>  	ret = setup_hw_info(runtime, subs);
>>  	if (ret < 0)
>> -		return ret;
>> +		goto err_open;
>>  	ret = snd_usb_autoresume(subs->stream->chip);
>>  	if (ret < 0)
>> -		return ret;
>> +		goto err_open;
>>  	ret = snd_media_stream_init(subs, as->pcm, direction);
>>  	if (ret < 0)
>> -		snd_usb_autosuspend(subs->stream->chip);
>> +		goto err_resume;
>> +
>> +	return 0;
>> +
>> +err_resume:
>> +	snd_usb_autosuspend(subs->stream->chip);
>> +err_open:
>> +	mutex_lock(&chip->mutex);
>> +	subs->opened = 0;
>> +	mutex_unlock(&chip->mutex);
>> +
>>  	return ret;
>>  }
>>  
>> @@ -1274,6 +1293,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream)
>>  	int direction = substream->stream;
>>  	struct snd_usb_stream *as = snd_pcm_substream_chip(substream);
>>  	struct snd_usb_substream *subs = &as->substream[direction];
>> +	struct snd_usb_audio *chip = subs->stream->chip;
>>  	int ret;
>>  
>>  	snd_media_stop_pipeline(subs);
>> @@ -1287,6 +1307,9 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream)
>>  
>>  	subs->pcm_substream = NULL;
>>  	snd_usb_autosuspend(subs->stream->chip);
>> +	mutex_lock(&chip->mutex);
>> +	subs->opened = 0;
>> +	mutex_unlock(&chip->mutex);
>>  
>>  	return 0;
>>  }