[PATCH net-next] mptcp: pm: Fix undefined behavior in mptcp_remove_anno_list_by_saddr()

[PATCH net-next] mptcp: pm: Fix undefined behavior in mptcp_remove_anno_list_by_saddr()

[Thorsten Blum]

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@linux.dev>
---
 net/mptcp/pm.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 18b19dbccbba..5a6d5e4897dd 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -151,10 +151,15 @@ bool mptcp_remove_anno_list_by_saddr(struct mptcp_sock *msk,
 				     const struct mptcp_addr_info *addr)
 {
 	struct mptcp_pm_add_entry *entry;
+	bool ret = false;
 
 	entry = mptcp_pm_del_add_timer(msk, addr, false);
-	kfree(entry);
-	return entry;
+	if (entry) {
+		ret = true;
+		kfree(entry);
+	}
+
+	return ret;
 }
 
 bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk)

Re: [PATCH net-next] mptcp: pm: Fix undefined behavior in mptcp_remove_anno_list_by_saddr()

[Jakub Kicinski]

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@linux.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.
-- 
pw-bot: cr

Re: [PATCH net-next] mptcp: pm: Fix undefined behavior in mptcp_remove_anno_list_by_saddr()

[Thorsten Blum]

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@linux.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. I'll submit a new patch for after the merge window.

Re: [PATCH net-next] mptcp: pm: Fix undefined behavior in mptcp_remove_anno_list_by_saddr()

[Matthieu Baerts]

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@linux.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.