Skip to content

[Coverity CID :195793]Insecure data handling in /drivers/counter/counter_ll_stm32_rtc.c #14415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aasthagr opened this issue Mar 12, 2019 · 5 comments
Assignees
Labels
area: Counter bug The issue is a bug, or the PR is fixing a bug Coverity A Coverity detected issue or its fix platform: STM32 ST Micro STM32 priority: medium Medium impact/importance bug

Comments

@aasthagr
Copy link
Collaborator

Static code scan issues seen in File: /drivers/counter/counter_ll_stm32_rtc.c
Category: Insecure data handling
Function: rtc_stm32_read
Component: Drivers
CID: 195793
Please fix or provide comments to square it off in coverity in the link: https://scan9.coverity.com/reports.htm#v32951/p12996

@galak galak added Coverity A Coverity detected issue or its fix priority: medium Medium impact/importance bug labels Mar 12, 2019
@galak galak added area: Counter platform: STM32 ST Micro STM32 bug The issue is a bug, or the PR is fixing a bug and removed bug The issue is a bug, or the PR is fixing a bug labels Mar 12, 2019
@erwango
Copy link
Member

erwango commented Mar 14, 2019

@galak, coverity complains on :

	time_t ts;
...
	ticks = counter_us_to_ticks(dev, (u64_t)(ts * USEC_PER_SEC));

With following comment:

CID 195871 (#1 of 1): Unintentional integer overflow 
(OVERFLOW_BEFORE_WIDEN)overflow_before_widen: Potentially overflowing expression ts * 
1000000UL with type unsigned long (32 bits, unsigned) is evaluated using 32-bit arithmetic,
and  then used in a context that expects an expression of type u64_t (64 bits, unsigned).
   To avoid overflow, cast either ts or 1000000UL to type u64_t rather than casting
 the result of the potential overflow.

But, sizeof(time_t) returns 8...

So actually this piece of code doesn't work anymore (it used to), and I need to cast ts on u32_t to get it back working. Is that possible time_t has changes from 4 to 8 bytes recently?

@GeorgeCGV
Copy link
Collaborator

For me with newlibc (required by stm32 rtc driver) with arm none eabi 7-2017-q4-major toolchain sizeof types u32_t, s32_t, and time_t are all 4 bytes.

@erwango
Copy link
Member

erwango commented Mar 15, 2019

with arm none eabi 7-2017-q4-major toolchain sizeof types u32_t, s32_t, and time_t are all 4 bytes

On my side, I'm using SDK0.10.0

@erwango
Copy link
Member

erwango commented Mar 15, 2019

Coverity analysis assumes time_t is 4 bytes while it has been updated to 8 bytes with SDK0.10.0.
Report does not apply anymore with 8 byte time_t, so issue will be fixed by upgrading SDK to 0.10.0 at next run.
I'm not able to update coverity report with this anlaysis (rights issue?), @aasthagr can you do it?

@GeorgeCGV
Copy link
Collaborator

GeorgeCGV commented Mar 15, 2019

newlib time_t has been changed to signed 64 bits integer by default in commit 4de8754bac676fc7f29082ccf8281b1c73ce629e back in September 2017.

SDK 0.10.0 uses newlib 3.1.0.
And, as it seems from the release notes, amr none eabi 7-2017-q4-major shall already include the newlib with the 64 bits time_t (but not in versions prior 7-2017-q4-major).

@erwango erwango closed this as completed Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Counter bug The issue is a bug, or the PR is fixing a bug Coverity A Coverity detected issue or its fix platform: STM32 ST Micro STM32 priority: medium Medium impact/importance bug
Projects
None yet
Development

No branches or pull requests

4 participants