linux-kernel - Re: [PATCH v5 2/5] iio: consumers: copy/release available info from producer to fix race

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZyJHFp6vbQ7deLFs@black.fi.intel.com>
Date: Wed, 30 Oct 2024 16:47:50 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Matteo Martelli <matteomartelli3@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Alisa-Dariana Roman <alisa.roman@...log.com>,
	Christian Eggers <ceggers@...i.de>, Peter Rosin <peda@...ntia.se>,
	Paul Cercueil <paul@...pouillou.net>,
	Sebastian Reichel <sre@...nel.org>, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-mips@...r.kernel.org,
	linux-pm@...r.kernel.org
Subject: Re: [PATCH v5 2/5] iio: consumers: copy/release available info from
 producer to fix race

On Mon, Oct 21, 2024 at 02:54:15PM +0200, Matteo Martelli wrote:
> Consumers need to call the producer's read_avail_release_resource()
> callback after reading producer's available info. To avoid a race
> condition with the producer unregistration, change inkern
> iio_channel_read_avail() so that it copies the available info from the
> producer and immediately calls its release callback with info_exists
> locked.
> 
> Also, modify the users of iio_read_avail_channel_raw() and
> iio_read_avail_channel_attribute() to free the copied available buffers
> after calling these functions. To let users free the copied buffer with
> a cleanup pattern, also add a iio_read_avail_channel_attr_retvals()
> consumer helper that is equivalent to iio_read_avail_channel_attribute()
> but stores the available values in the returned variable.

...

> +static void dpot_dac_read_avail_release_res(struct iio_dev *indio_dev,
> +					    struct iio_chan_spec const *chan,
> +					    const int *vals, long mask)
> +{
> +	kfree(vals);
> +}
> +
>  static int dpot_dac_write_raw(struct iio_dev *indio_dev,
>  			      struct iio_chan_spec const *chan,
>  			      int val, int val2, long mask)
> @@ -125,6 +132,7 @@ static int dpot_dac_write_raw(struct iio_dev *indio_dev,
>  static const struct iio_info dpot_dac_info = {
>  	.read_raw = dpot_dac_read_raw,
>  	.read_avail = dpot_dac_read_avail,
> +	.read_avail_release_resource = dpot_dac_read_avail_release_res,
>  	.write_raw = dpot_dac_write_raw,
>  };

I have a problem with this approach. The issue is that we allocate
memory in one place and must clear it in another. This is not well
designed thingy in my opinion. I was thinking a bit of the solution and
at least these two comes to my mind:

1) having a special callback for .read_avail_with_copy (choose better
name) that will dump the data to the intermediate buffer and clean it
after all;

2) introduce a new type (or bit there), like IIO_AVAIL_LIST_ALLOC.

In any case it looks fragile and not scalable. I propose to drop this
and think again.

Yes, yes, I'm fully aware about the problem you are trying to solve and
agree on the report, I think this solution is not good enough.

-- 
With Best Regards,
Andy Shevchenko