linux-kernel - Re: [media-pci-cx25821] question about value overwrite

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]

Date:   Mon, 22 May 2017 09:45:31 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     "Gustavo A. R. Silva" <garsilva@...eddedor.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [media-pci-cx25821] question about value overwrite

On 05/19/2017 12:07 AM, Gustavo A. R. Silva wrote:
> 
> Hello everybody,
> 
> While looking into Coverity ID 1226903 I ran into the following piece  
> of code at drivers/media/pci/cx25821/cx25821-medusa-video.c:393:
> 
> 393int medusa_set_videostandard(struct cx25821_dev *dev)
> 394{
> 395        int status = 0;
> 396        u32 value = 0, tmp = 0;
> 397
> 398        if (dev->tvnorm & V4L2_STD_PAL_BG || dev->tvnorm & V4L2_STD_PAL_DK)
> 399                status = medusa_initialize_pal(dev);
> 400        else
> 401                status = medusa_initialize_ntsc(dev);
> 402
> 403        /* Enable DENC_A output */
> 404        value = cx25821_i2c_read(&dev->i2c_bus[0], DENC_A_REG_4, &tmp);
> 405        value = setBitAtPos(value, 4);
> 406        status = cx25821_i2c_write(&dev->i2c_bus[0], DENC_A_REG_4, value);
> 407
> 408        /* Enable DENC_B output */
> 409        value = cx25821_i2c_read(&dev->i2c_bus[0], DENC_B_REG_4, &tmp);
> 410        value = setBitAtPos(value, 4);
> 411        status = cx25821_i2c_write(&dev->i2c_bus[0], DENC_B_REG_4, value);
> 412
> 413        return status;
> 414}
> 
> The issue is that the value stored in variable _status_ at lines 399  
> and 401 is overwritten by the one stored at line 406 and then at line  
> 411, before it can be used.
> 
> My question is if the original intention was to ORed the return  
> values, something like in the following patch:
> 
> index 0a9db05..226d14f 100644
> --- a/drivers/media/pci/cx25821/cx25821-medusa-video.c
> +++ b/drivers/media/pci/cx25821/cx25821-medusa-video.c
> @@ -403,12 +403,12 @@ int medusa_set_videostandard(struct cx25821_dev *dev)
>          /* Enable DENC_A output */
>          value = cx25821_i2c_read(&dev->i2c_bus[0], DENC_A_REG_4, &tmp);
>          value = setBitAtPos(value, 4);
> -       status = cx25821_i2c_write(&dev->i2c_bus[0], DENC_A_REG_4, value);
> +       status |= cx25821_i2c_write(&dev->i2c_bus[0], DENC_A_REG_4, value);
> 
>          /* Enable DENC_B output */
>          value = cx25821_i2c_read(&dev->i2c_bus[0], DENC_B_REG_4, &tmp);
>          value = setBitAtPos(value, 4);
> -       status = cx25821_i2c_write(&dev->i2c_bus[0], DENC_B_REG_4, value);
> +       status |= cx25821_i2c_write(&dev->i2c_bus[0], DENC_B_REG_4, value);
> 
>          return status;
>   }

This is a crappy driver, they just couldn't be bothered to check the error from
cx25821_i2c_read/write.

Strictly speaking the return value should be checked after every read/write and
returned in case of an error.

Not sure whether it is worth the effort fixing this.

Regards,

	Hans

> 
> What do you think?
> 
> I'd really appreciate any comment on this.
> 
> Thank you!
> --
> Gustavo A. R. Silva
> 
> 
> 
>