Re: [PATCH 3/4] pinctrl: Add checks for empty names in pinmux_search_function

[Tony Lindgren <tony@atomide.com>]

* Tony Lindgren <tony@atomide.com> [120120 09:09]:
> * Stephen Warren <swarren@nvidia.com> [120120 08:28]:
> > Tony Lindgren wrote at Friday, January 20, 2012 9:18 AM:
> > > Otherwise we can get the following when dealing with
> > > buggy data in a pinmux driver:
> > > 
> > > Unable to handle kernel NULL pointer dereference at virtual address 00000000
> > 
> > > diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> > > index 06b8943..ffe633d 100644
> > > --- a/drivers/pinctrl/pinmux.c
> > > +++ b/drivers/pinctrl/pinmux.c
> > > @@ -584,6 +584,13 @@ static int pinmux_search_function(struct pinctrl_dev *pctldev,
> > >  							   selector);
> > >  		int ret;
> > > 
> > > +		if (!fname) {
> > > +			pr_warning("no name for function%i\n",
> > > +				   selector);
> > > +			selector++;
> > > +			continue;
> > > +		}
> > > +
> > >  		if (!strcmp(map->function, fname)) {
> > >  			/* Found the function, check pin group */
> > >  			ret = pinmux_check_pin_group(pctldev, selector,
> > 
> > Shouldn't this be BUG_ON(!fname)?
> >
> > There are lots of other places that pmxops->get_function_name() is
> > called. Wouldn't it be better to enhance e.g. pinmux_check_ops() to
> > validate that all functions have a name during pinctrl registration?
> 
> Maybe yeah. I'll take a look next week when I'm back home.

There's a little bit of a init order issue here to add checking for
the function names during registration. That's because we don't allocate
pctldev until after pinmux_check_ops() in pinctrl_register. This means
we can't currently call ops->list_functions until unless we change the
init order.

To fix this we need to allocate pctldev earlier, and then change the
*_check_ops functions to use pctldev, see below.

Regards,

Tony


From: Tony Lindgren <tony@atomide.com>
Date: Tue, 24 Jan 2012 16:28:08 -0800
Subject: [PATCH] pinctrl: Add checks for empty function names

This is needed as otherwise we can get the following when dealing
with buggy data in a pinmux driver for pinmux_search_function:

Unable to handle kernel NULL pointer dereference at virtual address 00000000
...
PC is at strcmp+0xc/0x34
LR is at pinmux_get+0x350/0x8f4
...

As we need pctldev initialized to call ops->list_functions, let's
initialize it before check_ops calls and pass the pctldev to the
check_ops functions. Do this for both pinmux and pinconf check_ops
functions.

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -583,40 +583,40 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 	if (pctldesc->name == NULL)
 		return NULL;
 
+	pctldev = kzalloc(sizeof(struct pinctrl_dev), GFP_KERNEL);
+	if (pctldev == NULL)
+		return NULL;
+
+	/* Initialize pin control device struct */
+	pctldev->owner = pctldesc->owner;
+	pctldev->desc = pctldesc;
+	pctldev->driver_data = driver_data;
+	INIT_RADIX_TREE(&pctldev->pin_desc_tree, GFP_KERNEL);
+	spin_lock_init(&pctldev->pin_desc_tree_lock);
+	INIT_LIST_HEAD(&pctldev->gpio_ranges);
+	mutex_init(&pctldev->gpio_ranges_lock);
+	pctldev->dev = dev;
+
 	/* If we're implementing pinmuxing, check the ops for sanity */
 	if (pctldesc->pmxops) {
-		ret = pinmux_check_ops(pctldesc->pmxops);
+		ret = pinmux_check_ops(pctldev);
 		if (ret) {
 			pr_err("%s pinmux ops lacks necessary functions\n",
 			       pctldesc->name);
-			return NULL;
+			goto out_err;
 		}
 	}
 
 	/* If we're implementing pinconfig, check the ops for sanity */
 	if (pctldesc->confops) {
-		ret = pinconf_check_ops(pctldesc->confops);
+		ret = pinconf_check_ops(pctldev);
 		if (ret) {
 			pr_err("%s pin config ops lacks necessary functions\n",
 			       pctldesc->name);
-			return NULL;
+			goto out_err;
 		}
 	}
 
-	pctldev = kzalloc(sizeof(struct pinctrl_dev), GFP_KERNEL);
-	if (pctldev == NULL)
-		return NULL;
-
-	/* Initialize pin control device struct */
-	pctldev->owner = pctldesc->owner;
-	pctldev->desc = pctldesc;
-	pctldev->driver_data = driver_data;
-	INIT_RADIX_TREE(&pctldev->pin_desc_tree, GFP_KERNEL);
-	spin_lock_init(&pctldev->pin_desc_tree_lock);
-	INIT_LIST_HEAD(&pctldev->gpio_ranges);
-	mutex_init(&pctldev->gpio_ranges_lock);
-	pctldev->dev = dev;
-
 	/* Register all the pins */
 	pr_debug("try to register %d pins on %s...\n",
 		 pctldesc->npins, pctldesc->name);
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -205,8 +205,10 @@ int pin_config_group_set(const char *dev_name, const char *pin_group,
 }
 EXPORT_SYMBOL(pin_config_group_set);
 
-int pinconf_check_ops(const struct pinconf_ops *ops)
+int pinconf_check_ops(struct pinctrl_dev *pctldev)
 {
+	const struct pinconf_ops *ops = pctldev->desc->confops;
+
 	/* We must be able to read out pin status */
 	if (!ops->pin_config_get && !ops->pin_config_group_get)
 		return -EINVAL;
--- a/drivers/pinctrl/pinconf.h
+++ b/drivers/pinctrl/pinconf.h
@@ -13,7 +13,7 @@
 
 #ifdef CONFIG_PINCONF
 
-int pinconf_check_ops(const struct pinconf_ops *ops);
+int pinconf_check_ops(struct pinctrl_dev *pctldev);
 void pinconf_init_device_debugfs(struct dentry *devroot,
 				 struct pinctrl_dev *pctldev);
 int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
@@ -23,7 +23,7 @@ int pin_config_set_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
 
 #else
 
-static inline int pinconf_check_ops(const struct pinconf_ops *ops)
+static inline int pinconf_check_ops(struct pinctrl_dev *pctldev)
 {
 	return 0;
 }
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -904,8 +904,11 @@ void pinmux_disable(struct pinmux *pmx)
 }
 EXPORT_SYMBOL_GPL(pinmux_disable);
 
-int pinmux_check_ops(const struct pinmux_ops *ops)
+int pinmux_check_ops(struct pinctrl_dev *pctldev)
 {
+	const struct pinmux_ops *ops = pctldev->desc->pmxops;
+	unsigned selector = 0;
+
 	/* Check that we implement required operations */
 	if (!ops->list_functions ||
 	    !ops->get_function_name ||
@@ -914,6 +917,18 @@ int pinmux_check_ops(const struct pinmux_ops *ops)
 	    !ops->disable)
 		return -EINVAL;
 
+	/* Check that all functions registered have names */
+	while (ops->list_functions(pctldev, selector) >= 0) {
+		const char *fname = ops->get_function_name(pctldev,
+							   selector);
+		if (!fname) {
+			pr_err("pinmux ops has no name for function%u\n",
+				selector);
+			return -EINVAL;
+		}
+		selector++;
+	}
+
 	return 0;
 }
 
--- a/drivers/pinctrl/pinmux.h
+++ b/drivers/pinctrl/pinmux.h
@@ -12,7 +12,7 @@
  */
 #ifdef CONFIG_PINMUX
 
-int pinmux_check_ops(const struct pinmux_ops *ops);
+int pinmux_check_ops(struct pinctrl_dev *pctldev);
 void pinmux_init_device_debugfs(struct dentry *devroot,
 				struct pinctrl_dev *pctldev);
 void pinmux_init_debugfs(struct dentry *subsys_root);
@@ -21,7 +21,7 @@ void pinmux_unhog_maps(struct pinctrl_dev *pctldev);
 
 #else
 
-static inline int pinmux_check_ops(const struct pinmux_ops *ops)
+static inline int pinmux_check_ops(struct pinctrl_dev *pctldev)
 {
 	return 0;
 }