mptcp: cope racing subflow creation in mptcp_rcv_space_adjust #175

jainanmol84 · 2025-03-25T19:57:37Z

Commit Message Requirements
Built against Vault/LTS Environment
kABI Check Passed, where Valid (Pre 9.4 RT does not have kABI stability)
Boot Test
Kernel SelfTest results
Additional Tests as determined relevant

Commit message

jira VULN-46444
cve CVE-2024-53122
commit-author Paolo Abeni <[email protected]>
commit ce7356ae35943cc6494cc692e62d51a734062b7d

Additional active subflows - i.e. created by the in kernel path manager - are included into the subflow list before starting the 3whs.

A racing recvmsg() spooling data received on an already established subflow would unconditionally call tcp_cleanup_rbuf() on all the current subflows, potentially hitting a divide by zero error on the newly created ones.

Explicitly check that the subflow is in a suitable state before invoking tcp_cleanup_rbuf().

Fixes: c76c6956566f ("mptcp: call tcp_cleanup_rbuf on subflows")
	Signed-off-by: Paolo Abeni <[email protected]>
	Reviewed-by: Matthieu Baerts (NGI0) <[email protected]>
Link: https://patch.msgid.link/02374660836e1b52afc91966b7535c8c5f7bafb0.1731060874.git.pabeni@redhat.com
	Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit ce7356ae35943cc6494cc692e62d51a734062b7d)
	Signed-off-by: Anmol Jain <[email protected]>

Kernel build logs

kernel-build.log

/home/anmol/kernel-src-tree
no .config file found, moving on
[TIMER]{MRPROPER}: 0s
x86_64 architecture detected, copying config
'configs/kernel-x86_64-rhel.config' -> '.config'
Setting Local Version for build
CONFIG_LOCALVERSION="-_ajain__ciqlts9_4-201ea9b33"
Making olddefconfig
  HOSTCC  scripts/basic/fixdep
  HOSTCC  scripts/kconfig/conf.o
  HOSTCC  scripts/kconfig/confdata.o
  HOSTCC  scripts/kconfig/expr.o
  LEX     scripts/kconfig/lexer.lex.c
  YACC    scripts/kconfig/parser.tab.[ch]
  HOSTCC  scripts/kconfig/lexer.lex.o
  HOSTCC  scripts/kconfig/menu.o
  HOSTCC  scripts/kconfig/parser.tab.o
  HOSTCC  scripts/kconfig/preprocess.o
  HOSTCC  scripts/kconfig/symbol.o
  HOSTCC  scripts/kconfig/util.o
  HOSTLD  scripts/kconfig/conf
#
# configuration written to .config
#
Starting Build
  SYSHDR  arch/x86/include/generated/uapi/asm/unistd_32.h
  SYSHDR  arch/x86/include/generated/uapi/asm/unistd_64.h
  SYSHDR  arch/x86/include/generated/uapi/asm/unistd_x32.h
  SYSTBL  arch/x86/include/generated/asm/syscalls_32.h
  SYSHDR  arch/x86/include/generated/asm/unistd_32_ia32.h
  SYSHDR  arch/x86/include/generated/asm/unistd_64_x32.h
  WRAP    arch/x86/include/generated/uapi/asm/bpf_perf_event.h
  WRAP    arch/x86/include/generated/uapi/asm/errno.h
  WRAP    arch/x86/include/generated/uapi/asm/fcntl.h
  WRAP    arch/x86/include/generated/uapi/asm/ioctl.h
  [---snip---]
INSTALL /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+/kernel/sound/xen/snd_xen_front.ko
  STRIP   /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+/kernel/sound/xen/snd_xen_front.ko
  SIGN    /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+/kernel/sound/xen/snd_xen_front.ko
  INSTALL /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+/kernel/virt/lib/irqbypass.ko
  STRIP   /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+/kernel/virt/lib/irqbypass.ko
  SIGN    /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+/kernel/virt/lib/irqbypass.ko
  DEPMOD  /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+
[TIMER]{MODULES}: 42s
Making Install
sh ./arch/x86/boot/install.sh 5.14.0-_ajain__ciqlts9_4-201ea9b33+ \
        arch/x86/boot/bzImage System.map "/boot"
[TIMER]{INSTALL}: 31s
Checking kABI
kABI check passed
Setting Default Kernel to /boot/vmlinuz-5.14.0-_ajain__ciqlts9_4-201ea9b33+ and Index to 2
The default is /boot/loader/entries/c09ae7f16c7843a2947e0b4b0c2c6380-5.14.0-_ajain__ciqlts9_4-201ea9b33+.conf with index 2 and kernel /boot/vmlinuz-5.14.0-_ajain__ciqlts9_4-201ea9b33+
The default is /boot/loader/entries/c09ae7f16c7843a2947e0b4b0c2c6380-5.14.0-_ajain__ciqlts9_4-201ea9b33+.conf with index 2 and kernel /boot/vmlinuz-5.14.0-_ajain__ciqlts9_4-201ea9b33+
Generating grub configuration file ...
Adding boot menu entry for UEFI Firmware Settings ...
done
Hopefully Grub2.0 took everything ... rebooting after time metrices
[TIMER]{MRPROPER}: 0s
[TIMER]{BUILD}: 2106s
[TIMER]{MODULES}: 42s
[TIMER]{INSTALL}: 31s
[TIMER]{TOTAL} 2185s
Rebooting in 10 seconds

kernel-build.log

Kselftests

$ grep '^ok ' kselftest-before.log | wc -l && grep '^ok ' kselftest-after.log | wc -l
361
361
grep '^not ok ' kselftest-before.log | wc -l && grep '^not ok ' kselftest-after.log | wc -l
76
76

kselftest-after.log
kselftest-before.log

jira VULN-46444 cve CVE-2024-53122 commit-author Paolo Abeni <[email protected]> commit ce7356a Additional active subflows - i.e. created by the in kernel path manager - are included into the subflow list before starting the 3whs. A racing recvmsg() spooling data received on an already established subflow would unconditionally call tcp_cleanup_rbuf() on all the current subflows, potentially hitting a divide by zero error on the newly created ones. Explicitly check that the subflow is in a suitable state before invoking tcp_cleanup_rbuf(). Fixes: c76c695 ("mptcp: call tcp_cleanup_rbuf on subflows") Signed-off-by: Paolo Abeni <[email protected]> Reviewed-by: Matthieu Baerts (NGI0) <[email protected]> Link: https://patch.msgid.link/02374660836e1b52afc91966b7535c8c5f7bafb0.1731060874.git.pabeni@redhat.com Signed-off-by: Jakub Kicinski <[email protected]> (cherry picked from commit ce7356a) Signed-off-by: Anmol Jain <[email protected]>

thefossguy-ciq · 2025-03-26T10:03:53Z

Great first PR! I noticed a few things, please rectify them.

The kernel-build.log file is not attached.
The kselftest-after.log file is not uploaded. Doesn't link to anything.
There are two kselftest-before.log file links. The first link is to a PR and not to a file.
The build log shows that the kABI check passed but you forgot to tick the check box for the kABI check (this is at the very beginning of the PR text).

You can edit the PR and,

Upload/attach the kernel-build.log file.
Remove the last few lines for kselftest-*.log files and re-upload them.
Add an x between the [ ] brackets in the kABI Check Passed, where Valid (Pre 9.4 RT does not have kABI stability) list item.

jainanmol84 · 2025-03-26T11:04:59Z

Great first PR! I noticed a few things, please rectify them.

The kernel-build.log file is not attached.

The kselftest-after.log file is not uploaded. Doesn't link to anything.

There are two kselftest-before.log file links. The first link is to a PR and not to a file.

The build log shows that the kABI check passed but you forgot to tick the check box for the kABI check (this is at the very beginning of the PR text).

You can edit the PR and,

Upload/attach the kernel-build.log file.

Remove the last few lines for kselftest-*.log files and re-upload them.

Add an x between the [ ] brackets in the kABI Check Passed, where Valid (Pre 9.4 RT does not have kABI stability) list item.

kernel-build.log file is already there - mptcp: cope racing subflow creation in mptcp_rcv_space_adjust #175
I've updated kselftest-after.log` file. There was a syntax mistake.

thefossguy-ciq

🚤

bmastbergen

Nice work! @jainanmol84 🥌

PlaidCat · 2025-03-26T15:08:30Z

I looked at this and got a merge conflict on the fuzz above the conflicted change.

 tcp_sk(ssk)->window_clamp = window_clamp;
vs
 WRITE_ONCE(tcp_sk(ssk)->window_clamp, window_clamp);

Because the ssk data is used in both lines I investigated why they added the annotations:
torvalds/linux@f410cbe

Since it looks like they're exclusively referring to window_clamp and data race I didn't investigate to see if we should bring in this commit for annotation as it would require us to validate that all the assurances that that ssk->window_clam assertions are true that it can be read locklessly, as the sk_rcvbuf has these annotations without window_clamp.

I'm not going to block this for an update since we don't have a codified method of dealing with conflicts in surrounding code but usually if the code in conflict shares the same variables / symbols we might want to mention it, even if its not in the commit message and what we learned.

Maybe we add this to the PR as like an due diligence check, I could open a discussion on an Issue for public committers to
weigh in?

bmastbergen · 2025-03-26T20:09:22Z

I'm not going to block this for an update since we don't have a codified method of dealing with conflicts in surrounding code but usually if the code in conflict shares the same variables / symbols we might want to mention it, even if its not in the commit message and what we learned.

Maybe we add this to the PR as like an due diligence check, I could open a discussion on an Issue for public committers to weigh in?

Mentioning merge conflicts for surrounding code in the PR seems like a good idea. I've typically not mentioned them in the 'upstream-diff' section of the commit message with the justification that they technically aren't a difference from the code that was changed in the upstream commit. However I do look at those surrounding conflicts and make a determination about whether they are relevant or not. Not mentioning them at all might prevent the case where a reviewer makes a different determination as to their relevance than I did.

In this particular case I also noticed the surrounding difference and also determined that I didn't think they were relevant to the changeset under review.

TLDR; For backported code, let's mention merge conflicts in the code surrounding the code change under review at least in the PR, or even in an 'upstream-diff' commit log section if you think it might be useful to a future commit log reader.

Merge away @jainanmol84

solardiz · 2025-03-26T23:51:09Z

I looked at this and got a merge conflict on the fuzz above the conflicted change.
 tcp_sk(ssk)->window_clamp = window_clamp;
vs
 WRITE_ONCE(tcp_sk(ssk)->window_clamp, window_clamp);

FWIW, the upstream stable backport to 6.1.119 mentioned in https://lore.kernel.org/linux-cve-announce/2024120252-CVE-2024-53122-f35c@gregkh/T/#u also has this without the WRITE_ONCE, so at least we won't be alone with this combination (of no WRITE_ONCE addition and this fix):

$ git show 24995851d58c --pretty=fuller
commit 24995851d58c4a205ad0ffa7b2f21e479a9c8527
Author:     Paolo Abeni <[email protected]>
AuthorDate: Tue Nov 19 09:35:49 2024 +0100
Commit:     Greg Kroah-Hartman <[email protected]>
CommitDate: Fri Nov 22 15:37:33 2024 +0100

    mptcp: cope racing subflow creation in mptcp_rcv_space_adjust
    
    commit ce7356ae35943cc6494cc692e62d51a734062b7d upstream.
    
    Additional active subflows - i.e. created by the in kernel path
    manager - are included into the subflow list before starting the
    3whs.
    
    A racing recvmsg() spooling data received on an already established
    subflow would unconditionally call tcp_cleanup_rbuf() on all the
    current subflows, potentially hitting a divide by zero error on
    the newly created ones.
    
    Explicitly check that the subflow is in a suitable state before
    invoking tcp_cleanup_rbuf().
    
    Fixes: c76c6956566f ("mptcp: call tcp_cleanup_rbuf on subflows")
    Signed-off-by: Paolo Abeni <[email protected]>
    Reviewed-by: Matthieu Baerts (NGI0) <[email protected]>
    Link: https://patch.msgid.link/02374660836e1b52afc91966b7535c8c5f7bafb0.1731060874.git.pabeni@redhat.com
    Signed-off-by: Jakub Kicinski <[email protected]>
    [ Conflicts in protocol.c, because commit f410cbea9f3d ("tcp: annotate
      data-races around tp->window_clamp") has not been backported to this
      version. The conflict is easy to resolve, because only the context is
      different, but not the line to modify. ]
    Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
    Signed-off-by: Greg Kroah-Hartman <[email protected]>

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 78ac5c538e13..1acd4e37a0ea 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2057,7 +2057,8 @@ static void mptcp_rcv_space_adjust(struct mptcp_sock *msk, int copied)
                                slow = lock_sock_fast(ssk);
                                WRITE_ONCE(ssk->sk_rcvbuf, rcvbuf);
                                tcp_sk(ssk)->window_clamp = window_clamp;
-                               tcp_cleanup_rbuf(ssk, 1);
+                               if (tcp_can_send_ack(ssk))
+                                       tcp_cleanup_rbuf(ssk, 1);
                                unlock_sock_fast(ssk, slow);
                        }
                }

What's important is to not try backporting this fix to kernels that don't have the commit listed in Fixes. According to RH, they fixed this even for EL8, so perhaps they had backported the buggy commit to there - but this doesn't tell us when they did that. We may have 8.x LTS branches that don't have that, so need to be careful to check first.

solardiz · 2025-03-26T23:54:15Z

TLDR; For backported code, let's mention merge conflicts in the code surrounding the code change under review at least in the PR, or even in an 'upstream-diff' commit log section if you think it might be useful to a future commit log reader.

Here's how the same conflict is mentioned in the upstream stable commit I quoted above:

    [ Conflicts in protocol.c, because commit f410cbea9f3d ("tcp: annotate
      data-races around tp->window_clamp") has not been backported to this
      version. The conflict is easy to resolve, because only the context is
      different, but not the line to modify. ]

thefossguy-ciq · 2025-03-27T09:29:01Z

Re: Merge conflict

This PR exists as the result of me helping Amol get familiar with the Kernel CVE process by making her fix one CVE and assisting in that. I focused on the process of it and not much on the CVE fix itself because I believe that Anmol can/should handle that, given her experience with userspace CVEs.

Given that the issue was introduced with v5.10, when we were presented with a merge conflict, I suggested that we pull in the patch from v5.15--the closest longterm tree. The commit's message is generated by our cherry pick script but the actual diff is from the patch merged in the v5.15 tree: https://web.git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/net/mptcp/protocol.c?h=v5.15.179&id=0a9a182ea5c7bb0374e527130fd85024ace7279b

+1 on mentioning if there is a merge conflict and the engineer's solution for it.

PlaidCat · 2025-03-27T15:10:50Z

As long as we always use the Upstream Commit SHA but you can see that RedHat made changes from the 5.10. so a different line would have been in conflict but ultimately still in contextual information.

I have updates I need to do to the WIKI anyways to remove RT stuff so maybe I'll include that if everyone generally agrees.

Commit <5518f239aff1> ("iommu/vt-d: Move scalable mode ATS enablement to probe path") changed the PCI ATS enablement logic to run earlier, specifically before the default domain attachment. On some client platforms, this change resulted in boot failures, causing the kernel to panic with the following message and call trace: Kernel panic - not syncing: DMAR hardware is malfunctioning CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc3+ #175 Call Trace: <TASK> dump_stack_lvl+0x6f/0xb0 dump_stack+0x10/0x16 panic+0x10a/0x2b7 iommu_enable_translation.cold+0xc/0xc intel_iommu_init+0xe39/0xec0 ? trace_hardirqs_on+0x1e/0xd0 ? __pfx_pci_iommu_init+0x10/0x10 pci_iommu_init+0xd/0x40 do_one_initcall+0x5b/0x390 kernel_init_freeable+0x26d/0x2b0 ? __pfx_kernel_init+0x10/0x10 kernel_init+0x15/0x120 ret_from_fork+0x35/0x60 ? __pfx_kernel_init+0x10/0x10 ret_from_fork_asm+0x1a/0x30 RIP: 1f0f:0x0 Code: Unable to access opcode bytes at 0xffffffffffffffd6. RSP: 0000:0000000000000000 EFLAGS: 841f0f2e66 ORIG_RAX: 1f0f2e6600000000 RAX: 0000000000000000 RBX: 1f0f2e6600000000 RCX: 2e66000000000084 RDX: 0000000000841f0f RSI: 000000841f0f2e66 RDI: 00841f0f2e660000 RBP: 00841f0f2e660000 R08: 00841f0f2e660000 R09: 000000841f0f2e66 R10: 0000000000841f0f R11: 2e66000000000084 R12: 000000841f0f2e66 R13: 0000000000841f0f R14: 2e66000000000084 R15: 1f0f2e6600000000 </TASK> ---[ end Kernel panic - not syncing: DMAR hardware is malfunctioning ]--- Fix this by reverting the timing change for ATS enablement introduced by the offending commit and restoring the previous behavior. Fixes: 5518f23 ("iommu/vt-d: Move scalable mode ATS enablement to probe path") Reported-by: Jarkko Nikula <[email protected]> Closes: https://lore.kernel.org/linux-iommu/[email protected]/ Signed-off-by: Lu Baolu <[email protected]> Tested-by: Jarkko Nikula <[email protected]> Reviewed-by: Kevin Tian <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Joerg Roedel <[email protected]>

commit 4f1492e Author: Lu Baolu <[email protected]> Date: Wed Apr 16 15:36:08 2025 +0800 iommu/vt-d: Revert ATS timing change to fix boot failure Commit <5518f239aff1> ("iommu/vt-d: Move scalable mode ATS enablement to probe path") changed the PCI ATS enablement logic to run earlier, specifically before the default domain attachment. On some client platforms, this change resulted in boot failures, causing the kernel to panic with the following message and call trace: Kernel panic - not syncing: DMAR hardware is malfunctioning CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc3+ #175 Call Trace: <TASK> dump_stack_lvl+0x6f/0xb0 dump_stack+0x10/0x16 panic+0x10a/0x2b7 iommu_enable_translation.cold+0xc/0xc intel_iommu_init+0xe39/0xec0 ? trace_hardirqs_on+0x1e/0xd0 ? __pfx_pci_iommu_init+0x10/0x10 pci_iommu_init+0xd/0x40 do_one_initcall+0x5b/0x390 kernel_init_freeable+0x26d/0x2b0 ? __pfx_kernel_init+0x10/0x10 kernel_init+0x15/0x120 ret_from_fork+0x35/0x60 ? __pfx_kernel_init+0x10/0x10 ret_from_fork_asm+0x1a/0x30 RIP: 1f0f:0x0 Code: Unable to access opcode bytes at 0xffffffffffffffd6. RSP: 0000:0000000000000000 EFLAGS: 841f0f2e66 ORIG_RAX: 1f0f2e6600000000 RAX: 0000000000000000 RBX: 1f0f2e6600000000 RCX: 2e66000000000084 RDX: 0000000000841f0f RSI: 000000841f0f2e66 RDI: 00841f0f2e660000 RBP: 00841f0f2e660000 R08: 00841f0f2e660000 R09: 000000841f0f2e66 R10: 0000000000841f0f R11: 2e66000000000084 R12: 000000841f0f2e66 R13: 0000000000841f0f R14: 2e66000000000084 R15: 1f0f2e6600000000 </TASK> ---[ end Kernel panic - not syncing: DMAR hardware is malfunctioning ]--- Fix this by reverting the timing change for ATS enablement introduced by the offending commit and restoring the previous behavior. Fixes: 5518f23 ("iommu/vt-d: Move scalable mode ATS enablement to probe path") Reported-by: Jarkko Nikula <[email protected]> Closes: https://lore.kernel.org/linux-iommu/[email protected]/ Signed-off-by: Lu Baolu <[email protected]> Tested-by: Jarkko Nikula <[email protected]> Reviewed-by: Kevin Tian <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Joerg Roedel <[email protected]> (cherry picked from commit 4f1492e) Upstream-Status: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git Signed-off-by: Jerry Snitselaar <[email protected]> JIRA: https://issues.redhat.com/browse/RHEL-89891

thefossguy-ciq approved these changes Mar 26, 2025

View reviewed changes

bmastbergen self-requested a review March 26, 2025 14:00

bmastbergen approved these changes Mar 26, 2025

View reviewed changes

solardiz approved these changes Mar 26, 2025

View reviewed changes

jainanmol84 merged commit c58cea1 into ciqlts9_4 Mar 27, 2025
4 checks passed

jainanmol84 deleted the {ajain}_ciqlts9_4 branch March 27, 2025 15:54

PlaidCat mentioned this pull request Mar 28, 2025

[FIPS 9.2] mptcp CVEs {CVE-2024-57882,CVE-2024-53122} + bugfix #181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

mptcp: cope racing subflow creation in mptcp_rcv_space_adjust #175

mptcp: cope racing subflow creation in mptcp_rcv_space_adjust #175

Uh oh!

jainanmol84 commented Mar 25, 2025 •

edited

Loading

Uh oh!

thefossguy-ciq commented Mar 26, 2025

Uh oh!

jainanmol84 commented Mar 26, 2025

Uh oh!

thefossguy-ciq left a comment

Uh oh!

bmastbergen left a comment

Uh oh!

PlaidCat commented Mar 26, 2025

Uh oh!

bmastbergen commented Mar 26, 2025

Uh oh!

solardiz commented Mar 26, 2025

Uh oh!

solardiz commented Mar 26, 2025

Uh oh!

thefossguy-ciq commented Mar 27, 2025

Uh oh!

PlaidCat commented Mar 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

mptcp: cope racing subflow creation in mptcp_rcv_space_adjust #175

mptcp: cope racing subflow creation in mptcp_rcv_space_adjust #175

Uh oh!

Conversation

jainanmol84 commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Commit message

Kernel build logs

Kselftests

Uh oh!

thefossguy-ciq commented Mar 26, 2025

Uh oh!

jainanmol84 commented Mar 26, 2025

Uh oh!

thefossguy-ciq left a comment

Choose a reason for hiding this comment

Uh oh!

bmastbergen left a comment

Choose a reason for hiding this comment

Uh oh!

PlaidCat commented Mar 26, 2025

Uh oh!

bmastbergen commented Mar 26, 2025

Uh oh!

solardiz commented Mar 26, 2025

Uh oh!

solardiz commented Mar 26, 2025

Uh oh!

thefossguy-ciq commented Mar 27, 2025

Uh oh!

PlaidCat commented Mar 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

jainanmol84 commented Mar 25, 2025 •

edited

Loading