[PATCH v2] iio: frequency: admv1013: fix NULL pointer dereference on str

[PATCH v2] iio: frequency: admv1013: fix NULL pointer dereference on str

[Antoniu Miclaus]

When device_property_read_string() fails, str is left uninitialized
but the code falls through to strcmp(str, ...), dereferencing a garbage
pointer. Replace manual read/strcmp with
device_property_match_property_string() which reads the property as a
single string value and matches it against an array of known valid
strings, handling the missing property case internally.

Fixes: da35a7b526d9 ("iio: frequency: admv1013: add support for ADMV1013")
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
Changes in v2:
 - use device_property_match_property_string() instead of
   device_property_match_string() (Andy)
 - add Reviewed-by tag from Nuno

 drivers/iio/frequency/admv1013.c | 41 ++++++++++++++++----------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/frequency/admv1013.c b/drivers/iio/frequency/admv1013.c
index 9202443ef445..3eb0dc2f8dad 100644
--- a/drivers/iio/frequency/admv1013.c
+++ b/drivers/iio/frequency/admv1013.c
@@ -512,37 +512,36 @@ static void admv1013_powerdown(void *data)
 	admv1013_spi_update_bits(data, ADMV1013_REG_ENABLE, enable_reg_msk, enable_reg);
 }
 
+static const char * const admv1013_input_modes[] = { "iq", "if" };
+
+static const char * const admv1013_quad_se_modes[] = { "diff", "se-pos", "se-neg" };
+
 static int admv1013_properties_parse(struct admv1013_state *st)
 {
 	int ret;
-	const char *str;
 	struct device *dev = &st->spi->dev;
 
 	st->det_en = device_property_read_bool(dev, "adi,detector-enable");
 
-	ret = device_property_read_string(dev, "adi,input-mode", &str);
-	if (ret)
-		st->input_mode = ADMV1013_IQ_MODE;
-
-	if (!strcmp(str, "iq"))
-		st->input_mode = ADMV1013_IQ_MODE;
-	else if (!strcmp(str, "if"))
-		st->input_mode = ADMV1013_IF_MODE;
-	else
-		return -EINVAL;
-
-	ret = device_property_read_string(dev, "adi,quad-se-mode", &str);
-	if (ret)
-		st->quad_se_mode = ADMV1013_SE_MODE_DIFF;
+	ret = device_property_match_property_string(dev, "adi,input-mode",
+						    admv1013_input_modes,
+						    ARRAY_SIZE(admv1013_input_modes));
+	st->input_mode = ret >= 0 ? ret : ADMV1013_IQ_MODE;
 
-	if (!strcmp(str, "diff"))
-		st->quad_se_mode = ADMV1013_SE_MODE_DIFF;
-	else if (!strcmp(str, "se-pos"))
+	ret = device_property_match_property_string(dev, "adi,quad-se-mode",
+						    admv1013_quad_se_modes,
+						    ARRAY_SIZE(admv1013_quad_se_modes));
+	switch (ret) {
+	case 1:
 		st->quad_se_mode = ADMV1013_SE_MODE_POS;
-	else if (!strcmp(str, "se-neg"))
+		break;
+	case 2:
 		st->quad_se_mode = ADMV1013_SE_MODE_NEG;
-	else
-		return -EINVAL;
+		break;
+	default:
+		st->quad_se_mode = ADMV1013_SE_MODE_DIFF;
+		break;
+	}
 
 	ret = devm_regulator_bulk_get_enable(dev,
 					     ARRAY_SIZE(admv1013_vcc_regs),
-- 
2.43.0

Re: [PATCH v2] iio: frequency: admv1013: fix NULL pointer dereference on str

[Andy Shevchenko]

On Tue, Mar 03, 2026 at 11:52:28AM +0200, Antoniu Miclaus wrote:
> When device_property_read_string() fails, str is left uninitialized
> but the code falls through to strcmp(str, ...), dereferencing a garbage
> pointer. Replace manual read/strcmp with
> device_property_match_property_string() which reads the property as a
> single string value and matches it against an array of known valid
> strings, handling the missing property case internally.

...

> +static const char * const admv1013_input_modes[] = { "iq", "if" };

This array has to be indexed.

static const char * const admv1013_input_modes[] = {
	[ADMV1013_IQ_MODE] = "iq",
        [ADMV1013_IF_MODE] = "if",
};

...

> +static const char * const admv1013_quad_se_modes[] = { "diff", "se-pos", "se-neg" };

Also this one.
Taking into account the indices are not sequential, this may require another
enumerator.

Ideally you need to list all possible modes and choose only supported by
assigning an empty string to unsupported ones.

I haven't checked datasheet to understand why only 6, 9, 12 are in use.
Maybe they can be simply 1, 2, 3 with a formula like 3 + x * 3 ? Dunno.

...

> +	switch (ret) {
> +	case 1:
>  		st->quad_se_mode = ADMV1013_SE_MODE_POS;
> -	else if (!strcmp(str, "se-neg"))
> +		break;
> +	case 2:
>  		st->quad_se_mode = ADMV1013_SE_MODE_NEG;
> +		break;

This (the default) should take

	case 0:

as well

> +	default:
> +		st->quad_se_mode = ADMV1013_SE_MODE_DIFF;
> +		break;
> +	}

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2] iio: frequency: admv1013: fix NULL pointer dereference on str

[Miclaus, Antoniu]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@intel.com>
> Sent: Tuesday, March 3, 2026 1:34 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Jonathan Cameron <jic23@kernel.org>;
> David Lechner <dlechner@baylibre.com>; Sa, Nuno <Nuno.Sa@analog.com>;
> Andy Shevchenko <andy@kernel.org>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2] iio: frequency: admv1013: fix NULL pointer
> dereference on str
> 
> [External]
> 
> On Tue, Mar 03, 2026 at 11:52:28AM +0200, Antoniu Miclaus wrote:
> > When device_property_read_string() fails, str is left uninitialized
> > but the code falls through to strcmp(str, ...), dereferencing a garbage
> > pointer. Replace manual read/strcmp with
> > device_property_match_property_string() which reads the property as a
> > single string value and matches it against an array of known valid
> > strings, handling the missing property case internally.
> 
> ...
> 
> > +static const char * const admv1013_input_modes[] = { "iq", "if" };
> 
> This array has to be indexed.
> 
> static const char * const admv1013_input_modes[] = {
> 	[ADMV1013_IQ_MODE] = "iq",
>         [ADMV1013_IF_MODE] = "if",
> };
> 
> ...
> 
> > +static const char * const admv1013_quad_se_modes[] = { "diff", "se-pos",
> "se-neg" };
> 
> Also this one.
> Taking into account the indices are not sequential, this may require another
> enumerator.
> 
> Ideally you need to list all possible modes and choose only supported by
> assigning an empty string to unsupported ones.
> 
> I haven't checked datasheet to understand why only 6, 9, 12 are in use.
> Maybe they can be simply 1, 2, 3 with a formula like 3 + x * 3 ? Dunno.
> 

static const char * const admv1013_input_modes[] = {
       [ADMV1013_IQ_MODE] = "iq",
       [ADMV1013_IF_MODE] = "if",
};

static const char * const admv1013_quad_se_modes[] = {
       [ADMV1013_QUAD_SE_DIFF] = "diff",
       [ADMV1013_QUAD_SE_POS] = "se-pos",
       [ADMV1013_QUAD_SE_NEG] = "se-neg",
};

static const unsigned int admv1013_quad_se_regvals[] = {
       [ADMV1013_QUAD_SE_DIFF] = ADMV1013_SE_MODE_DIFF,
       [ADMV1013_QUAD_SE_POS] = ADMV1013_SE_MODE_POS,
       [ADMV1013_QUAD_SE_NEG] = ADMV1013_SE_MODE_NEG,
};

Does this make sense?

> ...
> 
> > +	switch (ret) {
> > +	case 1:
> >  		st->quad_se_mode = ADMV1013_SE_MODE_POS;
> > -	else if (!strcmp(str, "se-neg"))
> > +		break;
> > +	case 2:
> >  		st->quad_se_mode = ADMV1013_SE_MODE_NEG;
> > +		break;
> 
> This (the default) should take
> 
> 	case 0:
> 
> as well
> 
> > +	default:
> > +		st->quad_se_mode = ADMV1013_SE_MODE_DIFF;
> > +		break;
> > +	}
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v2] iio: frequency: admv1013: fix NULL pointer dereference on str

[Andy Shevchenko]

On Tue, Mar 03, 2026 at 12:01:31PM +0000, Miclaus, Antoniu wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > Sent: Tuesday, March 3, 2026 1:34 PM
> > On Tue, Mar 03, 2026 at 11:52:28AM +0200, Antoniu Miclaus wrote:

...

> > > +static const char * const admv1013_quad_se_modes[] = { "diff", "se-pos",
> > "se-neg" };
> > 
> > Taking into account the indices are not sequential, this may require another
> > enumerator.
> > 
> > Ideally you need to list all possible modes and choose only supported by
> > assigning an empty string to unsupported ones.
> > 
> > I haven't checked datasheet to understand why only 6, 9, 12 are in use.
> > Maybe they can be simply 1, 2, 3 with a formula like 3 + x * 3 ? Dunno.
> > 
> 
> static const char * const admv1013_input_modes[] = {
>        [ADMV1013_IQ_MODE] = "iq",
>        [ADMV1013_IF_MODE] = "if",
> };
> 
> static const char * const admv1013_quad_se_modes[] = {
>        [ADMV1013_QUAD_SE_DIFF] = "diff",
>        [ADMV1013_QUAD_SE_POS] = "se-pos",
>        [ADMV1013_QUAD_SE_NEG] = "se-neg",
> };
> 
> static const unsigned int admv1013_quad_se_regvals[] = {
>        [ADMV1013_QUAD_SE_DIFF] = ADMV1013_SE_MODE_DIFF,
>        [ADMV1013_QUAD_SE_POS] = ADMV1013_SE_MODE_POS,
>        [ADMV1013_QUAD_SE_NEG] = ADMV1013_SE_MODE_NEG,
> };
> 
> Does this make sense?

Yes, if there is an explanation why we have 6,9,12 to begin with. Can somebody
study the available datasheets and come up with the explanations?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2] iio: frequency: admv1013: fix NULL pointer dereference on str

[Miclaus, Antoniu]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@intel.com>
> Sent: Tuesday, March 3, 2026 3:55 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Jonathan Cameron <jic23@kernel.org>;
> David Lechner <dlechner@baylibre.com>; Sa, Nuno <Nuno.Sa@analog.com>;
> Andy Shevchenko <andy@kernel.org>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2] iio: frequency: admv1013: fix NULL pointer
> dereference on str
> 
> [External]
> 
> On Tue, Mar 03, 2026 at 12:01:31PM +0000, Miclaus, Antoniu wrote:
> > > -----Original Message-----
> > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > Sent: Tuesday, March 3, 2026 1:34 PM
> > > On Tue, Mar 03, 2026 at 11:52:28AM +0200, Antoniu Miclaus wrote:
> 
> ...
> 
> > > > +static const char * const admv1013_quad_se_modes[] = { "diff", "se-
> pos",
> > > "se-neg" };
> > >
> > > Taking into account the indices are not sequential, this may require another
> > > enumerator.
> > >
> > > Ideally you need to list all possible modes and choose only supported by
> > > assigning an empty string to unsupported ones.
> > >
> > > I haven't checked datasheet to understand why only 6, 9, 12 are in use.
> > > Maybe they can be simply 1, 2, 3 with a formula like 3 + x * 3 ? Dunno.
> > >
> >
> > static const char * const admv1013_input_modes[] = {
> >        [ADMV1013_IQ_MODE] = "iq",
> >        [ADMV1013_IF_MODE] = "if",
> > };
> >
> > static const char * const admv1013_quad_se_modes[] = {
> >        [ADMV1013_QUAD_SE_DIFF] = "diff",
> >        [ADMV1013_QUAD_SE_POS] = "se-pos",
> >        [ADMV1013_QUAD_SE_NEG] = "se-neg",
> > };
> >
> > static const unsigned int admv1013_quad_se_regvals[] = {
> >        [ADMV1013_QUAD_SE_DIFF] = ADMV1013_SE_MODE_DIFF,
> >        [ADMV1013_QUAD_SE_POS] = ADMV1013_SE_MODE_POS,
> >        [ADMV1013_QUAD_SE_NEG] = ADMV1013_SE_MODE_NEG,
> > };
> >
> > Does this make sense?
> 
> Yes, if there is an explanation why we have 6,9,12 to begin with. Can
> somebody
> study the available datasheets and come up with the explanations?

There are the values accepted by the QUAD_SE_MODE bitfield.

https://www.analog.com/media/en/technical-documentation/data-sheets/admv1013.pdf
Page 37 of 39, Table 15.  

> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v2] iio: frequency: admv1013: fix NULL pointer dereference on str

[Andy Shevchenko]

On Wed, Mar 04, 2026 at 09:52:46AM +0000, Miclaus, Antoniu wrote:
> > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > Sent: Tuesday, March 3, 2026 3:55 PM
> > On Tue, Mar 03, 2026 at 12:01:31PM +0000, Miclaus, Antoniu wrote:
> > > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > > Sent: Tuesday, March 3, 2026 1:34 PM
> > > > On Tue, Mar 03, 2026 at 11:52:28AM +0200, Antoniu Miclaus wrote:

...

> > > > > +static const char * const admv1013_quad_se_modes[] = { "diff", "se-
> > pos",
> > > > "se-neg" };
> > > >
> > > > Taking into account the indices are not sequential, this may require another
> > > > enumerator.
> > > >
> > > > Ideally you need to list all possible modes and choose only supported by
> > > > assigning an empty string to unsupported ones.
> > > >
> > > > I haven't checked datasheet to understand why only 6, 9, 12 are in use.
> > > > Maybe they can be simply 1, 2, 3 with a formula like 3 + x * 3 ? Dunno.
> > > >
> > >
> > > static const char * const admv1013_input_modes[] = {
> > >        [ADMV1013_IQ_MODE] = "iq",
> > >        [ADMV1013_IF_MODE] = "if",
> > > };
> > >
> > > static const char * const admv1013_quad_se_modes[] = {
> > >        [ADMV1013_QUAD_SE_DIFF] = "diff",
> > >        [ADMV1013_QUAD_SE_POS] = "se-pos",
> > >        [ADMV1013_QUAD_SE_NEG] = "se-neg",
> > > };
> > >
> > > static const unsigned int admv1013_quad_se_regvals[] = {
> > >        [ADMV1013_QUAD_SE_DIFF] = ADMV1013_SE_MODE_DIFF,
> > >        [ADMV1013_QUAD_SE_POS] = ADMV1013_SE_MODE_POS,
> > >        [ADMV1013_QUAD_SE_NEG] = ADMV1013_SE_MODE_NEG,
> > > };
> > >
> > > Does this make sense?
> > 
> > Yes, if there is an explanation why we have 6,9,12 to begin with. Can
> > somebody
> > study the available datasheets and come up with the explanations?
> 
> There are the values accepted by the QUAD_SE_MODE bitfield.
> 
> https://www.analog.com/media/en/technical-documentation/data-sheets/admv1013.pdf
> Page 37 of 39, Table 15.  

Thanks!

So, looking at the code and the datasheet I think we need to do the following.

Apply the rule that each HW related enum has to be defined with explicit
value. But looking at the code, I think we need to do exactly the opposite,
id est treat all enums as Linux driver ones. With this

1. Drop values from the given enum

  enum {
        ADMV1013_SE_MODE_POS,
        ADMV1013_SE_MODE_NEG,
        ADMV1013_SE_MODE_DIFF,
  };

2. Use it directly as indices for string arrays.
3. Use plain numbers in the mapping switch when assigning the values in
the similar way like it's done for other registers in other functions,
exempli gratia quad filters.

This will make consistent code among different bitfields used in the driver
and makes easier to use string arrays without double enums, et cetera.

-- 
With Best Regards,
Andy Shevchenko