| Message ID | 20241103-coverity1586045integeroverflow-v1-1-43ea37a3f3cd@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: invensense: fix integer overflow while multiplication | expand |
On Sun, 03 Nov 2024 08:43:14 +0000 Karan Sanghavi <karansanghvi98@gmail.com> wrote: Hi Karan, > Typecast a variable to int64_t for 64-bit arithmetic multiplication The path to actually triggering this is non obvious as these inputs are the result of rather complex code paths and per chip constraints. Have you identified a particular combination that overflows or is this just based on the type? I have no problem with applying this as hardening against future uses but unless we have a path to trigger it today it isn't a fix. If you do have a path, this description should state what it is. > > Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com> If it's a real bug, needs a Fixes tag so we know how far to backport it. > --- > drivers/iio/common/inv_sensors/inv_sensors_timestamp.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c > index f44458c380d9..d1d11d0b2458 100644 > --- a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c > +++ b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c > @@ -105,8 +105,8 @@ static bool inv_update_chip_period(struct inv_sensors_timestamp *ts, > > static void inv_align_timestamp_it(struct inv_sensors_timestamp *ts) > { > - const int64_t period_min = ts->min_period * ts->mult; > - const int64_t period_max = ts->max_period * ts->mult; > + const int64_t period_min = (int64_t)ts->min_period * ts->mult; > + const int64_t period_max = (int64_t)ts->max_period * ts->mult; > int64_t add_max, sub_max; > int64_t delta, jitter; > int64_t adjust; > > --- > base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e > change-id: 20241102-coverity1586045integeroverflow-cbbf357475d9 > > Best regards,
On Sun, Nov 03, 2024 at 11:18:27AM +0000, Jonathan Cameron wrote: > On Sun, 03 Nov 2024 08:43:14 +0000 > Karan Sanghavi <karansanghvi98@gmail.com> wrote: > > Hi Karan, > > > Typecast a variable to int64_t for 64-bit arithmetic multiplication > > The path to actually triggering this is non obvious as these > inputs are the result of rather complex code paths and per chip > constraints. Have you identified a particular combination that overflows > or is this just based on the type? I have no problem with applying this > as hardening against future uses but unless we have a path to trigger > it today it isn't a fix. > > If you do have a path, this description should state what it is. > The above issue is discovered by Coverity with CID 1586045 and 1586044. Link: https://scan7.scan.coverity.com/#/project-view/51946/11354?selectedIssue=1586045 Should I mention this path in the commit short message? > > > > Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com> > If it's a real bug, needs a Fixes tag so we know how far to backport it. > What kind of Fixes tag should I provide here. > > --- > > drivers/iio/common/inv_sensors/inv_sensors_timestamp.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c > > index f44458c380d9..d1d11d0b2458 100644 > > --- a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c > > +++ b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c > > @@ -105,8 +105,8 @@ static bool inv_update_chip_period(struct inv_sensors_timestamp *ts, > > > > static void inv_align_timestamp_it(struct inv_sensors_timestamp *ts) > > { > > - const int64_t period_min = ts->min_period * ts->mult; > > - const int64_t period_max = ts->max_period * ts->mult; > > + const int64_t period_min = (int64_t)ts->min_period * ts->mult; > > + const int64_t period_max = (int64_t)ts->max_period * ts->mult; > > int64_t add_max, sub_max; > > int64_t delta, jitter; > > int64_t adjust; > > > > --- > > base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e > > change-id: 20241102-coverity1586045integeroverflow-cbbf357475d9 > > > > Best regards, > Thank you, Karan.
On Sun, Nov 03, 2024 at 11:18:27AM +0000, Jonathan Cameron wrote: > On Sun, 03 Nov 2024 08:43:14 +0000 > Karan Sanghavi <karansanghvi98@gmail.com> wrote: > > Hi Karan, > > > Typecast a variable to int64_t for 64-bit arithmetic multiplication > > The path to actually triggering this is non obvious as these > inputs are the result of rather complex code paths and per chip > constraints. Have you identified a particular combination that overflows > or is this just based on the type? I have no problem with applying this > as hardening against future uses but unless we have a path to trigger > it today it isn't a fix. > > If you do have a path, this description should state what it is. > I found this in the coverity scan with CID:1586045 stating overflow_before_widen: Potentially overflowing expression ts->min_period * ts->mult with type unsigned int (32 bits, unsigned) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type int64_t const (64 bits, signed). > > > > Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com> > If it's a real bug, needs a Fixes tag so we know how far to backport it. > I thought that this is a fix to this coverity issue thus used fix in the subject. If I have to mention the fix tag for this then can you please let me know what should I mention in the fix tag. > > --- > > drivers/iio/common/inv_sensors/inv_sensors_timestamp.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c > > index f44458c380d9..d1d11d0b2458 100644 > > --- a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c > > +++ b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c > > @@ -105,8 +105,8 @@ static bool inv_update_chip_period(struct inv_sensors_timestamp *ts, > > > > static void inv_align_timestamp_it(struct inv_sensors_timestamp *ts) > > { > > - const int64_t period_min = ts->min_period * ts->mult; > > - const int64_t period_max = ts->max_period * ts->mult; > > + const int64_t period_min = (int64_t)ts->min_period * ts->mult; > > + const int64_t period_max = (int64_t)ts->max_period * ts->mult; > > int64_t add_max, sub_max; > > int64_t delta, jitter; > > int64_t adjust; > > > > --- > > base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e > > change-id: 20241102-coverity1586045integeroverflow-cbbf357475d9 > > > > Best regards, > Thank you, Karan.
On Mon, 4 Nov 2024 16:26:31 +0000 Karan Sanghavi <karansanghvi98@gmail.com> wrote: > On Sun, Nov 03, 2024 at 11:18:27AM +0000, Jonathan Cameron wrote: > > On Sun, 03 Nov 2024 08:43:14 +0000 > > Karan Sanghavi <karansanghvi98@gmail.com> wrote: > > > > Hi Karan, > > > > > Typecast a variable to int64_t for 64-bit arithmetic multiplication > > > > The path to actually triggering this is non obvious as these > > inputs are the result of rather complex code paths and per chip > > constraints. Have you identified a particular combination that overflows > > or is this just based on the type? I have no problem with applying this > > as hardening against future uses but unless we have a path to trigger > > it today it isn't a fix. > > > > If you do have a path, this description should state what it is. > > > > The above issue is discovered by Coverity with CID 1586045 and 1586044. > Link: https://scan7.scan.coverity.com/#/project-view/51946/11354?selectedIssue=1586045 > > Should I mention this path in the commit short message? That wasn't what I meant. I was after what combination of possible inputs actually trigger this rather than (I suspect) local analysis coverity has done. > > > > > > > Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com> > > If it's a real bug, needs a Fixes tag so we know how far to backport it. > > > > What kind of Fixes tag should I provide here. The patch that introduced the bug in the first place. See submitting patches docs for the format. However, I suspect this is coverity firing a false positive be it a reasonable one that we should tidy up. As such I'll queue this patch up, but not as a fix that I'm rushing in, but just as general cleanup for next cycle. If you find a path to trigger the overflow via userspace inputs etc then I'm happy to move it over to being handled as an urgent fix. Jonathan > > > > --- > > > drivers/iio/common/inv_sensors/inv_sensors_timestamp.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c > > > index f44458c380d9..d1d11d0b2458 100644 > > > --- a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c > > > +++ b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c > > > @@ -105,8 +105,8 @@ static bool inv_update_chip_period(struct inv_sensors_timestamp *ts, > > > > > > static void inv_align_timestamp_it(struct inv_sensors_timestamp *ts) > > > { > > > - const int64_t period_min = ts->min_period * ts->mult; > > > - const int64_t period_max = ts->max_period * ts->mult; > > > + const int64_t period_min = (int64_t)ts->min_period * ts->mult; > > > + const int64_t period_max = (int64_t)ts->max_period * ts->mult; > > > int64_t add_max, sub_max; > > > int64_t delta, jitter; > > > int64_t adjust; > > > > > > --- > > > base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e > > > change-id: 20241102-coverity1586045integeroverflow-cbbf357475d9 > > > > > > Best regards, > > > > Thank you, > Karan.
diff --git a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c index f44458c380d9..d1d11d0b2458 100644 --- a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c +++ b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c @@ -105,8 +105,8 @@ static bool inv_update_chip_period(struct inv_sensors_timestamp *ts, static void inv_align_timestamp_it(struct inv_sensors_timestamp *ts) { - const int64_t period_min = ts->min_period * ts->mult; - const int64_t period_max = ts->max_period * ts->mult; + const int64_t period_min = (int64_t)ts->min_period * ts->mult; + const int64_t period_max = (int64_t)ts->max_period * ts->mult; int64_t add_max, sub_max; int64_t delta, jitter; int64_t adjust;
Typecast a variable to int64_t for 64-bit arithmetic multiplication Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com> --- drivers/iio/common/inv_sensors/inv_sensors_timestamp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e change-id: 20241102-coverity1586045integeroverflow-cbbf357475d9 Best regards,