linux-kernel - Re: [PATCH v2] iio: dummy: iio_simple_dummy: check the return value of kstrdup()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20220115173803.417c0b8f@jic23-huawei>
Date:   Sat, 15 Jan 2022 17:38:03 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     xkernel.wang@...mail.com
Cc:     jic23@...23.retrosnub.co.uk, lars@...afoo.de,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] iio: dummy: iio_simple_dummy: check the return value
 of kstrdup()

On Fri, 17 Dec 2021 11:48:28 +0800
xkernel.wang@...mail.com wrote:

> From: Xiaoke Wang <xkernel.wang@...mail.com>
> 
> kstrdup() is also a memory allocation-related function, it return NULL
> when some memory errors happen. So it is better to check the return
> value of it so to catch the memory error in time. Besides, there should
> have a kfree() to clear up the allocation if we get a failure later in
> this function to prevent memory leak.
> 
> Signed-off-by: Xiaoke Wang <xkernel.wang@...mail.com>

Hi a few minor follow on comments. Sorry it took me so long to get back to
you on this. I wasn't rushing as I won't be picking up fixes until the merge
window is done and I can rebase on rc1.

> ---
> Changelog:
> 1. Clean up some error labels(error_ret & error_kzalloc), directly make
> them reflect what they are clearing up and return.
> 2. Clear up indio_dev->name on error path. I put that under label 
> `error_free_device`, as kfree(NULL) is safe. Or I may need to add an
> another label as the traget of `goto`.
> Note: Suggestions are from Jonathan Cameron <jic23@...23.retrosnub.co.uk>
> ---
>  drivers/iio/dummy/iio_simple_dummy.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/dummy/iio_simple_dummy.c b/drivers/iio/dummy/iio_simple_dummy.c
> index c0b7ef9..430c12a 100644
> --- a/drivers/iio/dummy/iio_simple_dummy.c
> +++ b/drivers/iio/dummy/iio_simple_dummy.c
> @@ -577,7 +577,7 @@ static struct iio_sw_device *iio_dummy_probe(const char *name)
>  	swd = kzalloc(sizeof(*swd), GFP_KERNEL);
>  	if (!swd) {
>  		ret = -ENOMEM;
> -		goto error_kzalloc;
> +		return ERR_PTR(ret);

		return ERR_PTR(-ENOMEM);

>  	}
>  	/*
>  	 * Allocate an IIO device.
> @@ -589,8 +589,9 @@ static struct iio_sw_device *iio_dummy_probe(const char *name)
>  	 */
>  	indio_dev = iio_device_alloc(parent, sizeof(*st));
>  	if (!indio_dev) {
> +		kfree(swd);
Ah. Not what I meant unfortunately. This should look something like.

	if (!indio_dev) {
		ret = -ENOMEM;
		goto error_free_swd;
	}	
With error_ret label renamed to error_free_swd to reflect where it is.

>  		ret = -ENOMEM;
> -		goto error_ret;
> +		return ERR_PTR(ret);
>  	}
>  
>  	st = iio_priv(indio_dev);
> @@ -616,6 +617,10 @@ static struct iio_sw_device *iio_dummy_probe(const char *name)
>  	 *    indio_dev->name = spi_get_device_id(spi)->name;
>  	 */
>  	indio_dev->name = kstrdup(name, GFP_KERNEL);
> +	if (!indio_dev->name) {
> +		ret = -ENOMEM;
> +		goto error_free_device;
> +	}
>  
>  	/* Provide description of available channels */
>  	indio_dev->channels = iio_dummy_channels;
> @@ -650,10 +655,9 @@ static struct iio_sw_device *iio_dummy_probe(const char *name)
>  error_unregister_events:
>  	iio_simple_dummy_events_unregister(indio_dev);
>  error_free_device:
> +	kfree(indio_dev->name);
>  	iio_device_free(indio_dev);
> -error_ret:
>  	kfree(swd);
> -error_kzalloc:
>  	return ERR_PTR(ret);
>  }
>