[PATCH] regulator: tps65219: Fix devm_kmalloc size allocation

[PATCH] regulator: tps65219: Fix devm_kmalloc size allocation

[Shree Ramamoorthy]

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 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
issues. The common and device-specific structs are now allocated one at a
time within the loop.

Fixes: 38c9f98db20a ("regulator: tps65219: Add support for TPS65215 Regulator IRQs")
Reported-by: Dhruva Gole <d-gole@ti.com>
Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.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
-- 
2.43.0

Re: [PATCH] regulator: tps65219: Fix devm_kmalloc size allocation

[Robert Nelson]

On Thu, Jun 19, 2025 at 7:09 PM Shree Ramamoorthy <s-ramamoorthy@ti.com> 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 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
> issues. The common and device-specific structs are now allocated one at a
> time within the loop.
>
> Fixes: 38c9f98db20a ("regulator: tps65219: Add support for TPS65215 Regulator IRQs")
> Reported-by: Dhruva Gole <d-gole@ti.com>
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>

Thanks Shree!  Starting testing on PB2/BeaglePlay's..


> base-commit: 5c8013ae2e86ec36b07500ba4cacb14ab4d6f728
> prerequisite-patch-id: cd76c901948780de20e932cf620806959e2177b1
> prerequisite-patch-id: e847098a38d07e5ff31e8c80d86d9702d132fdad
> prerequisite-patch-id: e6a01f111e527c6da442f6756f8daa4e79d0fa3c

ps, worked around these 3 missing in v6.16-rc2, which git tree do you
have them staged?

Regards,

-- 
Robert Nelson
https://rcn-ee.com/

Re: [PATCH] regulator: tps65219: Fix devm_kmalloc size allocation

[Robert Nelson]

On Fri, Jun 20, 2025 at 8:30 AM Robert Nelson <robertcnelson@gmail.com> wrote:
>
> On Thu, Jun 19, 2025 at 7:09 PM Shree Ramamoorthy <s-ramamoorthy@ti.com> 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 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
> > issues. The common and device-specific structs are now allocated one at a
> > time within the loop.
> >
> > Fixes: 38c9f98db20a ("regulator: tps65219: Add support for TPS65215 Regulator IRQs")
> > Reported-by: Dhruva Gole <d-gole@ti.com>
> > Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>

Sweet!  With 4 PB2's and 2 BeaglePlays... Power on, boot, Power off,
wait 30 seconds.. (repeat 10 times)

Before:
36/60: Bootup to File System

After
60/60: Bootup to File System

Tested-by: Robert Nelson <robertcnelson@gmail.com>

SInce i applied directly to rc2, here's my fixup of your patch for
reference: https://github.com/RobertCNelson/arm64-multiplatform/blob/v6.16.x-arm64-k3/patches/fixes/0001-regulator-tps65219-Fix-devm_kmalloc-size-allocation.patch

For TI, we also need to cherry pick this in v6.12.x-ti, to fix
BeaglePlay and PB2..

Thanks Everyone!

-- 
Robert Nelson
https://rcn-ee.com/

Re: [PATCH] regulator: tps65219: Fix devm_kmalloc size allocation

[Shree Ramamoorthy]

On 6/20/2025 9:52 AM, Robert Nelson wrote:
> On Fri, Jun 20, 2025 at 8:30 AM Robert Nelson <robertcnelson@gmail.com> wrote:
>> On Thu, Jun 19, 2025 at 7:09 PM Shree Ramamoorthy <s-ramamoorthy@ti.com> 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 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
>>> issues. The common and device-specific structs are now allocated one at a
>>> time within the loop.
>>>
>>> Fixes: 38c9f98db20a ("regulator: tps65219: Add support for TPS65215 Regulator IRQs")
>>> Reported-by: Dhruva Gole <d-gole@ti.com>
>>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> Sweet!  With 4 PB2's and 2 BeaglePlays... Power on, boot, Power off,
> wait 30 seconds.. (repeat 10 times)
>
> Before:
> 36/60: Bootup to File System
>
> After
> 60/60: Bootup to File System
>
> Tested-by: Robert Nelson <robertcnelson@gmail.com>
>
> SInce i applied directly to rc2, here's my fixup of your patch for
> reference: https://github.com/RobertCNelson/arm64-multiplatform/blob/v6.16.x-arm64-k3/patches/fixes/0001-regulator-tps65219-Fix-devm_kmalloc-size-allocation.patch
>
> For TI, we also need to cherry pick this in v6.12.x-ti, to fix
> BeaglePlay and PB2..
>
> Thanks Everyone!

Awesome! Glad to hear this resolved those boards :) I will backport this to ti-6.12y-cicd as well.

>

Re: [PATCH] regulator: tps65219: Fix devm_kmalloc size allocation

[Shree Ramamoorthy]

Hi Robert,

On 6/20/2025 8:30 AM, Robert Nelson wrote:
> On Thu, Jun 19, 2025 at 7:09 PM Shree Ramamoorthy <s-ramamoorthy@ti.com> 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 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
>> issues. The common and device-specific structs are now allocated one at a
>> time within the loop.
>>
>> Fixes: 38c9f98db20a ("regulator: tps65219: Add support for TPS65215 Regulator IRQs")
>> Reported-by: Dhruva Gole <d-gole@ti.com>
>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> Thanks Shree!  Starting testing on PB2/BeaglePlay's..
>
>
>> base-commit: 5c8013ae2e86ec36b07500ba4cacb14ab4d6f728
>> prerequisite-patch-id: cd76c901948780de20e932cf620806959e2177b1
>> prerequisite-patch-id: e847098a38d07e5ff31e8c80d86d9702d132fdad
>> prerequisite-patch-id: e6a01f111e527c6da442f6756f8daa4e79d0fa3c
> ps, worked around these 3 missing in v6.16-rc2, which git tree do you
> have them staged?

I have them staged in master, but those 3 commits (see links below) snuck in. 
I'll rebase this commit on a new branch & remove those for v2.
https://lore.kernel.org/all/aB3hiEM0CB8m_X8m@stanley.mountain/
https://lore.kernel.org/all/175034639178.919047.12885250485072078236.b4-ty@kernel.org/

>
> Regards,
>

Re: [PATCH] regulator: tps65219: Fix devm_kmalloc size allocation

[Andrew Davis]

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@ti.com>

> Fixes: 38c9f98db20a ("regulator: tps65219: Add support for TPS65215 Regulator IRQs")
> Reported-by: Dhruva Gole <d-gole@ti.com>
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.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

Re: [PATCH] regulator: tps65219: Fix devm_kmalloc size allocation

[Shree Ramamoorthy]

On 6/20/2025 9:58 AM, Andrew Davis wrote:
> 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@ti.com>

Thank you for reviewing!! I will update the cover letter & submit a v2.

>
>> Fixes: 38c9f98db20a ("regulator: tps65219: Add support for TPS65215
>> Regulator IRQs")
>> Reported-by: Dhruva Gole <d-gole@ti.com>
>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.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