* Re: [PATCH] comedi: comedi_test: avoid AI scan timing overflow
2026-06-09 0:14 [PATCH] comedi: comedi_test: avoid AI scan timing overflow Samuel Moelius
@ 2026-06-09 5:18 ` Greg Kroah-Hartman
2026-06-09 8:54 ` Ian Abbott
2026-06-09 9:07 ` Ian Abbott
2026-06-09 10:28 ` Ian Abbott
2 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2026-06-09 5:18 UTC (permalink / raw)
To: Samuel Moelius; +Cc: Ian Abbott, H Hartley Sweeten, open list
On Tue, Jun 09, 2026 at 12:14:04AM +0000, Samuel Moelius wrote:
> `waveform_ai_cmdtest()` tries to keep timer-driven analog-input scans
> representable by limiting `convert_arg` and by making `scan_begin_arg`
> at least `convert_arg * scan_end_arg`.
Please don't use markdown crud in changelog comments :(
> The conversion clamp tested `scan_begin_arg == TRIG_TIMER` instead of
> `scan_begin_src == TRIG_TIMER`, so normal timer scans skipped the clamp.
> The later product was computed in `unsigned int`, allowing a large
> conversion period to wrap and produce an accepted command whose true
> scan conversion time exceeds the scan period.
Is that a real problem as this is a test module?
> Require timer conversions to be at least one microsecond, apply the
> conversion limit when `scan_begin_src` is `TRIG_TIMER`, keep that limit
> on the same microsecond granularity as the rounded argument, and compute
> the scan-period floor in 64-bit before clamping it back to the ioctl
> argument range.
>
> Assisted-by: Codex:gpt-5.5-cyber-preview
> Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
What commit id does this fix? How was this tested?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] comedi: comedi_test: avoid AI scan timing overflow
2026-06-09 5:18 ` Greg Kroah-Hartman
@ 2026-06-09 8:54 ` Ian Abbott
2026-06-09 10:10 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: Ian Abbott @ 2026-06-09 8:54 UTC (permalink / raw)
To: Greg Kroah-Hartman, Samuel Moelius; +Cc: H Hartley Sweeten, open list
On 09/06/2026 06:18, Greg Kroah-Hartman wrote:
> On Tue, Jun 09, 2026 at 12:14:04AM +0000, Samuel Moelius wrote:
>> `waveform_ai_cmdtest()` tries to keep timer-driven analog-input scans
>> representable by limiting `convert_arg` and by making `scan_begin_arg`
>> at least `convert_arg * scan_end_arg`.
>
> Please don't use markdown crud in changelog comments :(
TBF I also use back-quotes for quoting literal programming text. Not
because they are the same quote marks that markdown uses to quote
literal text, but because they cannot be confused with C syntax.
--
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] comedi: comedi_test: avoid AI scan timing overflow
2026-06-09 8:54 ` Ian Abbott
@ 2026-06-09 10:10 ` Greg Kroah-Hartman
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2026-06-09 10:10 UTC (permalink / raw)
To: Ian Abbott; +Cc: Samuel Moelius, H Hartley Sweeten, open list
On Tue, Jun 09, 2026 at 09:54:51AM +0100, Ian Abbott wrote:
> On 09/06/2026 06:18, Greg Kroah-Hartman wrote:
> > On Tue, Jun 09, 2026 at 12:14:04AM +0000, Samuel Moelius wrote:
> > > `waveform_ai_cmdtest()` tries to keep timer-driven analog-input scans
> > > representable by limiting `convert_arg` and by making `scan_begin_arg`
> > > at least `convert_arg * scan_end_arg`.
> >
> > Please don't use markdown crud in changelog comments :(
>
> TBF I also use back-quotes for quoting literal programming text. Not
> because they are the same quote marks that markdown uses to quote literal
> text, but because they cannot be confused with C syntax.
But in a changelog, it's not needed. If it's a function, use () at the
end, if it's a variable name, great, just leave it as-is. No need for
`` at all.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] comedi: comedi_test: avoid AI scan timing overflow
2026-06-09 0:14 [PATCH] comedi: comedi_test: avoid AI scan timing overflow Samuel Moelius
2026-06-09 5:18 ` Greg Kroah-Hartman
@ 2026-06-09 9:07 ` Ian Abbott
2026-06-09 10:28 ` Ian Abbott
2 siblings, 0 replies; 6+ messages in thread
From: Ian Abbott @ 2026-06-09 9:07 UTC (permalink / raw)
To: Samuel Moelius; +Cc: H Hartley Sweeten, Greg Kroah-Hartman, open list
On 09/06/2026 01:14, Samuel Moelius wrote:
> `waveform_ai_cmdtest()` tries to keep timer-driven analog-input scans
> representable by limiting `convert_arg` and by making `scan_begin_arg`
> at least `convert_arg * scan_end_arg`.
>
> The conversion clamp tested `scan_begin_arg == TRIG_TIMER` instead of
> `scan_begin_src == TRIG_TIMER`, so normal timer scans skipped the clamp.
> The later product was computed in `unsigned int`, allowing a large
> conversion period to wrap and produce an accepted command whose true
> scan conversion time exceeds the scan period.
>
> Require timer conversions to be at least one microsecond, apply the
> conversion limit when `scan_begin_src` is `TRIG_TIMER`, keep that limit
> on the same microsecond granularity as the rounded argument, and compute
> the scan-period floor in 64-bit before clamping it back to the ioctl
> argument range.
>
> Assisted-by: Codex:gpt-5.5-cyber-preview
> Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
> ---
> drivers/comedi/drivers/comedi_test.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/comedi/drivers/comedi_test.c b/drivers/comedi/drivers/comedi_test.c
> index 1f430ffc7bd9..04b982f69751 100644
> --- a/drivers/comedi/drivers/comedi_test.c
> +++ b/drivers/comedi/drivers/comedi_test.c
> @@ -256,7 +256,8 @@ static int waveform_ai_cmdtest(struct comedi_device *dev,
> struct comedi_cmd *cmd)
> {
> int err = 0;
> - unsigned int arg, limit;
> + unsigned int arg, limit, min_scan_arg;
> + u64 min_scan_time;
>
> /* Step 1 : check if triggers are trivially valid */
>
> @@ -292,10 +293,8 @@ static int waveform_ai_cmdtest(struct comedi_device *dev,
> if (cmd->convert_src == TRIG_NOW) {
> err |= comedi_check_trigger_arg_is(&cmd->convert_arg, 0);
> } else { /* cmd->convert_src == TRIG_TIMER */
> - if (cmd->scan_begin_src == TRIG_FOLLOW) {
> - err |= comedi_check_trigger_arg_min(&cmd->convert_arg,
> - NSEC_PER_USEC);
> - }
> + err |= comedi_check_trigger_arg_min(&cmd->convert_arg,
> + NSEC_PER_USEC);
> }
>
> if (cmd->scan_begin_src == TRIG_FOLLOW) {
> @@ -342,7 +341,12 @@ static int waveform_ai_cmdtest(struct comedi_device *dev,
> arg = NSEC_PER_USEC * DIV_ROUND_CLOSEST(arg, NSEC_PER_USEC);
> if (cmd->convert_src == TRIG_TIMER) {
> /* but ensure scan_begin_arg is large enough */
> - arg = max(arg, cmd->convert_arg * cmd->scan_end_arg);
> + min_scan_time = (u64)cmd->convert_arg * cmd->scan_end_arg;
> + if (min_scan_time > UINT_MAX)
> + min_scan_arg = UINT_MAX;
> + else
> + min_scan_arg = min_scan_time;
> + arg = max(arg, min_scan_arg);
> }
> err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, arg);
> }
If it is done that way, it is possible for scan_begin_arg to end up
shorter than scan_end_arg * convert_arg (when both scan_begin_src and
convert_src are set to TRIG_TIMER). I prefer to impose an upper limit
on convert_arg so that the product of scan_end_arg and convert_arg is <=
UINT_MAX.
--
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] comedi: comedi_test: avoid AI scan timing overflow
2026-06-09 0:14 [PATCH] comedi: comedi_test: avoid AI scan timing overflow Samuel Moelius
2026-06-09 5:18 ` Greg Kroah-Hartman
2026-06-09 9:07 ` Ian Abbott
@ 2026-06-09 10:28 ` Ian Abbott
2 siblings, 0 replies; 6+ messages in thread
From: Ian Abbott @ 2026-06-09 10:28 UTC (permalink / raw)
To: Samuel Moelius; +Cc: H Hartley Sweeten, Greg Kroah-Hartman, open list
On 09/06/2026 01:14, Samuel Moelius wrote:
> `waveform_ai_cmdtest()` tries to keep timer-driven analog-input scans
> representable by limiting `convert_arg` and by making `scan_begin_arg`
> at least `convert_arg * scan_end_arg`.
>
> The conversion clamp tested `scan_begin_arg == TRIG_TIMER` instead of
> `scan_begin_src == TRIG_TIMER`, so normal timer scans skipped the clamp.
That was fixed in commit 8a3bee801d42 ("comedi: comedi_test: Fix
limiting of convert_arg in waveform_ai_cmdtest()").
> The later product was computed in `unsigned int`, allowing a large
> conversion period to wrap and produce an accepted command whose true
> scan conversion time exceeds the scan period.
Commit 783ddaebd397 ("staging: comedi: comedi_test: support
scan_begin_src == TRIG_FOLLOW") imposed an upper limit on `convert_arg`
to stop the product `convert_arg * scan_end_arg` overflowing an
`unsigned int`.
> Require timer conversions to be at least one microsecond, apply the
> conversion limit when `scan_begin_src` is `TRIG_TIMER`, keep that limit
> on the same microsecond granularity as the rounded argument, and compute
> the scan-period floor in 64-bit before clamping it back to the ioctl
> argument range.
We could do that but it doesn't really matter for this driver. It
already supports `convert_src == TRIG_NOW` with `convert_src == 0` and
that is pretty much the same as `convert_src == TRIG_TIMER` with
`convert_src == 0`. The current implementation just allows
`convert_arg` to be arbitrarily small (including 0) when `convert_src ==
TRIG_TIMER` except when `scan_begin_src == TRIG_FOLLOW` because that
would also make the overall scan period arbitrarily small, which we do
not want.
>
> Assisted-by: Codex:gpt-5.5-cyber-preview
> Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
> ---
> drivers/comedi/drivers/comedi_test.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/comedi/drivers/comedi_test.c b/drivers/comedi/drivers/comedi_test.c
> index 1f430ffc7bd9..04b982f69751 100644
> --- a/drivers/comedi/drivers/comedi_test.c
> +++ b/drivers/comedi/drivers/comedi_test.c
> @@ -256,7 +256,8 @@ static int waveform_ai_cmdtest(struct comedi_device *dev,
> struct comedi_cmd *cmd)
> {
> int err = 0;
> - unsigned int arg, limit;
> + unsigned int arg, limit, min_scan_arg;
> + u64 min_scan_time;
>
> /* Step 1 : check if triggers are trivially valid */
>
> @@ -292,10 +293,8 @@ static int waveform_ai_cmdtest(struct comedi_device *dev,
> if (cmd->convert_src == TRIG_NOW) {
> err |= comedi_check_trigger_arg_is(&cmd->convert_arg, 0);
> } else { /* cmd->convert_src == TRIG_TIMER */
> - if (cmd->scan_begin_src == TRIG_FOLLOW) {
> - err |= comedi_check_trigger_arg_min(&cmd->convert_arg,
> - NSEC_PER_USEC);
> - }
> + err |= comedi_check_trigger_arg_min(&cmd->convert_arg,
> + NSEC_PER_USEC);
> }
>
> if (cmd->scan_begin_src == TRIG_FOLLOW) {
> @@ -342,7 +341,12 @@ static int waveform_ai_cmdtest(struct comedi_device *dev,
> arg = NSEC_PER_USEC * DIV_ROUND_CLOSEST(arg, NSEC_PER_USEC);
> if (cmd->convert_src == TRIG_TIMER) {
> /* but ensure scan_begin_arg is large enough */
> - arg = max(arg, cmd->convert_arg * cmd->scan_end_arg);
> + min_scan_time = (u64)cmd->convert_arg * cmd->scan_end_arg;
> + if (min_scan_time > UINT_MAX)
> + min_scan_arg = UINT_MAX;
> + else
> + min_scan_arg = min_scan_time;
> + arg = max(arg, min_scan_arg);
> }
> err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, arg);
> }
--
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
^ permalink raw reply [flat|nested] 6+ messages in thread