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

[PATCH v4] 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() and consolidate the SE mode
enums into a single sequential enum, mapping to hardware register
values via a switch consistent with other bitfields in the driver.

Fixes: da35a7b526d9 ("iio: frequency: admv1013: add support for ADMV1013")
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
Changes in v4:
 - consolidate SE mode enums into a single sequential enum, removing the
   separate ADMV1013_QUAD_SE_* enum and regvals lookup table
 - update commit message accordingly

 drivers/iio/frequency/admv1013.c | 65 ++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/frequency/admv1013.c b/drivers/iio/frequency/admv1013.c
index 9202443ef445..b852378b3f68 100644
--- a/drivers/iio/frequency/admv1013.c
+++ b/drivers/iio/frequency/admv1013.c
@@ -85,9 +85,9 @@ enum {
 };
 
 enum {
-	ADMV1013_SE_MODE_POS = 6,
-	ADMV1013_SE_MODE_NEG = 9,
-	ADMV1013_SE_MODE_DIFF = 12
+	ADMV1013_SE_MODE_POS,
+	ADMV1013_SE_MODE_NEG,
+	ADMV1013_SE_MODE_DIFF,
 };
 
 struct admv1013_state {
@@ -468,10 +468,23 @@ static int admv1013_init(struct admv1013_state *st, int vcm_uv)
 	if (ret)
 		return ret;
 
-	data = FIELD_PREP(ADMV1013_QUAD_SE_MODE_MSK, st->quad_se_mode);
+	switch (st->quad_se_mode) {
+	case ADMV1013_SE_MODE_POS:
+		data = 6;
+		break;
+	case ADMV1013_SE_MODE_NEG:
+		data = 9;
+		break;
+	case ADMV1013_SE_MODE_DIFF:
+		data = 12;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	ret = __admv1013_spi_update_bits(st, ADMV1013_REG_QUAD,
-					 ADMV1013_QUAD_SE_MODE_MSK, data);
+					 ADMV1013_QUAD_SE_MODE_MSK,
+					 FIELD_PREP(ADMV1013_QUAD_SE_MODE_MSK, data));
 	if (ret)
 		return ret;
 
@@ -512,37 +525,33 @@ 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[] = {
+	[ADMV1013_IQ_MODE] = "iq",
+	[ADMV1013_IF_MODE] = "if",
+};
+
+static const char * const admv1013_quad_se_modes[] = {
+	[ADMV1013_SE_MODE_POS] = "se-pos",
+	[ADMV1013_SE_MODE_NEG] = "se-neg",
+	[ADMV1013_SE_MODE_DIFF] = "diff",
+};
+
 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_match_property_string(dev, "adi,input-mode",
+						    admv1013_input_modes,
+						    ARRAY_SIZE(admv1013_input_modes));
+	st->input_mode = ret >= 0 ? ret : ADMV1013_IQ_MODE;
 
-	ret = device_property_read_string(dev, "adi,quad-se-mode", &str);
-	if (ret)
-		st->quad_se_mode = ADMV1013_SE_MODE_DIFF;
-
-	if (!strcmp(str, "diff"))
-		st->quad_se_mode = ADMV1013_SE_MODE_DIFF;
-	else if (!strcmp(str, "se-pos"))
-		st->quad_se_mode = ADMV1013_SE_MODE_POS;
-	else if (!strcmp(str, "se-neg"))
-		st->quad_se_mode = ADMV1013_SE_MODE_NEG;
-	else
-		return -EINVAL;
+	ret = device_property_match_property_string(dev, "adi,quad-se-mode",
+						    admv1013_quad_se_modes,
+						    ARRAY_SIZE(admv1013_quad_se_modes));
+	st->quad_se_mode = ret >= 0 ? ret : ADMV1013_SE_MODE_DIFF;
 
 	ret = devm_regulator_bulk_get_enable(dev,
 					     ARRAY_SIZE(admv1013_vcc_regs),
-- 
2.43.0

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

[Andy Shevchenko]

On Thu, Mar 5, 2026 at 11:15 AM Antoniu Miclaus
<antoniu.miclaus@analog.com> 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() and consolidate the SE mode
> enums into a single sequential enum, mapping to hardware register
> values via a switch consistent with other bitfields in the driver.

...

> static int admv1013_init(struct admv1013_state *st, int vcm_uv)

> -       data = FIELD_PREP(ADMV1013_QUAD_SE_MODE_MSK, st->quad_se_mode);
> +       switch (st->quad_se_mode) {
> +       case ADMV1013_SE_MODE_POS:
> +               data = 6;
> +               break;
> +       case ADMV1013_SE_MODE_NEG:
> +               data = 9;
> +               break;
> +       case ADMV1013_SE_MODE_DIFF:
> +               data = 12;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }

Oh, I haven't realised that it can be done like this. I have in mind
just to have a switch in the probe, so we don't need to do that again
and again. Otherwise looks good to me.

-- 
With Best Regards,
Andy Shevchenko

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

[Miclaus, Antoniu]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Thursday, March 5, 2026 11:36 AM
> 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 v4] iio: frequency: admv1013: fix NULL pointer
> dereference on str
> 
> [External]
> 
> On Thu, Mar 5, 2026 at 11:15 AM Antoniu Miclaus
> <antoniu.miclaus@analog.com> 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() and consolidate the SE mode
> > enums into a single sequential enum, mapping to hardware register
> > values via a switch consistent with other bitfields in the driver.
> 
> ...
> 
> > static int admv1013_init(struct admv1013_state *st, int vcm_uv)
> 
> > -       data = FIELD_PREP(ADMV1013_QUAD_SE_MODE_MSK, st-
> >quad_se_mode);
> > +       switch (st->quad_se_mode) {
> > +       case ADMV1013_SE_MODE_POS:
> > +               data = 6;
> > +               break;
> > +       case ADMV1013_SE_MODE_NEG:
> > +               data = 9;
> > +               break;
> > +       case ADMV1013_SE_MODE_DIFF:
> > +               data = 12;
> > +               break;
> > +       default:
> > +               return -EINVAL;
> > +       }
> 
> Oh, I haven't realised that it can be done like this. I have in mind
> just to have a switch in the probe, so we don't need to do that again
> and again. Otherwise looks good to me.
>

Hmm, the _init() function is called only in the probe.
I think it makes a bit more sense to be here where we handle all the initial register configurations.

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

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

[Andy Shevchenko]

On Thu, Mar 05, 2026 at 09:44:35AM +0000, Miclaus, Antoniu wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Thursday, March 5, 2026 11:36 AM
> > On Thu, Mar 5, 2026 at 11:15 AM Antoniu Miclaus
> > <antoniu.miclaus@analog.com> wrote:

...

> > > static int admv1013_init(struct admv1013_state *st, int vcm_uv)
> > 
> > > -       data = FIELD_PREP(ADMV1013_QUAD_SE_MODE_MSK, st-
> > >quad_se_mode);
> > > +       switch (st->quad_se_mode) {
> > > +       case ADMV1013_SE_MODE_POS:
> > > +               data = 6;
> > > +               break;
> > > +       case ADMV1013_SE_MODE_NEG:
> > > +               data = 9;
> > > +               break;
> > > +       case ADMV1013_SE_MODE_DIFF:
> > > +               data = 12;
> > > +               break;
> > > +       default:
> > > +               return -EINVAL;
> > > +       }
> > 
> > Oh, I haven't realised that it can be done like this. I have in mind
> > just to have a switch in the probe, so we don't need to do that again
> > and again. Otherwise looks good to me.
> 
> Hmm, the _init() function is called only in the probe.

Ah, missed that. Then we are good.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

> I think it makes a bit more sense to be here where we handle all the initial
> register configurations.

Agree.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Thu, 5 Mar 2026 11:51:51 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Mar 05, 2026 at 09:44:35AM +0000, Miclaus, Antoniu wrote:
> > > -----Original Message-----
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Thursday, March 5, 2026 11:36 AM
> > > On Thu, Mar 5, 2026 at 11:15 AM Antoniu Miclaus
> > > <antoniu.miclaus@analog.com> wrote:  
> 
> ...
> 
> > > > static int admv1013_init(struct admv1013_state *st, int vcm_uv)  
> > >   
> > > > -       data = FIELD_PREP(ADMV1013_QUAD_SE_MODE_MSK, st-
> > > >quad_se_mode);
> > > > +       switch (st->quad_se_mode) {
> > > > +       case ADMV1013_SE_MODE_POS:
> > > > +               data = 6;
> > > > +               break;
> > > > +       case ADMV1013_SE_MODE_NEG:
> > > > +               data = 9;
> > > > +               break;
> > > > +       case ADMV1013_SE_MODE_DIFF:
> > > > +               data = 12;
> > > > +               break;
> > > > +       default:
> > > > +               return -EINVAL;
> > > > +       }  
> > > 
> > > Oh, I haven't realised that it can be done like this. I have in mind
> > > just to have a switch in the probe, so we don't need to do that again
> > > and again. Otherwise looks good to me.  
> > 
> > Hmm, the _init() function is called only in the probe.  
> 
> Ah, missed that. Then we are good.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

Unfortunately we have some cleanup quite a way back in my tree for this cycle
and I'm not keen to rebase to drop it all.  So we'll need to manually back port
this after it merges in the next merge window.

If we need to do it more urgently I'll need a patch against togreg-fixes. Let me know
if you think this is necessary.  For now I've applied it on the testing branch.

Jonathan

> 
> > I think it makes a bit more sense to be here where we handle all the initial
> > register configurations.  
> 
> Agree.
>