drivers: video: video_common: refactor video_closest_frmival_stepwise() #85697

josuah · 2025-02-13T01:03:45Z

Avoid an integer overflow by reducing the precision until the result is checked to fit in 32-bit. This is possible by simplifying fractions by 2, which should not affect the result significantly as this is only done when the values are extremely large (too large to fit 32-bit int).

Also adding a test case that would overflow without proper handling.

Fixes:

[Coverity CID: 444385] Unintentional integer overflow in drivers/video/video_common.c #84779
[Coverity CID: 444388] Unintentional integer overflow in drivers/video/video_common.c #84776
[Coverity CID: 444389] Unintentional integer overflow in drivers/video/video_common.c #84775

fabiobaltieri · 2025-02-13T11:11:34Z

drivers/video/video_common.c

+
+	/* Loose precision until landing within 32-bit range */
+	while (numerator_min > UINT32_MAX || numerator_step > UINT32_MAX ||
+	       denominator >UINT32_MAX) {


^^ missing space, see the compliance check

You are right... Should be fixed now!

github-actions · 2025-04-15T00:37:16Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

ngphibang · 2025-04-15T07:20:48Z

drivers/video/video_common.c

+	uint64_t numerator_min = stepwise->min.numerator;
+	uint64_t numerator_step = stepwise->step.numerator;
+	uint64_t numerator_goal = desired->numerator;
+	uint64_t denominator;
+	uint64_t numerator;


It should be uint32_t. There is a mistake in the mipi_csi2rx driver that I pushed a fix.

https://elixir.bootlin.com/linux/v6.11.1/source/include/uapi/linux/videodev2.h#L441

The local variables were made 64-bit to allow storing 32-bit by 32-bit multiplication result, but this might be solved through your other comment.

Yes, sorry, we indeed need uint64_t for storing the intermediate result.

ngphibang · 2025-04-15T07:25:32Z

drivers/video/video_common.c

+	/* Set a common denominator to these 2 values */
+	numerator_min *= stepwise->step.denominator;
+	numerator_step *= stepwise->min.denominator;
+	denominator = stepwise->min.denominator * stepwise->step.denominator;
+
+	/* Loose precision until landing within 32-bit range */
+	while (numerator_min > UINT32_MAX || numerator_step > UINT32_MAX ||
+	       denominator > UINT32_MAX) {
+		numerator_min /= 2;
+		numerator_step /= 2;
+		denominator /= 2;
+	}
+
+	/* Set a common denominator to these 3 values */
+	numerator_min *= desired->denominator;
+	numerator_step *= desired->denominator;
+	numerator_goal *= denominator;
+	denominator *= desired->denominator;
+
+	/* Loose precision until landing within 32-bit range */
+	while (numerator_min > UINT32_MAX || numerator_step > UINT32_MAX ||
+	       numerator_goal > UINT32_MAX || denominator > UINT32_MAX) {
+		numerator_min /= 2;
+		numerator_step /= 2;
+		numerator_goal /= 2;
+		denominator /= 2;
+	}


I think there should be a method to avoid exceeding the range ? For example, x * y / z could be done as x / z * y

I did not find one the last time except converting everything to frame intervals in microseconds and compare the values like this... Which would simplify everything. Is it good enough?

Yes, I see now, this is not a simple problem ...

Would it be possible and simpler if we activate CONFIG_FPU to do the computation ?

Maybe converting back and forth with integers and floats would give good precision, but not give exact fractional results, i.e. 1000/9999 vs 1/10. Maybe that works everywhere though!

Should frame intervals only be stored as nanosecond instead of fractions to avoid these kind of problems?

I just pushed a compromise performing the comparison as nanoseconds (using video_frmival_nsec(), could lead to different results in extreme "tie-breaker" situations), but storing the result as a fraction (like currently).

ngphibang · 2025-04-16T15:08:26Z

drivers/video/video_common.c

+	uint64_t numerator_min = stepwise->min.numerator;
+	uint64_t numerator_step = stepwise->step.numerator;
+	uint64_t numerator_goal = desired->numerator;
+	uint64_t denominator;
+	uint64_t numerator;


Yes, sorry, we indeed need uint64_t for storing the intermediate result.

ngphibang · 2025-04-16T16:10:33Z

drivers/video/video_common.c

+	/* Set a common denominator to these 2 values */
+	numerator_min *= stepwise->step.denominator;
+	numerator_step *= stepwise->min.denominator;
+	denominator = stepwise->min.denominator * stepwise->step.denominator;
+
+	/* Loose precision until landing within 32-bit range */
+	while (numerator_min > UINT32_MAX || numerator_step > UINT32_MAX ||
+	       denominator > UINT32_MAX) {
+		numerator_min /= 2;
+		numerator_step /= 2;
+		denominator /= 2;
+	}
+
+	/* Set a common denominator to these 3 values */
+	numerator_min *= desired->denominator;
+	numerator_step *= desired->denominator;
+	numerator_goal *= denominator;
+	denominator *= desired->denominator;
+
+	/* Loose precision until landing within 32-bit range */
+	while (numerator_min > UINT32_MAX || numerator_step > UINT32_MAX ||
+	       numerator_goal > UINT32_MAX || denominator > UINT32_MAX) {
+		numerator_min /= 2;
+		numerator_step /= 2;
+		numerator_goal /= 2;
+		denominator /= 2;
+	}


Yes, I see now, this is not a simple problem ...

Would it be possible and simpler if we activate CONFIG_FPU to do the computation ?

josuah · 2025-04-16T20:48:20Z

force-push:

simplify: comparison done as nanoseconds (simpler), result stored as exact fraction (as accurate)
rebase on top of latest main

josuah · 2025-06-02T12:16:55Z

Force-push:

Rebase on top of main after more commits were added to video_common.c

josuah · 2025-06-02T12:30:16Z

@ngphibang Would it be possible and simpler if we activate CONFIG_FPU to do the computation?

That sounds more than enough! I will try that now...

josuah · 2025-06-02T12:43:18Z

Force-push:

Convert to uint64_t instead of fractions

It seems like double would not have added more precision in this particular case.
If this considered good enough precision for video system, would it make sense to convert everything to use frame intervals? For instance, USB Video uses 100 nsec units instead of fractions.

Otherwise, feel free to validate this PR only and this will be scoped to only video_closest_frmival_stepwise() implementation.

josuah · 2025-06-02T18:35:13Z

I'll go fix the CI now, please wait a little more...

josuah · 2025-06-07T20:23:47Z

Force-push:

Rebase on latest main

josuah · 2025-06-07T21:05:44Z

By making all the computation in nanoseconds, we start to loose a bit of precision when the numerator and denominator are both huge:

>>> (29994500/20000000)
1.499725
>>> (1499999985/1000000000)
1.499999985

This test was passing without imprecision.

Avoid an integer overflow by dividing the numerator and denominator by two (dropping one bit of precision), which preserve the ratio. Result still accurate, as only done on values larger than UINT32_MAX. fixes CID-444385, CID-444388 and CID-444389 Signed-off-by: Josuah Demangeon <[email protected]>

sonarqubecloud · 2025-06-07T21:13:03Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

josuah · 2025-06-07T21:14:04Z

Now CI passes, but I had to introduce between 10 and 1000 nanoseconds of imprecision.
I will try with doubles and see if that improves it.

josuah · 2025-06-19T08:12:48Z

Double is not usable as the computation relies on integer division to select the correct step, with DIV_ROUND_CLOSEST() doing the actual selection operation.

josuah added area: Drivers area: Video Video subsystem labels Feb 13, 2025

zephyrbot requested review from loicpoulain and ngphibang February 13, 2025 01:04

josuah force-pushed the pr-fix-video-common branch from 2f1c32e to e62d581 Compare February 13, 2025 01:04

josuah mentioned this pull request Feb 13, 2025

[Coverity CID: 444378] Macro compares unsigned to 0 in drivers/video/video_common.c #84783

Open

josuah force-pushed the pr-fix-video-common branch from e62d581 to 7f723af Compare February 13, 2025 01:12

fabiobaltieri reviewed Feb 13, 2025

View reviewed changes

josuah force-pushed the pr-fix-video-common branch from 7f723af to eaee9f1 Compare February 13, 2025 12:49

github-actions bot added the Stale label Apr 15, 2025

ngphibang reviewed Apr 15, 2025

View reviewed changes

github-actions bot removed the Stale label Apr 16, 2025

ngphibang previously approved these changes Apr 16, 2025

View reviewed changes

josuah dismissed ngphibang’s stale review via 6b00d3e April 16, 2025 20:38

josuah force-pushed the pr-fix-video-common branch 4 times, most recently from 6667c1c to 0d7bacf Compare April 16, 2025 20:46

josuah force-pushed the pr-fix-video-common branch from 0d7bacf to 1644eb2 Compare June 2, 2025 12:15

github-actions bot requested a review from avolmat-st June 2, 2025 12:16

github-actions bot assigned ngphibang Jun 2, 2025

josuah force-pushed the pr-fix-video-common branch from 1644eb2 to 7561345 Compare June 2, 2025 12:41

josuah requested a review from ngphibang June 2, 2025 12:43

josuah marked this pull request as draft June 2, 2025 18:35

josuah force-pushed the pr-fix-video-common branch from 7561345 to a9b9c54 Compare June 7, 2025 20:23

josuah force-pushed the pr-fix-video-common branch from a9b9c54 to 9cc9fe1 Compare June 7, 2025 21:06

josuah force-pushed the pr-fix-video-common branch from 9cc9fe1 to 34d38bd Compare June 7, 2025 21:09

josuah mentioned this pull request Jun 19, 2025

video: common: prevent divide-by-zero in video_closest_frmival_stepwi… #91893

Open

drivers: video: video_common: refactor video_closest_frmival_stepwise() #85697

Are you sure you want to change the base?

drivers: video: video_common: refactor video_closest_frmival_stepwise() #85697

Conversation

josuah commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 15, 2025

Uh oh!

ngphibang Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josuah commented Apr 16, 2025

Uh oh!

josuah commented Jun 2, 2025

Uh oh!

josuah commented Jun 2, 2025

Uh oh!

josuah commented Jun 2, 2025

Uh oh!

josuah commented Jun 2, 2025

Uh oh!

josuah commented Jun 7, 2025

Uh oh!

josuah commented Jun 7, 2025

Uh oh!

sonarqubecloud bot commented Jun 7, 2025

Quality Gate passed

Uh oh!

josuah commented Jun 7, 2025

Uh oh!

josuah commented Jun 19, 2025

Uh oh!

Uh oh!

josuah commented Feb 13, 2025 •

edited

Loading

ngphibang Apr 15, 2025 •

edited

Loading