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

["NG, TZE YEE" <tze.yee.ng@altera.com>]

On 12/6/2026 3:54 pm, Greg Kroah-Hartman wrote:
> On Fri, Jun 12, 2026 at 03:43:44AM +0000, NG, TZE YEE wrote:
>> On 10/6/2026 2:07 pm, Greg Kroah-Hartman wrote:
>>> On Wed, Jun 10, 2026 at 01:47:44AM +0000, NG, TZE YEE wrote:
>>>> On 9/6/2026 6:13 pm, Greg Kroah-Hartman wrote:
>>>>> [Some people who received this message don't often get email from gregkh@linuxfoundation.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>>>
>>>>> On Tue, Jun 09, 2026 at 02:19:44AM -0700, tze.yee.ng@altera.com wrote:
>>>>>> From: Tze Yee Ng <tze.yee.ng@altera.com>
>>>>>>
>>>>>> 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@altera.com>
>>>>>> ---
>>>>>>     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.
>>>
>>> WARN_ON() will panic a box if it ever triggers, loosing all data, so
>>> please do not do that.  If this is something that can happen, handle it
>>> properly, don't just crash.
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> Hi Greg,
>>
>> Thanks for the review.
>>
>> I went through the code again. There are already checks for both kaddr
>> and chan to guard against invalid API use. The !chan check is mainly a
>> failsafe in case a caller misuses the API.
>>
>> Regarding WARN_ON(), thanks for catching that — I had added it to flag
>> the !chan case. Would it be acceptable if I split the checks into two
>> conditions and only warn on !chan?
>>
>> if (!chan) {
>> 	WARN_ON_ONCE(!chan);
> 
> again, never add new WARN_ON calls for something that can actually
> happen.  If it can never happen, there is no need to check for it.
> 
> thanks,
> 
> greg k-h
Hi Greg,

Alright, I will remove the WARN_ON calls. Also, I will remove the NULL 
checks from stratix10_svc_free_memory(). If a NULL is passed into a 
required API parameter the kernel crash immediately, so the bug is 
caught and fixed during development.

I will resend v2 with the fixes above and the cc: stable line for 
"Fixes:" tag.

Thanks,
Tze Yee