Re: [PATCH v8 3/5] iio: adc: ltc2378: Enable high-speed data capture

On 07/23, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Division by zero vulnerabilities in ltc2378_update_conversion_rate due to unhandled hardware rounding of period/frequency.
> - [Low] Unlocked data race on st->cnv_Hz between read_raw and write_raw.
> --
> 
> --- Patch [3]: [PATCH v8 3/5] iio: adc: ltc2378: Enable high-speed data capture ---
> commit cad96004cef5739b24db063923b36678ea8e3617
> Author: Marcelo Schmitt <[email protected]>
> Subject: iio: adc: ltc2378: Enable high-speed data capture
> 
> Make use of SPI transfer offloading to speed up data capture, enabling data
> acquisition at faster sample rates (up to 2 MSPS).
> 
> > diff --git a/drivers/iio/adc/ltc2378.c b/drivers/iio/adc/ltc2378.c
> > index 107388f870626..7baee02450d39 100644
> > --- a/drivers/iio/adc/ltc2378.c
> > +++ b/drivers/iio/adc/ltc2378.c
> > @@ -264,7 +384,137 @@ static int ltc2378_read_raw(struct iio_dev *indio_dev,
> >  			*val2 = chan->scan_type.realbits;
> >  
> >  		return IIO_VAL_FRACTIONAL_LOG2;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		*val = st->cnv_Hz;
> 
> [Severity: Low]
> Does this lockless read of st->cnv_Hz race with concurrent updates?
> 
Not a big deal, IMHO.

> While ltc2378_write_raw() acquires the IIO direct mode lock before calling
> ltc2378_update_conversion_rate() to update st->cnv_Hz, this read path appears
> to access it without synchronization or READ_ONCE()/WRITE_ONCE() macros.
> 
> > +		return IIO_VAL_INT;
> 
> [ ... ]
> 
> > +static int ltc2378_update_conversion_rate(struct ltc2378_state *st, int freq_Hz)
> > +{
> 
> [ ... ]
> 
> > +	/*
> > +	 * Configure SPI offload PWM trigger.
> > +	 * The trigger should fire after tBUSYLH + tCONV + tDSDOBUSYL.
> > +	 * Minimum time needed: TBUSYLH (13ns) + TCONV (part-specific) + TDSDOBUSYL (5ns)
> > +	 *
> > +	 * Use the same period as CNV PWM to avoid timing issues.
> > +	 * Convert back from period to frequency for the SPI offload API.
> > +	 */
> > +	offload_period_ns = cnv_wf.period_length_ns;
> > +	config.periodic.frequency_hz = div_u64(HZ_PER_GHZ, offload_period_ns);
> 
> [Severity: High]
> Can this result in a division by zero if offload_period_ns is 0?
> 
> If pwm_round_waveform_might_sleep() rounds the requested period down to 0,
> offload_period_ns will be 0 and cause a kernel panic here.
> 
> > +	min_read_offset = LTC2378_TBUSYLH_NS + st->info->tconv_ns + LTC2378_TDSDOBUSYL_NS;
> 
> [ ... ]
> 
> > +	/* Check the minimum CNV to SCLK delay is met */
> > +	if (config.periodic.offset_ns < min_read_offset)
> > +		return -EDOM;
> > +
> > +	/* Check the PWM periods remain the same */
> > +	offload_period_ns = div64_u64(HZ_PER_GHZ, config.periodic.frequency_hz);
> 
> [Severity: High]
> Does this code panic if config.periodic.frequency_hz evaluates to 0?
> 
> If a user writes a very low sampling frequency, the PWM driver might round
> the hardware period up to a value greater than HZ_PER_GHZ. If
> offload_period_ns > HZ_PER_GHZ, the earlier calculation for
> config.periodic.frequency_hz will evaluate to 0.
> 
> Additionally, if spi_offload_trigger_validate() mutates frequency_hz down
> to 0, it would also result in a division by zero here.
> 
> > +	if (cnv_wf.period_length_ns != offload_period_ns)
> > +		return -EDOM;
> 
Theory already refuted in previous version.
https://lore.kernel.org/linux-iio/alpZwoau3QVPtBsP@debian-BULLSEYE-live-builder-AMD64/