| Message ID | 20230721170022.3461-4-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: core: A few code cleanups and documentation fixes | expand |
On Fri, 21 Jul 2023 20:00:17 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Let the krealloc_array() copy the original data and > check for a multiplication overflow. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Nuno Sa <nuno.sa@analog.com> > --- > drivers/iio/industrialio-core.c | 20 +++++++------------- > 1 file changed, 7 insertions(+), 13 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index 4e45740331ee..6e28c2a3d223 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -1465,7 +1465,7 @@ int iio_device_register_sysfs_group(struct iio_dev *indio_dev, > const struct attribute_group **new, **old = iio_dev_opaque->groups; > unsigned int cnt = iio_dev_opaque->groupcounter; > > - new = krealloc(old, sizeof(*new) * (cnt + 2), GFP_KERNEL); > + new = krealloc_array(old, cnt + 2, sizeof(*new), GFP_KERNEL); > if (!new) > return -ENOMEM; > > @@ -1483,13 +1483,13 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev) > { > struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > int i, ret = 0, attrcount, attrn, attrcount_orig = 0; > + struct attribute **attrs, **attr, *clk = NULL; > struct iio_dev_attr *p; > - struct attribute **attr, *clk = NULL; > > /* First count elements in any existing group */ > - if (indio_dev->info->attrs) { > - attr = indio_dev->info->attrs->attrs; > - while (*attr++ != NULL) > + attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs : NULL; > + if (attrs) { > + for (attr = attrs; *attr; attr++) > attrcount_orig++; > } > attrcount = attrcount_orig; > @@ -1521,20 +1521,14 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev) > if (clk) > attrcount++; > > + /* Copy across original attributes, and point to original binary attributes */ > iio_dev_opaque->chan_attr_group.attrs = > - kcalloc(attrcount + 1, > - sizeof(iio_dev_opaque->chan_attr_group.attrs[0]), > - GFP_KERNEL); > + krealloc_array(attrs, attrcount + 1, sizeof(*attrs), GFP_KERNEL); I'm a little lost, but isn't this realloc()ing attrs, which should be provided by drivers as constant if it is set to indio_dev->info->attrs->attrs? That seems unlikely to work correctly, but I may well have lost track of the flow and attrs points somewhere else at this point. I guess it might work as the realloc code will detect it can't resize that array. Feels wrong approach however. > if (iio_dev_opaque->chan_attr_group.attrs == NULL) { > ret = -ENOMEM; > goto error_clear_attrs; > } > - /* Copy across original attributes, and point to original binary attributes */ > if (indio_dev->info->attrs) { > - memcpy(iio_dev_opaque->chan_attr_group.attrs, > - indio_dev->info->attrs->attrs, > - sizeof(iio_dev_opaque->chan_attr_group.attrs[0]) > - *attrcount_orig); > iio_dev_opaque->chan_attr_group.is_visible = > indio_dev->info->attrs->is_visible; > iio_dev_opaque->chan_attr_group.bin_attrs =
On Sat, Jul 22, 2023 at 06:28:20PM +0100, Jonathan Cameron wrote: > On Fri, 21 Jul 2023 20:00:17 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: ... > > + krealloc_array(attrs, attrcount + 1, sizeof(*attrs), GFP_KERNEL); > > > I'm a little lost, but isn't this realloc()ing attrs, which should be provided > by drivers as constant if it is set to indio_dev->info->attrs->attrs? > > That seems unlikely to work correctly, but I may well have lost track of the > flow and attrs points somewhere else at this point. I guess it might work > as the realloc code will detect it can't resize that array. Argh! The attrs are defined without const. So, basically to prevent code like this we have to make sure our local variables are defined as const. I will drop this hunk from the next version, need to think if it makes sense to refactor and if so, in which way.
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 4e45740331ee..6e28c2a3d223 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -1465,7 +1465,7 @@ int iio_device_register_sysfs_group(struct iio_dev *indio_dev, const struct attribute_group **new, **old = iio_dev_opaque->groups; unsigned int cnt = iio_dev_opaque->groupcounter; - new = krealloc(old, sizeof(*new) * (cnt + 2), GFP_KERNEL); + new = krealloc_array(old, cnt + 2, sizeof(*new), GFP_KERNEL); if (!new) return -ENOMEM; @@ -1483,13 +1483,13 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev) { struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); int i, ret = 0, attrcount, attrn, attrcount_orig = 0; + struct attribute **attrs, **attr, *clk = NULL; struct iio_dev_attr *p; - struct attribute **attr, *clk = NULL; /* First count elements in any existing group */ - if (indio_dev->info->attrs) { - attr = indio_dev->info->attrs->attrs; - while (*attr++ != NULL) + attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs : NULL; + if (attrs) { + for (attr = attrs; *attr; attr++) attrcount_orig++; } attrcount = attrcount_orig; @@ -1521,20 +1521,14 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev) if (clk) attrcount++; + /* Copy across original attributes, and point to original binary attributes */ iio_dev_opaque->chan_attr_group.attrs = - kcalloc(attrcount + 1, - sizeof(iio_dev_opaque->chan_attr_group.attrs[0]), - GFP_KERNEL); + krealloc_array(attrs, attrcount + 1, sizeof(*attrs), GFP_KERNEL); if (iio_dev_opaque->chan_attr_group.attrs == NULL) { ret = -ENOMEM; goto error_clear_attrs; } - /* Copy across original attributes, and point to original binary attributes */ if (indio_dev->info->attrs) { - memcpy(iio_dev_opaque->chan_attr_group.attrs, - indio_dev->info->attrs->attrs, - sizeof(iio_dev_opaque->chan_attr_group.attrs[0]) - *attrcount_orig); iio_dev_opaque->chan_attr_group.is_visible = indio_dev->info->attrs->is_visible; iio_dev_opaque->chan_attr_group.bin_attrs =