Re: [PATCH] firmware: stratix10-svc: fix memory leaks and list corruption bugs

[NG, TZE YEE]

On 9/6/2026 6:13 pm, Greg Kroah-Hartman wrote:
> [Some people who received this message don't often get email from gregkh@xxxxxxxxxxxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Tue, Jun 09, 2026 at 02:19:44AM -0700, tze.yee.ng@xxxxxxxxxx wrote:
>> From: Tze Yee Ng <tze.yee.ng@xxxxxxxxxx>
>>
>> Fix a memory leak when gen_pool_alloc() fails by freeing pmem on the error
>> path. Switch pmem allocation from devm_kzalloc() to kzalloc() with
>> explicit kfree() in the free path to match its list-managed life time.
>> Remove the erroneous list_del(&svc_data_mem) which corrupted the list head
>> on failed lookups. Add NULL guards instratix10_svc_free_memory().
>>
>> Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver")
>>
>> Signed-off-by: Tze Yee Ng <tze.yee.ng@xxxxxxxxxx>
>> ---
>>   drivers/firmware/stratix10-svc.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
>> index 1ef65bf845fc..3b0e2b14180f 100644
>> --- a/drivers/firmware/stratix10-svc.c
>> +++ b/drivers/firmware/stratix10-svc.c
>> @@ -1912,14 +1912,16 @@ void *stratix10_svc_allocate_memory(struct stratix10_svc_chan *chan,
>>        struct gen_pool *genpool = chan->ctrl->genpool;
>>        size_t s = roundup(size, 1 << genpool->min_alloc_order);
>>
>> -     pmem = devm_kzalloc(chan->ctrl->dev, sizeof(*pmem), GFP_KERNEL);
>> +     pmem = kzalloc_obj(*pmem);
>>        if (!pmem)
>>                return ERR_PTR(-ENOMEM);
>>
>>        guard(mutex)(&svc_mem_lock);
>>        va = gen_pool_alloc(genpool, s);
>> -     if (!va)
>> +     if (!va) {
>> +             kfree(pmem);
>>                return ERR_PTR(-ENOMEM);
>> +     }
>>
>>        memset((void *)va, 0, s);
>>        pa = gen_pool_virt_to_phys(genpool, va);
>> @@ -1945,6 +1947,9 @@ EXPORT_SYMBOL_GPL(stratix10_svc_allocate_memory);
>>   void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
>>   {
>>        struct stratix10_svc_data_mem *pmem;
>> +
>> +     if (!chan || !kaddr)
>> +             return;
> 
> What if one is not NULL but the other is?  Will you not leak memory here
> now?
> 
> thanks,
> 
> greg k-h
Hi Greg,

Good catch on the asymmetric case. The guard is not meant to support 
callers passing one NULL and one non-NULL argument.

kaddr == NULL: no-op, nothing to free. The guard prevents the old bug 
where a failed lookup fell through to list_del(&svc_data_mem) and 
corrupted the list head.

chan == NULL with valid kaddr: would leak, but that is invalid API 
usage. The previous code would oops on chan->ctrl->genpool instead of 
freeing. In-tree callers always pass both valid pointers from 
stratix10_svc_allocate_memory().

If you prefer not to silently swallow misuse, I can change this to 
WARN_ON_ONCE(!chan || !kaddr) before returning, or drop the !chan check 
and only guard !kaddr so a NULL channel still faults on dereference per 
normal kernel API expectations.

Regards,
Tze Yee