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] [day] [month] [year] [list]
Message-ID: <7f5dc278-ff5f-4554-b490-49de23f1e1b0@ti.com>
Date: Fri, 20 Jun 2025 10:22:48 -0500
From: Shree Ramamoorthy <s-ramamoorthy@...com>
To: Andrew Davis <afd@...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/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@...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@...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