linux-kernel - Re: [PATCH v2] iio: core: use kstrdup_const/kfree

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: <20210227174952.06f45503@archlinux>
Date:   Sat, 27 Feb 2021 17:49:52 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Alexandru Ardelean <ardeleanalex@...il.com>
Cc:     linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Alexandru Ardelean <alexandru.ardelean@...log.com>
Subject: Re: [PATCH v2] iio: core: use kstrdup_const/kfree_const for buffer
 attributes

On Tue, 23 Feb 2021 09:29:26 +0200
Alexandru Ardelean <ardeleanalex@...il.com> wrote:

> When the buffer attributes were wrapped in iio_dev_attr types, I forgot to
> duplicate the names, so that when iio_free_chan_devattr_list() gets called
> on cleanup, these get free'd.
> I stumbled over this while accidentally breaking a driver doing
> iio_device_register(), and then the issue appeared.
> 
> Some ways to fix this are:
> 1. Just use kstrdup() during iio_buffer_wrap_attr()
> 2. Just use kfree_const() during iio_free_chan_devattr_list
> 3. Use both kstrdup_const() & kfree_const() (in the places mentioned above)
> 
> This implements the third option, as it allows some users/drivers to
> specify some attributes allocated on the heap.
> 
> Fixes: a1a11142f66c ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> ---
>  drivers/iio/industrialio-buffer.c | 1 +
>  drivers/iio/industrialio-core.c   | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 5d641f8adfbd..ac882e60c419 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1306,6 +1306,7 @@ static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer,
>  		return NULL;
>  
>  	iio_attr->buffer = buffer;
> +	iio_attr->dev_attr.attr.name = kstrdup_const(attr->name, GFP_KERNEL);
>  	memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr));

Doesn't this wipe out the duplicated string?  I swapped the two lines above
which I think should avoid that.

Merged into original patch

Jonathan


>  
>  	list_add(&iio_attr->l, &buffer->buffer_attr_list);
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 0d8c6e88d993..cb2735d2ae4b 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1358,7 +1358,7 @@ void iio_free_chan_devattr_list(struct list_head *attr_list)
>  	struct iio_dev_attr *p, *n;
>  
>  	list_for_each_entry_safe(p, n, attr_list, l) {
> -		kfree(p->dev_attr.attr.name);
> +		kfree_const(p->dev_attr.attr.name);
>  		list_del(&p->l);
>  		kfree(p);
>  	}