Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 09 Oct 2018 16:51:43 +0200
Message-ID: <s5hd0sjyxz4.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     "Wenwen Wang" <wang6495@umn.edu>
Cc:     "moderated list:SOUND" <alsa-devel@alsa-project.org>,
        "Jaroslav Kysela" <perex@perex.cz>, "Kangjie Lu" <kjlu@umn.edu>,
        "open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ALSA: intel8x0: fix a redundant check bug
In-Reply-To: <1539095754-5609-1-git-send-email-wang6495@umn.edu>
References: <1539095754-5609-1-git-send-email-wang6495@umn.edu>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/26
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 09 Oct 2018 16:35:52 +0200,
Wenwen Wang wrote:
> 
> In snd_intel8x0_codec_semaphore(), the parameter 'codec' is firstly
> checked to see whether it is greater than 2. If yes, an error code EIO is
> returned. Otherwise, 'chip->in_sdin_init' is further checked. If
> 'chip->in_sdin_init' is not zero, 'codec' is updated immediately with
> 'chip->codec_isr_bits'. That means, the parameter 'codec' is not used in
> this branch. Actually, it is only used in the else branch when
> 'chip->in_sdin_init' is zero. Thus, the check on the parameter 'codec' is
> redundant if 'chip->in_sdin_init' is not zero. This can cause potential
> incorrect execution in this function. Suppose the parameter 'codec' is 3,
> which is greater than 2, and 'chip->in_sdin_init' is not zero. The current
> version of this function will return EIO after the first check because
> 'codec' is greater than 2. However, since 'codec' will be updated in the
> following execution when 'chip->in_sdin_init' is not zero, this check will
> be meaningless and the execution should continue, instead of returning the
> error code EIO.
> 
> This patch avoids the above issue by moving the check on the parameter
> 'codec' to the else branch of the if statement that checks
> 'chip->in_sdin_init'.
> 
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>

Passing codec > 2 is just incorrect, no matter whether it's in
in_sdin_init state of not.  In the in_sdin_init state, it's supposed
to be ignored, but still passing such a value is already wrong.

That said, there is no need to skip the check.


Of course, if your patch fixes any real issue (i.e. a bug), I'll
happily apply the patch.  In that case, please show the exact use case
that hits the bug.


thanks,

Takashi

> ---
>  sound/pci/intel8x0.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> index 5ee468d..f619e58 100644
> --- a/sound/pci/intel8x0.c
> +++ b/sound/pci/intel8x0.c
> @@ -515,13 +515,13 @@ static int snd_intel8x0_codec_semaphore(struct intel8x0 *chip, unsigned int code
>  {
>  	int time;
>  	
> -	if (codec > 2)
> -		return -EIO;
>  	if (chip->in_sdin_init) {
>  		/* we don't know the ready bit assignment at the moment */
>  		/* so we check any */
>  		codec = chip->codec_isr_bits;
>  	} else {
> +		if (codec > 2)
> +			return -EIO;
>  		codec = chip->codec_bit[chip->ac97_sdin[codec]];
>  	}
>  
> -- 
> 2.7.4
> 
>