Re: [PATCH] ALSA: intel8x0: fix a redundant check bug

[Takashi Iwai]

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@xxxxxxx>

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
> 
>