Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 22 Jul 2023 18:28:20 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
        Lars-Peter Clausen <lars@metafoo.de>,
        Nuno Sa <nuno.sa@analog.com>
Subject: Re: [PATCH v2 3/8] iio: core: Switch to krealloc_array()
Message-ID: <20230722182820.28fb3115@jic23-huawei>
In-Reply-To: <20230721170022.3461-4-andriy.shevchenko@linux.intel.com>
References: <20230721170022.3461-1-andriy.shevchenko@linux.intel.com>
        <20230721170022.3461-4-andriy.shevchenko@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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 =