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, 29 Jul 2023 16:00:06 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Chenyuan Mi <michenyuan@huawei.com>
Cc:     <lars@metafoo.de>, <linux-iio@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V2] tools: iio: iio_generic_buffer: Fix some integer
 type and  calculation
Message-ID: <20230729160006.7352f957@jic23-huawei>
In-Reply-To: <20230725092407.62545-1-michenyuan@huawei.com>
References: <20230725092407.62545-1-michenyuan@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 25 Jul 2023 09:24:07 +0000
Chenyuan Mi <michenyuan@huawei.com> wrote:

> In function size_from_channelarray(), the return value 'bytes' is defined
> as int type. However, the calcution of 'bytes' in this function is designed
> to use the unsigned int type. So it is necessary to change 'bytes' type to
> unsigned int to avoid integer overflow.
> 
> The size_from_channelarray() is called in main() function, its return value
> is directly multipled by 'buf_len' and then used as the malloc() parameter.
> The 'buf_len' is completely controllable by user, thus a multiplication
> overflow may occur here. This could allocate an unexpected small area.
> 
> Signed-off-by: Chenyuan Mi <michenyuan@huawei.com>
> ---

Hi

There should be a change log here so we know what is different from v1.

I'm assuming it's mostly fixing up whatever made this not apply in v1.

Applied to the togreg branch of iio.git

Thanks,

Jonathan


>  tools/iio/iio_generic_buffer.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
> index f8deae4e26a1..44bbf80f0cfd 100644
> --- a/tools/iio/iio_generic_buffer.c
> +++ b/tools/iio/iio_generic_buffer.c
> @@ -51,9 +51,9 @@ enum autochan {
>   * Has the side effect of filling the channels[i].location values used
>   * in processing the buffer output.
>   **/
> -static int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
> +static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
>  {
> -	int bytes = 0;
> +	unsigned int bytes = 0;
>  	int i = 0;
>  
>  	while (i < num_channels) {
> @@ -348,7 +348,7 @@ int main(int argc, char **argv)
>  	ssize_t read_size;
>  	int dev_num = -1, trig_num = -1;
>  	char *buffer_access = NULL;
> -	int scan_size;
> +	unsigned int scan_size;
>  	int noevents = 0;
>  	int notrigger = 0;
>  	char *dummy;
> @@ -674,7 +674,16 @@ int main(int argc, char **argv)
>  	}
>  
>  	scan_size = size_from_channelarray(channels, num_channels);
> -	data = malloc(scan_size * buf_len);
> +
> +	size_t total_buf_len = scan_size * buf_len;
> +
> +	if (scan_size > 0 && total_buf_len / scan_size != buf_len) {
> +		ret = -EFAULT;
> +		perror("Integer overflow happened when calculate scan_size * buf_len");
> +		goto error;
> +	}
> +
> +	data = malloc(total_buf_len);
>  	if (!data) {
>  		ret = -ENOMEM;
>  		goto error;