Patchwork [1/2] mfd: tps65910: Fix crash in i2c_driver .probe

login
register
mail settings
Submitter Tuomas Tynkkynen
Date June 17, 2013, 5:47 p.m.
Message ID <1371491257-23791-2-git-send-email-ttynkkynen@nvidia.com>
Download mbox | patch
Permalink /patch/252025/
State Not Applicable
Headers show

Comments

Tuomas Tynkkynen - June 17, 2013, 5:47 p.m.
Commit "i2c: core: make it possible to match a pure device tree driver"
changed semantics of the i2c probing for device tree devices.
Device tree probed devices now get a NULL i2c_device_id pointer.
This caused kernel panics due to NULL dereference.

Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com>
---
 drivers/mfd/tps65910.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Stephen Warren - June 17, 2013, 6:07 p.m.
On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
> Commit "i2c: core: make it possible to match a pure device tree driver"
> changed semantics of the i2c probing for device tree devices.
> Device tree probed devices now get a NULL i2c_device_id pointer.
> This caused kernel panics due to NULL dereference.

> diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c

>  	pmic_plat_data = dev_get_platdata(&i2c->dev);
>  
> -	if (!pmic_plat_data && i2c->dev.of_node) {
> +	if (id) {
> +		chip_id = id->driver_data;
> +	} else if (i2c->dev.of_node) {
>  		pmic_plat_data = tps65910_parse_dt(i2c, &chip_id);

That over-writes pmic_plat_data even if it was already set above. This
should really only happen if the earlier assignment didn't find any
pdata, i.e. if (!pmic_plat_data) here.

Looking at patch 2/2, the structure in that driver is correct, and
perhaps could be implemented the same or similarly here?

>  		of_pmic_plat_data = pmic_plat_data;

Or just swap those assignments:

of_pmic_plat_data = tps65910_parse_dt(...);
if (!pmic_plat_data)
    pmic_plat_data = of_pmic_plat_data;

(although there's perhaps little point parsing the pdata from DT if it's
already provided through the device object)

>  	}
>  
> -	if (!pmic_plat_data)
> +	if (!pmic_plat_data || chip_id < 0)
>  		return -EINVAL;

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - June 17, 2013, 6:16 p.m.
On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
> Commit "i2c: core: make it possible to match a pure device tree driver"
> changed semantics of the i2c probing for device tree devices.
> Device tree probed devices now get a NULL i2c_device_id pointer.
> This caused kernel panics due to NULL dereference.

Tested-by: Stephen Warren <swarren@nvidia.com>

(although I imagine I'd need to retest if there was a v2 to address my
previous comments)
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Samuel Ortiz - June 18, 2013, 9:16 a.m.
On Mon, Jun 17, 2013 at 12:16:33PM -0600, Stephen Warren wrote:
> On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
> > Commit "i2c: core: make it possible to match a pure device tree driver"
> > changed semantics of the i2c probing for device tree devices.
> > Device tree probed devices now get a NULL i2c_device_id pointer.
> > This caused kernel panics due to NULL dereference.
> 
> Tested-by: Stephen Warren <swarren@nvidia.com>
> 
> (although I imagine I'd need to retest if there was a v2 to address my
> previous comments)
I would prefer seeing a v2, especially to address the pmic_plat_data
overwriting issue.

Cheers,
Samuel.
Tuomas Tynkkynen - June 18, 2013, 9:35 a.m.
On 06/17/2013 09:07 PM, Stephen Warren wrote:
> On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
>> Commit "i2c: core: make it possible to match a pure device tree driver"
>> changed semantics of the i2c probing for device tree devices.
>> Device tree probed devices now get a NULL i2c_device_id pointer.
>> This caused kernel panics due to NULL dereference.
> 
>> diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
> 
>>  	pmic_plat_data = dev_get_platdata(&i2c->dev);
>>  
>> -	if (!pmic_plat_data && i2c->dev.of_node) {
>> +	if (id) {
>> +		chip_id = id->driver_data;
>> +	} else if (i2c->dev.of_node) {
>>  		pmic_plat_data = tps65910_parse_dt(i2c, &chip_id);
> 
> That over-writes pmic_plat_data even if it was already set above. This
> should really only happen if the earlier assignment didn't find any
> pdata, i.e. if (!pmic_plat_data) here.

That would cause the probe() to fail since it doesn't have a chip_id.

> 
> Looking at patch 2/2, the structure in that driver is correct, and
> perhaps could be implemented the same or similarly here?
> 

This seems to be the best way, I'll change it that way.

>>  		of_pmic_plat_data = pmic_plat_data;
> 
> Or just swap those assignments:
> 
> of_pmic_plat_data = tps65910_parse_dt(...);
> if (!pmic_plat_data)
>     pmic_plat_data = of_pmic_plat_data;
> 
> (although there's perhaps little point parsing the pdata from DT if it's
> already provided through the device object)

Yeah, especially since tps65910_parse_dt can dev_warn().

> 
>>  	}
>>  
>> -	if (!pmic_plat_data)
>> +	if (!pmic_plat_data || chip_id < 0)
>>  		return -EINVAL;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index d792772..a62c30d 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -461,16 +461,18 @@  static int tps65910_i2c_probe(struct i2c_client *i2c,
 	struct tps65910_board *of_pmic_plat_data = NULL;
 	struct tps65910_platform_data *init_data;
 	int ret = 0;
-	int chip_id = id->driver_data;
+	int chip_id = -1;
 
 	pmic_plat_data = dev_get_platdata(&i2c->dev);
 
-	if (!pmic_plat_data && i2c->dev.of_node) {
+	if (id) {
+		chip_id = id->driver_data;
+	} else if (i2c->dev.of_node) {
 		pmic_plat_data = tps65910_parse_dt(i2c, &chip_id);
 		of_pmic_plat_data = pmic_plat_data;
 	}
 
-	if (!pmic_plat_data)
+	if (!pmic_plat_data || chip_id < 0)
 		return -EINVAL;
 
 	init_data = devm_kzalloc(&i2c->dev, sizeof(*init_data), GFP_KERNEL);