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] [day] [month] [year] [list]

Date:   Tue, 20 Jun 2017 16:57:56 -0500
From:   "Gustavo A. R. Silva" <garsilva@...eddedor.com>
To:     Hans Verkuil <hverkuil@...all.nl>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [media-pci-cx25821] question about value overwrite

Hi Hans,

Quoting Hans Verkuil <hverkuil@...all.nl>:

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

Yeah, the same happens in functions medusa_initialize_pal() and  
medusa_initialize_ntsc()

> Not sure whether it is worth the effort fixing this.
>

Thank you for your reply.
--
Gustavo A. R. Silva