linux-kernel - Re: [PATCH] regulator: max20086: fix invalid memory access

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: <e901a2f3-55fc-497a-9bcd-10d75b26990d@gmail.com>
Date: Thu, 8 May 2025 12:17:47 +0300
From: Cosmin Tanislav <demonsingur@...il.com>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Watson Chow <watson.chow@...et.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] regulator: max20086: fix invalid memory access



On 5/8/25 9:49 AM, Cosmin Tanislav wrote:
> max20086_parse_regulators_dt() calls of_regulator_match() using an
> array of struct of_regulator_match allocated on the stack for the
> matches argument.
> 
> of_regulator_match() calls devm_of_regulator_put_matches(), which calls
> devres_alloc() to allocate a struct devm_of_regulator_matches which will
> be de-allocated using devm_of_regulator_put_matches().
> 
> struct devm_of_regulator_matches is populated with the stack allocated
> matches array.
> 
> If the device fails to probe, devm_of_regulator_put_matches() will be
> called and will try to call of_node_put() on that stack pointer,
> generating the following dmesg entries:
> 
> max20086 6-0028: Failed to read DEVICE_ID reg: -121
> kobject: '\xc0$\xa5\x03' (000000002cebcb7a): is not initialized, yet
> kobject_put() is being called.
> 
> Followed by a stack trace matching the call flow described above.
> 
> Switch to allocating the matches array using devm_kcalloc() to
> avoid accessing the stack pointer long after it's out of scope.
> 
> This also has the advantage of allowing multiple max20086 to probe
> without overriding the data stored inside the global of_regulator_match.
> 

I've made an error, the above paragraph is not relevant here, although
it is relevant for other usages of of_regulator_match() where the struct
of_regulator_match is a global array, which causes the data stored in it
to be overridden if another device using the same driver probes again,
which could cause of_node_put() to be called twice on the same of_node.

I'm sure that's not a common issue since it hasn't been fixed yet.

> Fixes: bfff546aae50 ("regulator: Add MAX20086-MAX20089 driver")
> Signed-off-by: Cosmin Tanislav <demonsingur@...il.com>
> ---
>   drivers/regulator/max20086-regulator.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/max20086-regulator.c b/drivers/regulator/max20086-regulator.c
> index 59eb23d467ec..198d45f8e884 100644
> --- a/drivers/regulator/max20086-regulator.c
> +++ b/drivers/regulator/max20086-regulator.c
> @@ -132,7 +132,7 @@ static int max20086_regulators_register(struct max20086 *chip)
>   
>   static int max20086_parse_regulators_dt(struct max20086 *chip, bool *boot_on)
>   {
> -	struct of_regulator_match matches[MAX20086_MAX_REGULATORS] = { };
> +	struct of_regulator_match *matches;
>   	struct device_node *node;
>   	unsigned int i;
>   	int ret;
> @@ -143,6 +143,11 @@ static int max20086_parse_regulators_dt(struct max20086 *chip, bool *boot_on)
>   		return -ENODEV;
>   	}
>   
> +	matches = devm_kcalloc(chip->dev, chip->info->num_outputs,
> +			       sizeof(*matches), GFP_KERNEL);
> +	if (!matches)
> +		return -ENOMEM;
> +
>   	for (i = 0; i < chip->info->num_outputs; ++i)
>   		matches[i].name = max20086_output_names[i];
>