-
Notifications
You must be signed in to change notification settings - Fork 11
[LTS 8.6] net/ulp: use consistent error code when blocking ULP #282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Just checked the
Perhaps the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥌
Thanks for the follow up of this and fixing the original backport. Since both of these are technically bug fixes to the CVE could you put the cve tag to :
This is more of a legacy tag information but we might be using it in the future. |
jira VULN-3655 cve-bf CVE-2023-0461 commit-author Paolo Abeni <[email protected]> commit 8ccc993 upstream-diff This commit is the closure of 68e4adc, solving two issues: 1. The backported mainline fix 2c02d41 had a follow-up in 8ccc993, which was missing from `ciqlts8_6'. (The original intent of the cherry-picked commit) 2. The way changes to `inet_csk_listen_start' were applied from upstream left a potential branching path which would result in the returned `err' different than before the change, for the exact same inputs. While effectively ignoring the initialization of `err' to `-EADDRINUSE' was justified in upstream because of the inevitable assignment at line 1237, the same cannot be done in the versions prior to 9.2 as the initial `-EADDRINUSE' can survive in `err' up to its returning from function. (The piggy-backed correction included here for the lack of better place) The referenced commit changed the error code returned by the kernel when preventing a non-established socket from attaching the ktls ULP. Before to such a commit, the user-space got ENOTCONN instead of EINVAL. The existing self-tests depend on such error code, and the change caused a failure: RUN global.non_established ... tls.c:1673:non_established:Expected errno (22) == ENOTCONN (107) non_established: Test failed at step ctrliq#3 FAIL global.non_established In the unlikely event existing applications do the same, address the issue by restoring the prior error code in the above scenario. Note that the only other ULP performing similar checks at init time - smc_ulp_ops - also fails with ENOTCONN when trying to attach the ULP to a non-established socket. Reported-by: Sabrina Dubroca <[email protected]> Fixes: 2c02d41 ("net/ulp: prevent ULP without clone op from entering the LISTEN status") Signed-off-by: Paolo Abeni <[email protected]> Reviewed-by: Sabrina Dubroca <[email protected]> Link: https://lore.kernel.org/r/7bb199e7a93317fb6f8bf8b9b2dc71c18f337cde.1674042685.git.pabeni@redhat.com Signed-off-by: Jakub Kicinski <[email protected]> (cherry picked from commit 8ccc993) Signed-off-by: Marcin Wcisło <[email protected]>
9c615be
to
90a11ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank You
[LTS 8.6]
CVE-2023-0461
VULN-3655
Problem
https://www.cve.org/CVERecord?id=CVE-2023-0461
Applicability
The
CONFIG_TLS
option mentioned as sufficient prerequisite for the bug is present in all configurations ofciqlts8_6
:However, CVE-2023-0461 has already been addressed for LTS 8.6 in the commit 68e4adc, so this specific vulnerability should not apply to
ciqlts8_6
anymore. This PR was created nevertheless to close some gaps and finish the patch. See below.Analysis and solution
Assessment
The patch is given in mainline in 2c02d41 commit. It was backported onto 7 stable branches which, along with the mainline, can be categorized into 3 groups based on their diffs
Solution A
Solution B
Solution C
Additionally, the patch is present in Rocky Linux for (at least) 4 versions LTS 8.6, LTS 8.8, LTS 9.2, LTS 9.4, contained in 3 commits, each representing a class in itself.
Solution D
Solution E
Solution F
The differences occur along four axes:
W:
inet_ulp_can_listen
function definition1: Lack of
icsk->icsk_ulp_ops->clone
in the condition check2: Inclusion of
icsk->icsk_ulp_ops->clone
in the condition checkX:
net/ipv4/tcp_ulp.c
file modification1: No modification
2: The
EINVAL
version3: The
ENOTCONN
versionY: Initialization of
err
ininet_csk_listen_start
1: De-initialization
From
to
2: Left as-is: uninitialized
3: Left as-is: initialized to
-EADDRINUSE
Z: Restoration of
err
to-EADDRINUSE
after checkinginet_ulp_can_listen
1: No
2: Yes
The patch points are spaced as follows:
Discussion
The W is somewhat correlated with X: upstream versions 4.14-5.4 simply lack the
clone
operation in ulp_ops, so it doesn't make sense to check for it and W1 goes with X1 for A - comparetcp_ulp_ops
definition from 755193f (4.19) with the same struct in f8ed0a9 (5.10). What's interesting, however, is that Rocky LTS 8.6, despite being 4.18, does contain theclone
ULP operation just like newer versions. (Perhaps it was backported by RedHat)Initialization of
err
(Y3) is pointless in any case where it occurs, as the variable is assignedinet_ulp_can_listen(sk)
in the very next line. That was probably the reason for choosing Y1 in official backports for pre-6.0 versions (groups A, B).The restoration of
err
to-EADDRINUSE
(axis Z), however, is not pointless in the pre-6.0 versions, including Rocky LTS 8.6 and LTS 8.8: a potential branching path exists which would result in the returnederr
different than before the change, for the exact same inputs. While effectively ignoring the initialization oferr
to-EADDRINUSE
was justified in upstream mainline because of the inevitable assignment at line 1237, the same cannot be done in the earlier versions as the initial-EADDRINUSE
can survive inerr
up to its return from the function.The use of
-ENOTCONN
instead of-EINVAL
(X2 vs X1) is the result of applying commit 8ccc993, which was a follow-up of the mainline CVE fix 2c02d41:Conclusions and solution
err
to-EADDRINUSE
(D: Z1 → Z2)just as it's done in LTS 8.8 (E: Z2). For the versions 9.2, 9.4 this is not required. Unless there is some reason going beyond the analysis local to theinet_csk_listen_start
function this should probably be done to groups A, B as well, although that's outside of scope.clone
operation checking is done properly in LTS 8.6 and should be left as it is, despite its deviation from the upstream backports.err
to-EADDRINUSE
in all Rocky versions 8.6-9.4 can be left as is - it's harmless.kABI check: passed
Boot test: passed
boot-test.log
Kselftests: passed relative
Coverage
android
,bpf
(excepttest_xsk.sh
,test_sockmap
,test_progs-no_alu32
,test_kmod.sh
,test_progs
),breakpoints
,capabilities
,core
,cpu-hotplug
,cpufreq
,efivarfs
,exec
,firmware
,fpu
,ftrace
,futex
,gpio
,intel_pstate
,ipc
,kcmp
,kexec
,kvm
,lib
,livepatch
,membarrier
,memfd
,memory-hotplug
,mount
,net/forwarding
(exceptmirror_gre_vlan_bridge_1q.sh
,tc_actions.sh
,sch_tbf_prio.sh
,sch_tbf_ets.sh
,sch_tbf_root.sh
,ipip_hier_gre_keys.sh
,mirror_gre_bridge_1d_vlan.sh
,sch_ets.sh
),net/mptcp
(exceptsimult_flows.sh
),net
(exceptudpgro_fwd.sh
,ip_defrag.sh
,txtimestamp.sh
,udpgso_bench.sh
,xfrm_policy.sh
,reuseport_addr_any.sh
,gro.sh
,reuseaddr_conflict
),netfilter
(exceptnft_trans_stress.sh
),nsfs
,proc
,pstore
,ptrace
,rseq
,sgx
,sigaltstack
,size
,splice
,static_keys
,tc-testing
,timens
,timers
(exceptraw_skew
),tpm2
,vm
,x86
,zram
Reference
kselftests–ciqlts8_6–run1.log
kselftests–ciqlts8_6–run2.log
kselftests–ciqlts8_6–run3.log
kselftests–ciqlts8_6–run4.log
Patch
kselftests–ciqlts8_6-CVE-2023-0461–run1.log
kselftests–ciqlts8_6-CVE-2023-0461–run2.log
kselftests–ciqlts8_6-CVE-2023-0461–run3.log
Comparison
The difference in test results showcase what was mentioned in the 8ccc993 commit - the
net:tls
test is now passing.Specific tests: skipped