linux-kernel - Re: [PATCH net-next] mptcp: pm: Fix undefined behavior in mptcp_remove_anno_list_by

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: <75cccf77-7a4e-4ef0-9dca-2f1903c4a7ca@kernel.org>
Date: Tue, 25 Mar 2025 15:49:21 +0100
From: Matthieu Baerts <matttbe@...nel.org>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: Mat Martineau <martineau@...nel.org>, Geliang Tang <geliang@...nel.org>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
 netdev@...r.kernel.org, mptcp@...ts.linux.dev, linux-kernel@...r.kernel.org,
 Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next] mptcp: pm: Fix undefined behavior in
 mptcp_remove_anno_list_by_saddr()

Hi Thorsten,

On 25/03/2025 13:51, Thorsten Blum wrote:
> On 25. Mar 2025, at 13:30, Jakub Kicinski wrote:
>> On Tue, 25 Mar 2025 12:33:11 +0100 Thorsten Blum wrote:
>>> On 25. Mar 2025, at 12:06, Thorsten Blum wrote:
>>>>
>>>> Commit e4c28e3d5c090 ("mptcp: pm: move generic PM helpers to pm.c")
>>>> removed a necessary if-check, leading to undefined behavior because
>>>> the freed pointer is subsequently returned from the function.
>>>>
>>>> Reintroduce the if-check to fix this and add a local return variable to
>>>> prevent further checkpatch warnings, which originally led to the removal
>>>> of the if-check.
>>>>
>>>> Fixes: e4c28e3d5c090 ("mptcp: pm: move generic PM helpers to pm.c")
>>>> Signed-off-by: Thorsten Blum <thorsten.blum@...ux.dev>
>>>> ---  
>>>
>>> Never mind, technically it's not actually undefined behavior because of
>>> the implicit bool conversion, but returning a freed pointer still seems
>>> confusing.
>>
>> CCing the list back in.
> 
> Thanks!
> 
> The change imo still makes sense, but the commit message should be
> updated.

Yes, I think it still makes sense, and I understand the confusion.
Another reason to change that is to avoid future issues when kfree()
will be able to set variables to NULL [1].

Instead of re-introducing the if-statement, why not assigning 'ret' to
'entry'?

  entry = mptcp_pm_del_add_timer(...);
  ret = entry; // or assign it at the previous line? ret = entry = (...)
  kfree(entry);

[1] https://lore.kernel.org/20250321202620.work.175-kees@kernel.org

> I'll submit a new patch for after the merge window.
Thank you!

An alternative if you want to send it before is to rebase it on top of
the MPTCP "export" branch, and to send it only to the MPTCP ML. I can
apply the new version in our tree, and send it to Netdev later with
other patches.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.