Re: [PATCH RFC v4 08/10] iio: frequency: ad9910: show channel priority in debugfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing `.open` callback in debugfs `file_operations` leads to a guaranteed NULL pointer dereference.
- [High] Function prototype mismatch in `file_operations` `.read` callbacks causes build failures and CFI panics.
- [Medium] Register bitmasks are applied to the wrong control registers, leading to corrupted state evaluation.
- [Low] Using `simple_read_from_buffer` with dynamically evaluated string pointers creates a TOCTOU race condition.
- [Low] The strings exposed through debugfs lack a trailing newline.
--

commit f3ad86f1b54a4f67b1e1ded4fa3bd327c17e7ba9
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

iio: frequency: ad9910: show channel priority in debugfs

Expose frequency_source, phase_source and amplitude_source attributes in
debugfs to indicate which channel the specific DDS parameter is being sourced
from.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index 1fdbaba356d73..d8fe88259f22d 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c
> @@ -2082,6 +2082,170 @@ static int ad9910_setup(struct device *dev, struct ad9910_state *st,
[ ... ]
> +	/* Parallel data port enabled and data destination is frequency */
> +	mode_en = FIELD_GET(AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK,
> +			    st->reg[AD9910_REG_CFR1].val32);

Does this apply a CFR2 mask to a CFR1 register? It seems
AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK should be evaluated against
st->reg[AD9910_REG_CFR2].

> +	if (mode_en) /* TODO: get destination from backend once it is supported */
> +		return ad9910_channel_str[AD9910_CHAN_IDX_PARALLEL_PORT];
[ ... ]
> +static int ad9910_frequency_source_show(struct file *file, char __user *userbuf,
> +					size_t count, loff_t *ppos)

Does the .read callback in struct file_operations require a return type of
ssize_t instead of int? Could this mismatch cause build failures or
Control Flow Integrity (CFI) kernel panics?

> +{
> +	struct iio_dev *indio_dev = file->private_data;

Without an .open handler like simple_open(), the VFS and debugfs proxy
layers leave file->private_data initialized to NULL. Does this lead to a NULL
pointer dereference when iio_priv() extracts the state from indio_dev?

> +	const char *src = ad9910_frequency_source_get(indio_dev);

Do the static strings returned here lack a trailing newline (\n)? Reading
this file might overwrite or corrupt the user's shell prompt.

> +
> +	return simple_read_from_buffer(userbuf, count, ppos, src, strlen(src));

If userspace performs a partial read and the hardware state changes before
the next read(), could ad9910_frequency_source_get() return a different,
shorter string?

If this occurs, simple_read_from_buffer() would apply the old *ppos to the
new string, potentially returning garbage. Would using seq_file
via DEFINE_SHOW_ATTRIBUTE() safely cache the output buffer upon open()?

> +}
> +
> +static inline const char *ad9910_phase_source_get(struct iio_dev *indio_dev)
> +{
[ ... ]
> +	/* Parallel data port enabled and data destination is phase */
> +	mode_en = FIELD_GET(AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK,
> +			    st->reg[AD9910_REG_CFR1].val32);

Is this another case where a CFR2 mask is applied to a CFR1 register?

> +	if (mode_en) /* TODO: get destination from backend once it is supported */
> +		return ad9910_channel_str[AD9910_CHAN_IDX_PARALLEL_PORT];
[ ... ]
> +static int ad9910_phase_source_show(struct file *file, char __user *userbuf,
> +				    size_t count, loff_t *ppos)

Could this return type also be ssize_t instead of int to match the .read
callback signature?

> +{
> +	struct iio_dev *indio_dev = file->private_data;

Does this have the same NULL pointer dereference issue as the frequency show
function due to missing simple_open()?

> +	const char *src = ad9910_phase_source_get(indio_dev);
> +
> +	return simple_read_from_buffer(userbuf, count, ppos, src, strlen(src));
> +}
> +
> +static inline const char *ad9910_amplitude_source_get(struct iio_dev *indio_dev)
> +{
> +	struct ad9910_state *st = iio_priv(indio_dev);
> +	bool ram_en, mode_en;
> +	u32 destination;
> +
> +	guard(mutex)(&st->lock);
> +
> +	/* OSK enabled */
> +	mode_en = FIELD_GET(AD9910_CFR1_OSK_ENABLE_MSK,
> +			    st->reg[AD9910_REG_CFR2].val32);

Does this apply a CFR1 mask to a CFR2 register?

> +	if (mode_en)
> +		return ad9910_channel_str[AD9910_CHAN_IDX_OSK];
> +
[ ... ]
> +	/* Parallel data port enabled and data destination is phase */
> +	mode_en = FIELD_GET(AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK,
> +			    st->reg[AD9910_REG_CFR1].val32);

Is this a third instance of applying a CFR2 mask to a CFR1 register?

> +	if (mode_en) /* TODO: get destination from backend once it is supported */
> +		return ad9910_channel_str[AD9910_CHAN_IDX_PARALLEL_PORT];
[ ... ]
> +static int ad9910_amplitude_source_show(struct file *file, char __user *userbuf,
> +					size_t count, loff_t *ppos)

Could this return type also be ssize_t instead of int?

> +{
> +	struct iio_dev *indio_dev = file->private_data;

Does this have the same NULL pointer dereference issue as the other show
functions?

> +	const char *src = ad9910_amplitude_source_get(indio_dev);
> +
> +	return simple_read_from_buffer(userbuf, count, ppos, src, strlen(src));
> +}
> +
> +static const struct file_operations ad9910_frequency_source_fops = {
> +	.owner = THIS_MODULE,
> +	.read = ad9910_frequency_source_show,
> +};

Should this struct include an .open callback, such as simple_open(), to ensure
that file->private_data is initialized correctly before .read is called?

> +
> +static const struct file_operations ad9910_phase_source_fops = {
> +	.owner = THIS_MODULE,
> +	.read = ad9910_phase_source_show,
> +};
> +
> +static const struct file_operations ad9910_amplitude_source_fops = {
> +	.owner = THIS_MODULE,
> +	.read = ad9910_amplitude_source_show,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-ad9910-iio-driver-v4-0-d26bfd20ee3d@analog.com?part=8