From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4ADF0E92FF9 for ; Fri, 6 Oct 2023 08:36:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231221AbjJFIgC (ORCPT ); Fri, 6 Oct 2023 04:36:02 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54046 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230266AbjJFIf7 (ORCPT ); Fri, 6 Oct 2023 04:35:59 -0400 Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id ADE0E93 for ; Fri, 6 Oct 2023 01:35:58 -0700 (PDT) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 6BDBF1F45F; Fri, 6 Oct 2023 08:35:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1696581357; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ZGO2mlUqE3aPcsibFoQXFlBLId1U3oI6EbfB7CylCGI=; b=wP2UXOhuOPQagS4qtjhTDzOcJuBmqZQagcfwVqH74ZrdSomM771xtKx2HusMgvXX2t8vRU Q9AHM8DQNRsnBvPCfLO1SqNw1qChCJE18GgPl2qRrDiGhgBSIdPbpOsblcmopRWlMfflKq I4NQSQxpUVFk6fNFzmnRcFF8SU296Io= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1696581357; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ZGO2mlUqE3aPcsibFoQXFlBLId1U3oI6EbfB7CylCGI=; b=UhUaC8ONSa4MQOaXEnJZh82SKSYlYnn6Nro96QOl8VO42nDc7URMp0BCzwdElCIibJ+vrL CSLUS6HXWR9G/QCg== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 2575F13A2E; Fri, 6 Oct 2023 08:35:57 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id KvsuCO3GH2W+GQAAMHmgww (envelope-from ); Fri, 06 Oct 2023 08:35:57 +0000 Date: Fri, 06 Oct 2023 10:35:56 +0200 Message-ID: <87zg0ww3kj.wl-tiwai@suse.de> From: Takashi Iwai To: Ma Ke Cc: perex@perex.cz, tiwai@suse.com, mhocko@suse.com, mgorman@techsingularity.net, 42.hyeyoo@gmail.com, surenb@google.com, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] ALSA: pcm: oss: Fix race at SNDCTL_DSP_SETTRIGGER In-Reply-To: <20230921135837.3590897-1-make_ruc2021@163.com> References: <20230921135837.3590897-1-make_ruc2021@163.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0 MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 21 Sep 2023 15:58:37 +0200, Ma Ke wrote: > > There is a small race window at snd_pcm_oss_set_trigger() that is > called from OSS PCM SNDCTL_DSP_SETTRIGGER ioctl; namely the function > calls snd_pcm_oss_make_ready() at first, then takes the params_lock > mutex for the rest. When the stream is set up again by another thread > between them, it leads to inconsistency, and may result in unexpected > results such as NULL dereference of OSS buffer as a fuzzer spotted > recently. > The fix is simply to cover snd_pcm_oss_make_ready() call into the same > params_lock mutex with snd_pcm_oss_make_ready_locked() variant. Sorry for the late response, as I've been (still) off since the last week. The code change itself looks OK, but unlike the change (with almost same changelog) in commit 8423f0b6d513, this won't hit a serious problem like NULL dereference. The code path merely sets runtime->oss.trigger and start_threshold flags, then issues the ioctl outside the lock. Unless you really hit a problem with a fuzzer, the changelog is misleading and better to be rewritten. thanks, Takashi > > Signed-off-by: Ma Ke > --- > sound/core/oss/pcm_oss.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c > index 728c211142d1..fd9d23c3684b 100644 > --- a/sound/core/oss/pcm_oss.c > +++ b/sound/core/oss/pcm_oss.c > @@ -2083,21 +2083,16 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr > psubstream = pcm_oss_file->streams[SNDRV_PCM_STREAM_PLAYBACK]; > csubstream = pcm_oss_file->streams[SNDRV_PCM_STREAM_CAPTURE]; > > - if (psubstream) { > - err = snd_pcm_oss_make_ready(psubstream); > - if (err < 0) > - return err; > - } > - if (csubstream) { > - err = snd_pcm_oss_make_ready(csubstream); > - if (err < 0) > - return err; > - } > if (psubstream) { > runtime = psubstream->runtime; > cmd = 0; > if (mutex_lock_interruptible(&runtime->oss.params_lock)) > return -ERESTARTSYS; > + err = snd_pcm_oss_make_ready_locked(psubstream); > + if (err < 0) { > + mutex_unlock(&runtime->oss.params_lock); > + return err; > + } > if (trigger & PCM_ENABLE_OUTPUT) { > if (runtime->oss.trigger) > goto _skip1; > @@ -2128,6 +2123,11 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr > cmd = 0; > if (mutex_lock_interruptible(&runtime->oss.params_lock)) > return -ERESTARTSYS; > + err = snd_pcm_oss_make_ready_locked(csubstream); > + if (err < 0) { > + mutex_unlock(&runtime->oss.params_lock); > + return err; > + } > if (trigger & PCM_ENABLE_INPUT) { > if (runtime->oss.trigger) > goto _skip2; > -- > 2.37.2 >