| Message ID | 20220318160059.328208-2-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | introduce fwnode in the I2C subsystem | expand |
On Fri, Mar 18, 2022 at 05:00:47PM +0100, Clément Léger wrote: > Add fwnode_property_read_string_index() function which allows to > retrieve a string from an array by its index. This function is the > equivalent of of_property_read_string_index() but for fwnode support. ... > + values = kcalloc(nval, sizeof(*values), GFP_KERNEL); > + if (!values) > + return -ENOMEM; > + > + ret = fwnode_property_read_string_array(fwnode, propname, values, nval); > + if (ret < 0) > + goto out; > + > + *string = values[index]; > +out: > + kfree(values); Here is UAF (use after free). How is it supposed to work?
Le Fri, 18 Mar 2022 18:26:00 +0200, Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > On Fri, Mar 18, 2022 at 05:00:47PM +0100, Clément Léger wrote: > > Add fwnode_property_read_string_index() function which allows to > > retrieve a string from an array by its index. This function is the > > equivalent of of_property_read_string_index() but for fwnode support. > > ... > > > + values = kcalloc(nval, sizeof(*values), GFP_KERNEL); > > + if (!values) > > + return -ENOMEM; > > + > > + ret = fwnode_property_read_string_array(fwnode, propname, values, nval); > > + if (ret < 0) > > + goto out; > > + > > + *string = values[index]; > > +out: > > + kfree(values); > > Here is UAF (use after free). How is it supposed to work? > values is an array of pointers. I'm only retrieving a pointer out of it.
On Fri, Mar 18, 2022 at 05:49:12PM +0100, Clément Léger wrote: > Le Fri, 18 Mar 2022 18:26:00 +0200, > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > > On Fri, Mar 18, 2022 at 05:00:47PM +0100, Clément Léger wrote: > > > Add fwnode_property_read_string_index() function which allows to > > > retrieve a string from an array by its index. This function is the > > > equivalent of of_property_read_string_index() but for fwnode support. ... > > > + values = kcalloc(nval, sizeof(*values), GFP_KERNEL); > > > + if (!values) > > > + return -ENOMEM; > > > + > > > + ret = fwnode_property_read_string_array(fwnode, propname, values, nval); > > > + if (ret < 0) > > > + goto out; > > > + > > > + *string = values[index]; > > > +out: > > > + kfree(values); > > > > Here is UAF (use after free). How is it supposed to work? > > values is an array of pointers. I'm only retrieving a pointer out of > it. I see, thanks for pointing out. Nevertheless, I don't like the idea of allocating memory in this case. Can we rather add a new callback that will provide us the necessary property directly?
Le Fri, 18 Mar 2022 20:09:37 +0200, Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > On Fri, Mar 18, 2022 at 05:49:12PM +0100, Clément Léger wrote: > > Le Fri, 18 Mar 2022 18:26:00 +0200, > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > > > On Fri, Mar 18, 2022 at 05:00:47PM +0100, Clément Léger wrote: > > > > Add fwnode_property_read_string_index() function which allows to > > > > retrieve a string from an array by its index. This function is the > > > > equivalent of of_property_read_string_index() but for fwnode support. > > ... > > > > > + values = kcalloc(nval, sizeof(*values), GFP_KERNEL); > > > > + if (!values) > > > > + return -ENOMEM; > > > > + > > > > + ret = fwnode_property_read_string_array(fwnode, propname, values, nval); > > > > + if (ret < 0) > > > > + goto out; > > > > + > > > > + *string = values[index]; > > > > +out: > > > > + kfree(values); > > > > > > Here is UAF (use after free). How is it supposed to work? > > > > values is an array of pointers. I'm only retrieving a pointer out of > > it. > > I see, thanks for pointing out. > > Nevertheless, I don't like the idea of allocating memory in this case. > Can we rather add a new callback that will provide us the necessary > property directly? > IMHO, it would indeed be better. However, fwnode_property_match_string() also allocates memory to do the same kind of operation. Would you also like a callback for this one ? Thanks,
On Mon, Mar 21, 2022 at 08:49:21AM +0100, Clément Léger wrote: > Le Fri, 18 Mar 2022 20:09:37 +0200, > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > > On Fri, Mar 18, 2022 at 05:49:12PM +0100, Clément Léger wrote: > > > Le Fri, 18 Mar 2022 18:26:00 +0200, > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > > > > On Fri, Mar 18, 2022 at 05:00:47PM +0100, Clément Léger wrote: ... > > > > > + values = kcalloc(nval, sizeof(*values), GFP_KERNEL); > > > > > + if (!values) > > > > > + return -ENOMEM; > > > > > + > > > > > + ret = fwnode_property_read_string_array(fwnode, propname, values, nval); > > > > > + if (ret < 0) > > > > > + goto out; > > > > > + > > > > > + *string = values[index]; > > > > > +out: > > > > > + kfree(values); > > > > > > > > Here is UAF (use after free). How is it supposed to work? > > > > > > values is an array of pointers. I'm only retrieving a pointer out of > > > it. > > > > I see, thanks for pointing out. > > > > Nevertheless, I don't like the idea of allocating memory in this case. > > Can we rather add a new callback that will provide us the necessary > > property directly? > > > > IMHO, it would indeed be better. However, > fwnode_property_match_string() also allocates memory to do the same > kind of operation. Would you also like a callback for this one ? But matching string will need all of them to cover all possible cases. So, it doesn't rely on the certain index and needs allocation anyway.
Le Mon, 21 Mar 2022 12:31:58 +0200, Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > > > > IMHO, it would indeed be better. However, > > fwnode_property_match_string() also allocates memory to do the same > > kind of operation. Would you also like a callback for this one ? > > But matching string will need all of them to cover all possible cases. > So, it doesn't rely on the certain index and needs allocation anyway. Acked, it makes sense to keep it that way.
diff --git a/drivers/base/property.c b/drivers/base/property.c index e6497f6877ee..67c33c11f00c 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -451,6 +451,54 @@ int fwnode_property_match_string(const struct fwnode_handle *fwnode, } EXPORT_SYMBOL_GPL(fwnode_property_match_string); +/** + * fwnode_property_read_string_index - read a string in an array using an index + * @fwnode: Firmware node to get the property of + * @propname: Name of the property holding the array + * @index: Index of the string to look for + * @string: Pointer to the string if found + * + * Find a string by a given index in a string array and if it is found return + * the string value in @string. + * + * Return: %0 if the property was found (success), + * %-EINVAL if given arguments are not valid, + * %-ENODATA if the property does not have a value, + * %-EPROTO if the property is not an array of strings, + * %-ENXIO if no suitable firmware interface is present. + */ +int fwnode_property_read_string_index(const struct fwnode_handle *fwnode, + const char *propname, int index, + const char **string) +{ + const char **values; + int nval, ret; + + nval = fwnode_property_read_string_array(fwnode, propname, NULL, 0); + if (nval < 0) + return nval; + + if (index >= nval) + return -EINVAL; + + if (nval == 0) + return -ENODATA; + + values = kcalloc(nval, sizeof(*values), GFP_KERNEL); + if (!values) + return -ENOMEM; + + ret = fwnode_property_read_string_array(fwnode, propname, values, nval); + if (ret < 0) + goto out; + + *string = values[index]; +out: + kfree(values); + return ret; +} +EXPORT_SYMBOL_GPL(fwnode_property_read_string_index); + /** * fwnode_property_get_reference_args() - Find a reference with arguments * @fwnode: Firmware node where to look for the reference diff --git a/include/linux/property.h b/include/linux/property.h index 7399a0b45f98..a033920eb10a 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -70,6 +70,9 @@ int fwnode_property_read_string_array(const struct fwnode_handle *fwnode, size_t nval); int fwnode_property_read_string(const struct fwnode_handle *fwnode, const char *propname, const char **val); +int fwnode_property_read_string_index(const struct fwnode_handle *fwnode, + const char *propname, int index, + const char **string); int fwnode_property_match_string(const struct fwnode_handle *fwnode, const char *propname, const char *string); int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
Add fwnode_property_read_string_index() function which allows to retrieve a string from an array by its index. This function is the equivalent of of_property_read_string_index() but for fwnode support. Signed-off-by: Clément Léger <clement.leger@bootlin.com> --- drivers/base/property.c | 48 ++++++++++++++++++++++++++++++++++++++++ include/linux/property.h | 3 +++ 2 files changed, 51 insertions(+)