zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.05k stars 6.18k forks source link

[Coverity CID: 353644] Unintended sign extension in drivers/sensor/ams/tsl2591/tsl2591.c #74750

Open zephyrbot opened 2 weeks ago

zephyrbot commented 2 weeks ago

Static code scan issues found in file:

https://github.com/zephyrproject-rtos/zephyr/tree/dcf42917c550714d2457947538b9e29d083e872e/drivers/sensor/ams/tsl2591/tsl2591.c#L139

Category: Integer handling issues Function: tsl2591_channel_get Component: Drivers CID: 353644

Details:

https://github.com/zephyrproject-rtos/zephyr/blob/dcf42917c550714d2457947538b9e29d083e872e/drivers/sensor/ams/tsl2591/tsl2591.c#L139

133     }
134    
135     static int tsl2591_channel_get(const struct device *dev, enum sensor_channel chan,
136                             struct sensor_value *val)
137     {
138      const struct tsl2591_data *data = dev->data;
>>>     CID 353644:  Integer handling issues  (SIGN_EXTENSION)
>>>     Suspicious implicit sign extension: "data->atime" with type "uint16_t" (16 bits, unsigned) is promoted in "data->atime * data->again" to type "int" (32 bits, signed), then sign-extended to type "long long" (64 bits, signed).  If "data->atime * data->again" is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.
139      int64_t cpl = data->atime * data->again;
140      int64_t strength;
141    
142      /* Unfortunately, datasheet does not provide a lux conversion formula for this particular
143       * device. There is still ongoing discussion about the proper formula, though this
144       * implementation uses a slightly modified version of the Adafruit library formula:

For more information about the violation, check the Coverity Reference. (CWE-194)

Please fix or provide comments in coverity using the link:

https://scan9.scan.coverity.com/#/project-view/29271/12996?selectedIssue=353644

Note: This issue was created automatically. Priority was set based on classification of the file affected and the impact field in coverity. Assignees were set using the MAINTAINERS file.

MaureenHelm commented 1 week ago

@kurtjd please take a look

kurtjd commented 6 days ago

Although with the code as currently written this statement can't overflow (since data->again can never exceed 9,200 and data->atime can never exceed 600), it doesn't hurt to be on the safe side. I'll get a PR for a fix submitted shortly!