[PATCH] mfd: tps6105x: Fix NULL pointer access.

[PATCH] mfd: tps6105x: Fix NULL pointer access.

[Grigoryev Denis]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2565 bytes --]

When tps6105x used in TPS6105X_MODE_SHUTDOWN mode the driver calls
mfd_add_devices() with mfd_cell->name == NULL, that causes an ooops in
platform_device_register() later.

This patch reorders mfd_cell structures and makes code to call
mfd_add_devices() with proper number of cells.

Signed-off-by: Denis Grigoryev <grigoryev@spb.prosoft.ru>
---
 drivers/mfd/tps6105x.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/tps6105x.c b/drivers/mfd/tps6105x.c
index 5de95c2..be2c286 100644
--- a/drivers/mfd/tps6105x.c
+++ b/drivers/mfd/tps6105x.c
@@ -124,11 +124,11 @@ static int tps6105x_startup(struct tps6105x *tps6105x)
  */
 static struct mfd_cell tps6105x_cells[] = {
 	{
-		/* name will be runtime assigned */
+		.name = "tps6105x-gpio",
 		.id = -1,
 	},
 	{
-		.name = "tps6105x-gpio",
+		/* name will be runtime assigned */
 		.id = -1,
 	},
 };
@@ -138,6 +138,7 @@ static int tps6105x_probe(struct i2c_client *client,
 {
 	struct tps6105x			*tps6105x;
 	struct tps6105x_platform_data	*pdata;
+	int n_cells = ARRAY_SIZE(tps6105x_cells);
 	int ret;
 	int i;
 
@@ -162,33 +163,34 @@ static int tps6105x_probe(struct i2c_client *client,
 	case TPS6105X_MODE_SHUTDOWN:
 		dev_info(&client->dev,
 			 "present, not used for anything, only GPIO\n");
+		n_cells = 1;
 		break;
 	case TPS6105X_MODE_TORCH:
-		tps6105x_cells[0].name = "tps6105x-leds";
+		tps6105x_cells[1].name = "tps6105x-leds";
 		dev_warn(&client->dev,
 			 "torch mode is unsupported\n");
 		break;
 	case TPS6105X_MODE_TORCH_FLASH:
-		tps6105x_cells[0].name = "tps6105x-flash";
+		tps6105x_cells[1].name = "tps6105x-flash";
 		dev_warn(&client->dev,
 			 "flash mode is unsupported\n");
 		break;
 	case TPS6105X_MODE_VOLTAGE:
-		tps6105x_cells[0].name ="tps6105x-regulator";
+		tps6105x_cells[1].name = "tps6105x-regulator";
 		break;
 	default:
 		break;
 	}
 
 	/* Set up and register the platform devices. */
-	for (i = 0; i < ARRAY_SIZE(tps6105x_cells); i++) {
+	for (i = 0; i < n_cells; i++) {
 		/* One state holder for all drivers, this is simple */
 		tps6105x_cells[i].platform_data = tps6105x;
 		tps6105x_cells[i].pdata_size = sizeof(*tps6105x);
 	}
 
 	return mfd_add_devices(&client->dev, 0, tps6105x_cells,
-			       ARRAY_SIZE(tps6105x_cells), NULL, 0, NULL);
+			       n_cells, NULL, 0, NULL);
 }
 
 static int tps6105x_remove(struct i2c_client *client)
-- 
1.7.10.4
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] mfd: tps6105x: Fix NULL pointer access.

[Lee Jones]

On Fri, 04 Sep 2015, Grigoryev Denis wrote:

> When tps6105x used in TPS6105X_MODE_SHUTDOWN mode the driver calls
> mfd_add_devices() with mfd_cell->name == NULL, that causes an ooops in
> platform_device_register() later.
> 
> This patch reorders mfd_cell structures and makes code to call
> mfd_add_devices() with proper number of cells.
> 
> Signed-off-by: Denis Grigoryev <grigoryev@spb.prosoft.ru>
> ---
>  drivers/mfd/tps6105x.c |   16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mfd/tps6105x.c b/drivers/mfd/tps6105x.c
> index 5de95c2..be2c286 100644
> --- a/drivers/mfd/tps6105x.c
> +++ b/drivers/mfd/tps6105x.c
> @@ -124,11 +124,11 @@ static int tps6105x_startup(struct tps6105x *tps6105x)
>   */
>  static struct mfd_cell tps6105x_cells[] = {
>  	{
> -		/* name will be runtime assigned */
> +		.name = "tps6105x-gpio",
>  		.id = -1,
>  	},
>  	{
> -		.name = "tps6105x-gpio",
> +		/* name will be runtime assigned */
>  		.id = -1,
>  	},
>  };

So you have 2 cells with identical .name's and identical .id's.

How does that work?

> @@ -138,6 +138,7 @@ static int tps6105x_probe(struct i2c_client *client,
>  {
>  	struct tps6105x			*tps6105x;
>  	struct tps6105x_platform_data	*pdata;
> +	int n_cells = ARRAY_SIZE(tps6105x_cells);
>  	int ret;
>  	int i;
>  
> @@ -162,33 +163,34 @@ static int tps6105x_probe(struct i2c_client *client,
>  	case TPS6105X_MODE_SHUTDOWN:
>  		dev_info(&client->dev,
>  			 "present, not used for anything, only GPIO\n");
> +		n_cells = 1;
>  		break;
>  	case TPS6105X_MODE_TORCH:
> -		tps6105x_cells[0].name = "tps6105x-leds";
> +		tps6105x_cells[1].name = "tps6105x-leds";

Now you're over-writing cell names!  I'd say this was a hack.

Please just create an entry for each device, like usual.

>  		dev_warn(&client->dev,
>  			 "torch mode is unsupported\n");
>  		break;
>  	case TPS6105X_MODE_TORCH_FLASH:
> -		tps6105x_cells[0].name = "tps6105x-flash";
> +		tps6105x_cells[1].name = "tps6105x-flash";
>  		dev_warn(&client->dev,
>  			 "flash mode is unsupported\n");
>  		break;
>  	case TPS6105X_MODE_VOLTAGE:
> -		tps6105x_cells[0].name ="tps6105x-regulator";
> +		tps6105x_cells[1].name = "tps6105x-regulator";
>  		break;
>  	default:
>  		break;
>  	}
>  
>  	/* Set up and register the platform devices. */
> -	for (i = 0; i < ARRAY_SIZE(tps6105x_cells); i++) {
> +	for (i = 0; i < n_cells; i++) {
>  		/* One state holder for all drivers, this is simple */
>  		tps6105x_cells[i].platform_data = tps6105x;
>  		tps6105x_cells[i].pdata_size = sizeof(*tps6105x);
>  	}
>  
>  	return mfd_add_devices(&client->dev, 0, tps6105x_cells,
> -			       ARRAY_SIZE(tps6105x_cells), NULL, 0, NULL);
> +			       n_cells, NULL, 0, NULL);
>  }
>  
>  static int tps6105x_remove(struct i2c_client *client)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: tps6105x: Fix NULL pointer access.

[Grigoryev Denis]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3974 bytes --]



On Sat., 20/09/2015 в 05:20 +0100, Lee Jones wrote:
> On Fri, 04 Sep 2015, Grigoryev Denis wrote:
> 
> > When tps6105x used in TPS6105X_MODE_SHUTDOWN mode the driver calls
> > mfd_add_devices() with mfd_cell->name == NULL, that causes an ooops in
> > platform_device_register() later.
> > 
> > This patch reorders mfd_cell structures and makes code to call
> > mfd_add_devices() with proper number of cells.
> > 
> > Signed-off-by: Denis Grigoryev <grigoryev@spb.prosoft.ru>
> > ---
> >  drivers/mfd/tps6105x.c |   16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mfd/tps6105x.c b/drivers/mfd/tps6105x.c
> > index 5de95c2..be2c286 100644
> > --- a/drivers/mfd/tps6105x.c
> > +++ b/drivers/mfd/tps6105x.c
> > @@ -124,11 +124,11 @@ static int tps6105x_startup(struct tps6105x *tps6105x)
> >   */
> >  static struct mfd_cell tps6105x_cells[] = {
> >  	{
> > -		/* name will be runtime assigned */
> > +		.name = "tps6105x-gpio",
> >  		.id = -1,
> >  	},
> >  	{
> > -		.name = "tps6105x-gpio",
> > +		/* name will be runtime assigned */
> >  		.id = -1,
> >  	},
> >  };
> 
> So you have 2 cells with identical .name's and identical .id's.
> 
> How does that work?
> 

 No. Here is the first cell with .name = "tps6105x-gpio", the second one
has no name assigned. The name of second cell (or it's absence) will be
chosen in runtime in switch() below depending on selected IC operation
mode.

 This patch is a fix of possible NULL pointer access I encountered with
minimal invasion to the code. I apologize for too short description to
this patch.

> > @@ -138,6 +138,7 @@ static int tps6105x_probe(struct i2c_client *client,
> >  {
> >  	struct tps6105x			*tps6105x;
> >  	struct tps6105x_platform_data	*pdata;
> > +	int n_cells = ARRAY_SIZE(tps6105x_cells);
> >  	int ret;
> >  	int i;
> >  
> > @@ -162,33 +163,34 @@ static int tps6105x_probe(struct i2c_client *client,
> >  	case TPS6105X_MODE_SHUTDOWN:
> >  		dev_info(&client->dev,
> >  			 "present, not used for anything, only GPIO\n");
> > +		n_cells = 1;
> >  		break;
> >  	case TPS6105X_MODE_TORCH:
> > -		tps6105x_cells[0].name = "tps6105x-leds";
> > +		tps6105x_cells[1].name = "tps6105x-leds";
> 
> Now you're over-writing cell names!  I'd say this was a hack.
>

No. Here I assign a name to the second cell. It's been unknown until I
get desired IC mode.

> Please just create an entry for each device, like usual.
> 

Here is only two devices. The first one is "gpio" which always present,
the second one is selected depending on IC mode. A patch with an entry
for each device will be too complex for a fix.

I'm working on "flash" function and DT bindings for this driver so you
can reject this patch if you like.

> >  		dev_warn(&client->dev,
> >  			 "torch mode is unsupported\n");
> >  		break;
> >  	case TPS6105X_MODE_TORCH_FLASH:
> > -		tps6105x_cells[0].name = "tps6105x-flash";
> > +		tps6105x_cells[1].name = "tps6105x-flash";
> >  		dev_warn(&client->dev,
> >  			 "flash mode is unsupported\n");
> >  		break;
> >  	case TPS6105X_MODE_VOLTAGE:
> > -		tps6105x_cells[0].name ="tps6105x-regulator";
> > +		tps6105x_cells[1].name = "tps6105x-regulator";
> >  		break;
> >  	default:
> >  		break;
> >  	}
> >  
> >  	/* Set up and register the platform devices. */
> > -	for (i = 0; i < ARRAY_SIZE(tps6105x_cells); i++) {
> > +	for (i = 0; i < n_cells; i++) {
> >  		/* One state holder for all drivers, this is simple */
> >  		tps6105x_cells[i].platform_data = tps6105x;
> >  		tps6105x_cells[i].pdata_size = sizeof(*tps6105x);
> >  	}
> >  
> >  	return mfd_add_devices(&client->dev, 0, tps6105x_cells,
> > -			       ARRAY_SIZE(tps6105x_cells), NULL, 0, NULL);
> > +			       n_cells, NULL, 0, NULL);
> >  }
> >  
> >  static int tps6105x_remove(struct i2c_client *client)
> 


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH v2] mfd: tps6105x: Fix NULL pointer access.

[Grigoryev Denis]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3427 bytes --]

When tps6105x used in TPS6105X_MODE_SHUTDOWN mode the driver calls
mfd_add_devices() with mfd_cell->name == NULL, that causes an ooops in
platform_device_register() later.

This patch adds an mfd_cell for each possible mode thereby excluding runtime
.name assignment.

Signed-off-by: Denis Grigoryev <grigoryev@fastwel.ru>
---
 drivers/mfd/tps6105x.c |   62 ++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 26 deletions(-)

diff --git a/drivers/mfd/tps6105x.c b/drivers/mfd/tps6105x.c
index 5de95c2..4dbfb45 100644
--- a/drivers/mfd/tps6105x.c
+++ b/drivers/mfd/tps6105x.c
@@ -119,17 +119,25 @@ static int tps6105x_startup(struct tps6105x *tps6105x)
 }
 
 /*
- * MFD cells - we have one cell which is selected operation
- * mode, and we always have a GPIO cell.
- */
+ *  * MFD cells - we always have a GPIO cell and we have one cell
+ *   * which is selected operation mode.
+ *    */
 static struct mfd_cell tps6105x_cells[] = {
-	{
-		/* name will be runtime assigned */
-		.id = -1,
+	[TPS6105X_REG0_MODE_SHUTDOWN]   = {
+		.name			= "tps6105x-gpio",
+		.id			= -1,
+	},
+	[TPS6105X_REG0_MODE_TORCH]	= {
+		.name			= "tps6105x-leds",
+		.id			= -1,
+	},
+	[TPS6105X_REG0_MODE_TORCH_FLASH] = {
+		.name			= "tps6105x-flash",
+		.id			= -1,
 	},
-	{
-		.name = "tps6105x-gpio",
-		.id = -1,
+	[TPS6105X_REG0_MODE_VOLTAGE]	= {
+		.name			= "tps6105x-regulator",
+		.id			= -1,
 	},
 };
 
@@ -157,6 +165,18 @@ static int tps6105x_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	/* Set up and register the platform devices. */
+	for (i = 0; i < ARRAY_SIZE(tps6105x_cells); i++) {
+		/* One state holder for all drivers, this is simple */
+		tps6105x_cells[i].platform_data = tps6105x;
+		tps6105x_cells[i].pdata_size = sizeof(*tps6105x);
+	}
+
+	ret = mfd_add_devices(&client->dev, -1, tps6105x_cells,
+			      1, NULL, 0, NULL);
+	if (ret)
+		return ret;
+
 	/* Remove warning texts when you implement new cell drivers */
 	switch (pdata->mode) {
 	case TPS6105X_MODE_SHUTDOWN:
@@ -164,31 +184,21 @@ static int tps6105x_probe(struct i2c_client *client,
 			 "present, not used for anything, only GPIO\n");
 		break;
 	case TPS6105X_MODE_TORCH:
-		tps6105x_cells[0].name = "tps6105x-leds";
-		dev_warn(&client->dev,
-			 "torch mode is unsupported\n");
-		break;
 	case TPS6105X_MODE_TORCH_FLASH:
-		tps6105x_cells[0].name = "tps6105x-flash";
-		dev_warn(&client->dev,
-			 "flash mode is unsupported\n");
-		break;
 	case TPS6105X_MODE_VOLTAGE:
-		tps6105x_cells[0].name ="tps6105x-regulator";
+		ret = mfd_add_devices(&client->dev, -1,
+				      &tps6105x_cells[pdata->mode],
+				      1, NULL, 0, NULL);
 		break;
 	default:
+		dev_warn(&client->dev, "invalid mode: %d\n", pdata->mode);
 		break;
 	}
 
-	/* Set up and register the platform devices. */
-	for (i = 0; i < ARRAY_SIZE(tps6105x_cells); i++) {
-		/* One state holder for all drivers, this is simple */
-		tps6105x_cells[i].platform_data = tps6105x;
-		tps6105x_cells[i].pdata_size = sizeof(*tps6105x);
-	}
+	if (ret)
+		mfd_remove_devices(&client->dev);
 
-	return mfd_add_devices(&client->dev, 0, tps6105x_cells,
-			       ARRAY_SIZE(tps6105x_cells), NULL, 0, NULL);
+	return ret;
 }
 
 static int tps6105x_remove(struct i2c_client *client)
-- 
1.7.10.4

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2] mfd: tps6105x: Fix NULL pointer access.

[Grigoryev Denis]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3840 bytes --]

Oh, sorry. With further testing I realized that I made a mistake and
typos. I will send the patch v3.

My apologies for the flood. I'm new in LKML.

On Mon., 21/09/2015 at 13:41 +0000, Grigoryev Denis wrote:
> When tps6105x used in TPS6105X_MODE_SHUTDOWN mode the driver calls
> mfd_add_devices() with mfd_cell->name == NULL, that causes an ooops in
> platform_device_register() later.
> 
> This patch adds an mfd_cell for each possible mode thereby excluding runtime
> .name assignment.
> 
> Signed-off-by: Denis Grigoryev <grigoryev@fastwel.ru>
> ---
>  drivers/mfd/tps6105x.c |   62 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 36 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/mfd/tps6105x.c b/drivers/mfd/tps6105x.c
> index 5de95c2..4dbfb45 100644
> --- a/drivers/mfd/tps6105x.c
> +++ b/drivers/mfd/tps6105x.c
> @@ -119,17 +119,25 @@ static int tps6105x_startup(struct tps6105x *tps6105x)
>  }
>  
>  /*
> - * MFD cells - we have one cell which is selected operation
> - * mode, and we always have a GPIO cell.
> - */
> + *  * MFD cells - we always have a GPIO cell and we have one cell
> + *   * which is selected operation mode.
> + *    */
>  static struct mfd_cell tps6105x_cells[] = {
> -	{
> -		/* name will be runtime assigned */
> -		.id = -1,
> +	[TPS6105X_REG0_MODE_SHUTDOWN]   = {
> +		.name			= "tps6105x-gpio",
> +		.id			= -1,
> +	},
> +	[TPS6105X_REG0_MODE_TORCH]	= {
> +		.name			= "tps6105x-leds",
> +		.id			= -1,
> +	},
> +	[TPS6105X_REG0_MODE_TORCH_FLASH] = {
> +		.name			= "tps6105x-flash",
> +		.id			= -1,
>  	},
> -	{
> -		.name = "tps6105x-gpio",
> -		.id = -1,
> +	[TPS6105X_REG0_MODE_VOLTAGE]	= {
> +		.name			= "tps6105x-regulator",
> +		.id			= -1,
>  	},
>  };
>  
> @@ -157,6 +165,18 @@ static int tps6105x_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	/* Set up and register the platform devices. */
> +	for (i = 0; i < ARRAY_SIZE(tps6105x_cells); i++) {
> +		/* One state holder for all drivers, this is simple */
> +		tps6105x_cells[i].platform_data = tps6105x;
> +		tps6105x_cells[i].pdata_size = sizeof(*tps6105x);
> +	}
> +
> +	ret = mfd_add_devices(&client->dev, -1, tps6105x_cells,
> +			      1, NULL, 0, NULL);
> +	if (ret)
> +		return ret;
> +
>  	/* Remove warning texts when you implement new cell drivers */
>  	switch (pdata->mode) {
>  	case TPS6105X_MODE_SHUTDOWN:
> @@ -164,31 +184,21 @@ static int tps6105x_probe(struct i2c_client *client,
>  			 "present, not used for anything, only GPIO\n");
>  		break;
>  	case TPS6105X_MODE_TORCH:
> -		tps6105x_cells[0].name = "tps6105x-leds";
> -		dev_warn(&client->dev,
> -			 "torch mode is unsupported\n");
> -		break;
>  	case TPS6105X_MODE_TORCH_FLASH:
> -		tps6105x_cells[0].name = "tps6105x-flash";
> -		dev_warn(&client->dev,
> -			 "flash mode is unsupported\n");
> -		break;
>  	case TPS6105X_MODE_VOLTAGE:
> -		tps6105x_cells[0].name ="tps6105x-regulator";
> +		ret = mfd_add_devices(&client->dev, -1,
> +				      &tps6105x_cells[pdata->mode],
> +				      1, NULL, 0, NULL);
>  		break;
>  	default:
> +		dev_warn(&client->dev, "invalid mode: %d\n", pdata->mode);
>  		break;
>  	}
>  
> -	/* Set up and register the platform devices. */
> -	for (i = 0; i < ARRAY_SIZE(tps6105x_cells); i++) {
> -		/* One state holder for all drivers, this is simple */
> -		tps6105x_cells[i].platform_data = tps6105x;
> -		tps6105x_cells[i].pdata_size = sizeof(*tps6105x);
> -	}
> +	if (ret)
> +		mfd_remove_devices(&client->dev);
>  
> -	return mfd_add_devices(&client->dev, 0, tps6105x_cells,
> -			       ARRAY_SIZE(tps6105x_cells), NULL, 0, NULL);
> +	return ret;
>  }
>  
>  static int tps6105x_remove(struct i2c_client *client)

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥