[bug report] pinctrl: devicetree: fix null pointer dereferencing in pinctrl_dt_to_map

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

Hello Zeng Heng,

Commit 91d5c5060ee2 ("pinctrl: devicetree: fix null pointer
dereferencing in pinctrl_dt_to_map") from Nov 10, 2022, leads to the
following Smatch static checker warning:

	drivers/pinctrl/devicetree.c:224 pinctrl_dt_to_map()
	warn: refcount leak 'np->kobj.kref.refcount.refs.counter': lines='224'

drivers/pinctrl/devicetree.c
    196 int pinctrl_dt_to_map(struct pinctrl *p, struct pinctrl_dev *pctldev)
    197 {
    198         struct device_node *np = p->dev->of_node;
    199         int state, ret;
    200         char *propname;
    201         struct property *prop;
    202         const char *statename;
    203         const __be32 *list;
    204         int size, config;
    205         phandle phandle;
    206         struct device_node *np_config;
    207 
    208         /* CONFIG_OF enabled, p->dev not instantiated from DT */
    209         if (!np) {
    210                 if (of_have_populated_dt())
    211                         dev_dbg(p->dev,
    212                                 "no of_node; not parsing pinctrl DT\n");
    213                 return 0;
    214         }
    215 
    216         /* We may store pointers to property names within the node */
    217         of_node_get(np);
    218 
    219         /* For each defined state ID */
    220         for (state = 0; ; state++) {
    221                 /* Retrieve the pinctrl-* property */
    222                 propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
    223                 if (!propname)
--> 224                         return -ENOMEM;

This should probably be "ret = -ENOMEM; goto err;", but is it okay to
goto err on the first iteration when state is zero?

    225                 prop = of_find_property(np, propname, &size);
    226                 kfree(propname);
    227                 if (!prop) {
    228                         if (state == 0) {
    229                                 of_node_put(np);
    230                                 return -ENODEV;

Here state == 0 is treated as a special case.  Which is fine.  But
could it also have done a goto err instead?  I hope so because otherwise
we'd have a bug later on the first iteration...

    231                         }
    232                         break;
    233                 }
    234                 list = prop->value;
    235                 size /= sizeof(*list);
    236 
    237                 /* Determine whether pinctrl-names property names the state */
    238                 ret = of_property_read_string_index(np, "pinctrl-names",
    239                                                     state, &statename);
    240                 /*
    241                  * If not, statename is just the integer state ID. But rather
    242                  * than dynamically allocate it and have to free it later,
    243                  * just point part way into the property name for the string.
    244                  */
    245                 if (ret < 0)
    246                         statename = prop->name + strlen("pinctrl-");
    247 
    248                 /* For every referenced pin configuration node in it */
    249                 for (config = 0; config < size; config++) {
    250                         phandle = be32_to_cpup(list++);
    251 
    252                         /* Look up the pin configuration node */
    253                         np_config = of_find_node_by_phandle(phandle);
    254                         if (!np_config) {
    255                                 dev_err(p->dev,
    256                                         "prop %s index %i invalid phandle\n",
    257                                         prop->name, config);
    258                                 ret = -EINVAL;
    259                                 goto err;
    260                         }
    261 
    262                         /* Parse the node */
    263                         ret = dt_to_map_one_config(p, pctldev, statename,
    264                                                    np_config);
    265                         of_node_put(np_config);
    266                         if (ret < 0)
    267                                 goto err;
    268                 }
    269 
    270                 /* No entries in DT? Generate a dummy state table entry */
    271                 if (!size) {
    272                         ret = dt_remember_dummy_state(p, statename);
    273                         if (ret < 0)
    274                                 goto err;
    275                 }
    276         }
    277 
    278         return 0;
    279 
    280 err:
    281         pinctrl_dt_free_maps(p);
    282         return ret;
    283 }

regards,
dan carpenter