linux-kernel - Re: [PATCH] regulator: tps65219: Fix devm

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: <67352ec8-6bd0-4434-8553-9fe7829ed188@ti.com>
Date: Fri, 20 Jun 2025 09:58:22 -0500
From: Andrew Davis <afd@...com>
To: Shree Ramamoorthy <s-ramamoorthy@...com>, <aaro.koskinen@....fi>,
        <andreas@...nade.info>, <khilman@...libre.com>, <rogerq@...nel.org>,
        <tony@...mide.com>, <lee@...nel.org>, <d-gole@...com>,
        <robertcnelson@...il.com>, <jkridner@...il.com>,
        <linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC: <m-leonard@...com>, <praneeth@...com>
Subject: Re: [PATCH] regulator: tps65219: Fix devm_kmalloc size allocation

On 6/19/25 7:09 PM, Shree Ramamoorthy wrote:
> In probe(), devm_kmalloc uses pmic->common_irq_size to allocate an array of
> 2 bytes, but pmic->common_irq_size is being used like an array of structs.

The issue is with both `common_irq_size` and `dev_irq_size`, so might be good
to mention both since you fix both in the patch. Also "2" bytes was an example,
to be more technically correct (the best kind of correct) you can say:

In probe(), two arrays of structs are allocated but the memory size of the
allocations were given as the arrays' length, not the total memory needed.

> The param sent should've been pmic->common_irq_size * sizeof(*irq_data).
> This led to an issue with the kmalloc'd buffer being corrupted and EVM boot

Don't need to mention "EVM" here unless you want to give a specific example
by name (PocketBeagle2 being the board that first exposed this issue IIRC).

Anyway, the issue wasn't just the kmalloc'd buffer being corrupted, but
other structures in the heap after, and since the issues caused by heap
overflows are usually random, easier to say:

This led to a heap overflow when the struct array was used.

> issues. The common and device-specific structs are now allocated one at a
> time within the loop.
> 

Acked-by: Andrew Davis <afd@...com>

> Fixes: 38c9f98db20a ("regulator: tps65219: Add support for TPS65215 Regulator IRQs")
> Reported-by: Dhruva Gole <d-gole@...com>
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@...com>
> ---
>   drivers/regulator/tps65219-regulator.c | 28 +++++++++++++-------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
> index d80749cdae1d..d77ca486879f 100644
> --- a/drivers/regulator/tps65219-regulator.c
> +++ b/drivers/regulator/tps65219-regulator.c
> @@ -436,46 +436,46 @@ static int tps65219_regulator_probe(struct platform_device *pdev)
>   					     pmic->rdesc[i].name);
>   	}
>   
> -	irq_data = devm_kmalloc(tps->dev, pmic->common_irq_size, GFP_KERNEL);
> -	if (!irq_data)
> -		return -ENOMEM;
> -
>   	for (i = 0; i < pmic->common_irq_size; ++i) {
>   		irq_type = &pmic->common_irq_types[i];
>   		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>   		if (irq < 0)
>   			return -EINVAL;
>   
> -		irq_data[i].dev = tps->dev;
> -		irq_data[i].type = irq_type;
> +		irq_data = devm_kmalloc(tps->dev, sizeof(*irq_data), GFP_KERNEL);
> +		if (!irq_data)
> +			return -ENOMEM;
> +
> +		irq_data->dev = tps->dev;
> +		irq_data->type = irq_type;
>   		error = devm_request_threaded_irq(tps->dev, irq, NULL,
>   						  tps65219_regulator_irq_handler,
>   						  IRQF_ONESHOT,
>   						  irq_type->irq_name,
> -						  &irq_data[i]);
> +						  irq_data);
>   		if (error)
>   			return dev_err_probe(tps->dev, error,
>   					     "Failed to request %s IRQ %d\n",
>   					     irq_type->irq_name, irq);
>   	}
>   
> -	irq_data = devm_kmalloc(tps->dev, pmic->dev_irq_size, GFP_KERNEL);
> -	if (!irq_data)
> -		return -ENOMEM;
> -
>   	for (i = 0; i < pmic->dev_irq_size; ++i) {
>   		irq_type = &pmic->irq_types[i];
>   		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>   		if (irq < 0)
>   			return -EINVAL;
>   
> -		irq_data[i].dev = tps->dev;
> -		irq_data[i].type = irq_type;
> +		irq_data = devm_kmalloc(tps->dev, sizeof(*irq_data), GFP_KERNEL);
> +		if (!irq_data)
> +			return -ENOMEM;
> +
> +		irq_data->dev = tps->dev;
> +		irq_data->type = irq_type;
>   		error = devm_request_threaded_irq(tps->dev, irq, NULL,
>   						  tps65219_regulator_irq_handler,
>   						  IRQF_ONESHOT,
>   						  irq_type->irq_name,
> -						  &irq_data[i]);
> +						  irq_data);
>   		if (error)
>   			return dev_err_probe(tps->dev, error,
>   					     "Failed to request %s IRQ %d\n",
> 
> base-commit: 5c8013ae2e86ec36b07500ba4cacb14ab4d6f728
> prerequisite-patch-id: cd76c901948780de20e932cf620806959e2177b1
> prerequisite-patch-id: e847098a38d07e5ff31e8c80d86d9702d132fdad
> prerequisite-patch-id: e6a01f111e527c6da442f6756f8daa4e79d0fa3c