[PATCH] comedi: comedi_test: avoid AI scan timing overflow

[PATCH] comedi: comedi_test: avoid AI scan timing overflow

[Samuel Moelius]

`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);
 	}
-- 
2.43.0

Re: [PATCH] comedi: comedi_test: avoid AI scan timing overflow

[Greg Kroah-Hartman]

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

Re: [PATCH] comedi: comedi_test: avoid AI scan timing overflow

[Ian Abbott]

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 )=-

Re: [PATCH] comedi: comedi_test: avoid AI scan timing overflow

[Ian Abbott]

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 )=-

Re: [PATCH] comedi: comedi_test: avoid AI scan timing overflow

[Greg Kroah-Hartman]

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

Re: [PATCH] comedi: comedi_test: avoid AI scan timing overflow

[Ian Abbott]

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 )=-