linux-kernel - Re: [PATCH] ALSA: firewire-tascam: add missing unwind goto in snd_tscm_stream_start

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: <20230406140529.GA159563@workstation>
Date:   Thu, 6 Apr 2023 23:05:29 +0900
From:   Takashi Sakamoto <o-takashi@...amocchi.jp>
To:     Xu Biang <xubiang@...t.edu.cn>
Cc:     Clemens Ladisch <clemens@...isch.de>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, dzm91@...t.edu.cn,
        error27@...il.com, hust-os-kernel-patches@...glegroups.com,
        Takashi Iwai <tiwai@...e.de>, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ALSA: firewire-tascam: add missing unwind goto in
 snd_tscm_stream_start_duplex()

Hi,

On Thu, Apr 06, 2023 at 06:28:01AM -0700, Xu Biang wrote:
> Smatch Warns:
> sound/firewire/tascam/tascam-stream.c:493 snd_tscm_stream_start_duplex()
> warn: missing unwind goto?
> 
> The direct return will cause the stream list of "&tscm->domain" unemptied
> and the session in "tscm" unfinished if amdtp_domain_start() returns with
> an error.
> 
> Fix this by changing the direct return to a goto which will empty the
> stream list of "&tscm->domain" and finish the session in "tscm".
> 
> The snd_tscm_stream_start_duplex() function is called in the prepare
> callback of PCM. According to "ALSA Kernel API Documentation", the prepare
> callback of PCM will be called many times at each setup. So, if the
> "&d->streams" list is not emptied, when the prepare callback is called
> next time, snd_tscm_stream_start_duplex() will receive -EBUSY from
> amdtp_domain_add_stream() that tries to add an existing stream to the
> domain. The error handling code after the "error" label will be executed
> in this case, and the "&d->streams" list will be emptied. So not emptying
> the "&d->streams" list will not cause an issue. But it is more efficient
> and readable to empty it on the first error by changing the direct return
> to a goto statement.
> 
> The session in "tscm" has been begun before amdtp_domain_start(), so it
> needs to be finished when amdtp_domain_start() fails.
> 
> Fixes: c281d46a51e3 ("ALSA: firewire-tascam: support AMDTP domain")
> Signed-off-by: Xu Biang <xubiang@...t.edu.cn>
> Reviewed-by: Dan Carpenter <error27@...il.com>
> ---
> Note that this finding is from static analysis and not tested.
> 
>  sound/firewire/tascam/tascam-stream.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Indeed. I overlooked it when posting the patch. The bug exists Linux
kernel v5.4 or later and the fix should be forward to stable kernels.

Acked-by: Takashi Sakamoto <o-takashi@...amocchi.jp>

> diff --git a/sound/firewire/tascam/tascam-stream.c b/sound/firewire/tascam/tascam-stream.c
> index 53e094cc411f..dfe783d01d7d 100644
> --- a/sound/firewire/tascam/tascam-stream.c
> +++ b/sound/firewire/tascam/tascam-stream.c
> @@ -490,7 +490,7 @@ int snd_tscm_stream_start_duplex(struct snd_tscm *tscm, unsigned int rate)
>  		// packet is important for media clock recovery.
>  		err = amdtp_domain_start(&tscm->domain, tx_init_skip_cycles, true, true);
>  		if (err < 0)
> -			return err;
> +			goto error;
>  
>  		if (!amdtp_domain_wait_ready(&tscm->domain, READY_TIMEOUT_MS)) {
>  			err = -ETIMEDOUT;
> -- 
> 2.17.1


Thanks

Takashi Sakamoto