On Fri, 21 Jul 2023 20:00:17 +0300
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> Let the krealloc_array() copy the original data and
> check for a multiplication overflow.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> ---
> 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 =
