linux-kernel - Re: [PATCH v4 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: <20241019130414.014a6384@jic23-huawei>
Date: Sat, 19 Oct 2024 13:04:14 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Matteo Martelli <matteomartelli3@...il.com>
Cc: 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 v4 2/5] iio: consumers: copy/release available info from
 producer to fix race

On Fri, 18 Oct 2024 12:16:41 +0200
Matteo Martelli <matteomartelli3@...il.com> 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.
> 
> Signed-off-by: Matteo Martelli <matteomartelli3@...il.com>
Hi Matteo,

The cleanup.h stuff is new and comes with footguns. As such the
'rules' applied are perhaps stricter than then will be in the long term.
https://lore.kernel.org/all/172294149613.2215.3274492813920223809.tip-bot2@tip-bot2/
is what we have today. Particularly the last few paragraphs on usage.

> @@ -857,7 +879,7 @@ static int iio_channel_read_min(struct iio_channel *chan,
>  				int *val, int *val2, int *type,
>  				enum iio_chan_info_enum info)
>  {
> -	const int *vals;
> +	const int *vals __free(kfree) = NULL;

Unlike below this one is 'almost' ok because there isn't much below. However,
still not good because of the risk of future code putting something in between.
At very minimum move it down to just before the place it's allocated.

It's a bit messy but maybe what we need is:

int *iio_read_avail_channel_attribute_retvals(struct iio_channel *chan,
				     	      int *type, int *length,
				              enum iio_chan_info_enum attr)
{
	int *vals;
	int ret;

	ret = iio_read_avail_channel_attribute(chan, &vals, type, len, attr;
	if (ret)
		return ERR_PTR(ret);

	return vals;
}

Then you can do
	const int *vals __free(kfree) =
		iio_channel_read_avail_retvals(chan, type, &length, info);

	if (IS_ERR(vals))
		...

and obey the suggested style for cleanup.h usage.

Would need some clear comments on why it exists though!
(+ maybe a better name)


>  	int length;
>  	int ret;
>  

> diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c
> index 0a40f425c27723ccec49985b8b5e14a737b6a7eb..6b7856e69f5fb7b8b73166b9b6825f4af7b19129 100644
> --- a/drivers/power/supply/ingenic-battery.c
> +++ b/drivers/power/supply/ingenic-battery.c
> @@ -6,12 +6,14 @@
>   * based on drivers/power/supply/jz4740-battery.c
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/iio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/property.h>
> +#include <linux/slab.h>
>  
>  struct ingenic_battery {
>  	struct device *dev;
> @@ -62,7 +64,7 @@ static int ingenic_battery_get_property(struct power_supply *psy,
>   */
>  static int ingenic_battery_set_scale(struct ingenic_battery *bat)
>  {
> -	const int *scale_raw;
> +	const int *scale_raw __free(kfree) = NULL;
This isn't a good pattern as per the docs I just replied to v3 with.
Whilst the code is functionally correct today, it is fragile for the reasons
in those docs.

>  	int scale_len, scale_type, best_idx = -1, best_mV, max_raw, i, ret;
>  	u64 max_mV;
>