lkml.org 
[lkml]   [2012]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 3/4] pinctrl: Add checks for empty names in pinmux_search_function
* 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;
}

\
 
 \ /
  Last update: 2012-01-25 23:11    [W:0.046 / U:6.240 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site