[PATCH] iio: adc: ina2xx: avoid NULL pointer dereference on OF device match

[PATCH] iio: adc: ina2xx: avoid NULL pointer dereference on OF device match

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

The affected lines were resulting in a NULL pointer dereference on our
platform because the device tree contained the following list of
compatible strings:

    power-sensor@40 {
        compatible = "ti,ina232", "ti,ina231";
        ...
    };

Since the driver doesn't declare a compatible string "ti,ina232", the OF
matching succeeds on "ti,ina231". But the I2C device ID info is
populated via the first compatible string, cf. modalias population in
of_i2c_get_board_info(). Since there is no "ina232" entry in the legacy
I2C device ID table either, the struct i2c_device_id *id pointer in the
probe function is NULL.

Fix this by using the already populated type variable instead, which
points to the proper driver data. Since the name is also wanted, add a
generic one to the ina2xx_config table.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
Fixes: c43a102e67db ("iio: ina2xx: add support for TI INA2xx Power Monitors")
---
 drivers/iio/adc/ina2xx-adc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 213526c1592f..aea83f369437 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -124,6 +124,7 @@ static const struct regmap_config ina2xx_regmap_config = {
 enum ina2xx_ids { ina219, ina226 };
 
 struct ina2xx_config {
+	const char *name;
 	u16 config_default;
 	int calibration_value;
 	int shunt_voltage_lsb;	/* nV */
@@ -155,6 +156,7 @@ struct ina2xx_chip_info {
 
 static const struct ina2xx_config ina2xx_config[] = {
 	[ina219] = {
+		.name = "ina219",
 		.config_default = INA219_CONFIG_DEFAULT,
 		.calibration_value = 4096,
 		.shunt_voltage_lsb = 10000,
@@ -164,6 +166,7 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.chip_id = ina219,
 	},
 	[ina226] = {
+		.name = "ina226",
 		.config_default = INA226_CONFIG_DEFAULT,
 		.calibration_value = 2048,
 		.shunt_voltage_lsb = 2500,
@@ -996,7 +999,7 @@ static int ina2xx_probe(struct i2c_client *client)
 	/* Patch the current config register with default. */
 	val = chip->config->config_default;
 
-	if (id->driver_data == ina226) {
+	if (type == ina226) {
 		ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
 		ina226_set_int_time_vbus(chip, INA226_DEFAULT_IT, &val);
 		ina226_set_int_time_vshunt(chip, INA226_DEFAULT_IT, &val);
@@ -1015,7 +1018,7 @@ static int ina2xx_probe(struct i2c_client *client)
 	}
 
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	if (id->driver_data == ina226) {
+	if (type == ina226) {
 		indio_dev->channels = ina226_channels;
 		indio_dev->num_channels = ARRAY_SIZE(ina226_channels);
 		indio_dev->info = &ina226_info;
@@ -1024,7 +1027,7 @@ static int ina2xx_probe(struct i2c_client *client)
 		indio_dev->num_channels = ARRAY_SIZE(ina219_channels);
 		indio_dev->info = &ina219_info;
 	}
-	indio_dev->name = id->name;
+	indio_dev->name = id ? id->name : chip->config->name;
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
 					  &ina2xx_setup_ops);
-- 
2.40.0

Re: [PATCH] iio: adc: ina2xx: avoid NULL pointer dereference on OF device match

[Jonathan Cameron]

On Mon, 19 Jun 2023 16:12:39 +0200
Alvin Šipraga <alvin@pqrs.dk> wrote:

> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> The affected lines were resulting in a NULL pointer dereference on our
> platform because the device tree contained the following list of
> compatible strings:
> 
>     power-sensor@40 {
>         compatible = "ti,ina232", "ti,ina231";
>         ...
>     };
> 
> Since the driver doesn't declare a compatible string "ti,ina232", the OF
> matching succeeds on "ti,ina231". But the I2C device ID info is
> populated via the first compatible string, cf. modalias population in
> of_i2c_get_board_info(). Since there is no "ina232" entry in the legacy
> I2C device ID table either, the struct i2c_device_id *id pointer in the
> probe function is NULL.
> 
> Fix this by using the already populated type variable instead, which
> points to the proper driver data. Since the name is also wanted, add a
> generic one to the ina2xx_config table.
> 
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> Fixes: c43a102e67db ("iio: ina2xx: add support for TI INA2xx Power Monitors")

Ouch.  We've cleaned a bunch of these up over the years but I guess this
one got through because it did define the data in both id tables so we didn't
notice it still assumed id was set.

I'd forgotten this was one of the rare drivers where we have a driver in both
hwmon and IIO for the same devices.  So any binding update to include the new
part (making the fallback compatible explicit as well) would need to be in the
binding under hwmon.

Applied this as a fix to the fixes-togreg branch of iio.git and marked it for
stable.

Thanks,

Jonathan


> ---
>  drivers/iio/adc/ina2xx-adc.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 213526c1592f..aea83f369437 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -124,6 +124,7 @@ static const struct regmap_config ina2xx_regmap_config = {
>  enum ina2xx_ids { ina219, ina226 };
>  
>  struct ina2xx_config {
> +	const char *name;
>  	u16 config_default;
>  	int calibration_value;
>  	int shunt_voltage_lsb;	/* nV */
> @@ -155,6 +156,7 @@ struct ina2xx_chip_info {
>  
>  static const struct ina2xx_config ina2xx_config[] = {
>  	[ina219] = {
> +		.name = "ina219",
>  		.config_default = INA219_CONFIG_DEFAULT,
>  		.calibration_value = 4096,
>  		.shunt_voltage_lsb = 10000,
> @@ -164,6 +166,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>  		.chip_id = ina219,
>  	},
>  	[ina226] = {
> +		.name = "ina226",
>  		.config_default = INA226_CONFIG_DEFAULT,
>  		.calibration_value = 2048,
>  		.shunt_voltage_lsb = 2500,
> @@ -996,7 +999,7 @@ static int ina2xx_probe(struct i2c_client *client)
>  	/* Patch the current config register with default. */
>  	val = chip->config->config_default;
>  
> -	if (id->driver_data == ina226) {
> +	if (type == ina226) {
>  		ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
>  		ina226_set_int_time_vbus(chip, INA226_DEFAULT_IT, &val);
>  		ina226_set_int_time_vshunt(chip, INA226_DEFAULT_IT, &val);
> @@ -1015,7 +1018,7 @@ static int ina2xx_probe(struct i2c_client *client)
>  	}
>  
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> -	if (id->driver_data == ina226) {
> +	if (type == ina226) {
>  		indio_dev->channels = ina226_channels;
>  		indio_dev->num_channels = ARRAY_SIZE(ina226_channels);
>  		indio_dev->info = &ina226_info;
> @@ -1024,7 +1027,7 @@ static int ina2xx_probe(struct i2c_client *client)
>  		indio_dev->num_channels = ARRAY_SIZE(ina219_channels);
>  		indio_dev->info = &ina219_info;
>  	}
> -	indio_dev->name = id->name;
> +	indio_dev->name = id ? id->name : chip->config->name;
>  
>  	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
>  					  &ina2xx_setup_ops);