[LTS 8.6] net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve

[pvts-mat]

[LTS 8.6]
CVE-2023-4623
VULN-4106

Problem

https://www.cve.org/CVERecord?id=CVE-2023-4623

A use-after-free vulnerability in the Linux kernel's net/sched: sch_hfsc (HFSC qdisc traffic control) component can be exploited to achieve local privilege escalation. If a class with a link-sharing curve (i.e. with the HFSC_FSC flag set) has a parent without a link-sharing curve, then init_vf() will call vttree_insert() on the parent, but vttree_remove() will be skipped in update_vf(). This leaves a dangling pointer that can cause a use-after-free.

Analysis and solution

A single commit was identified as a fix for this issue: b3d26c5702c7d6c45456326e56d2ccf3f103e60f net/sched: sch_hfsc: Ensure inner classes have fsc curve.

The solution consisted of rejecting the addition of a class with a link-sharing curve to the class without it (see Specific tests for details):

[root@ciqlts8_6-rt pvts]# (
    tc qdisc add dev lo root handle 1: hfsc default 10
    tc class add dev lo parent 1: classid 1:1 hfsc rt m2 100kbps
    tc class add dev lo parent 1:1 classid 1:10 hfsc ls m2 50kbps
    echo $?
)

Error: Invalid parent - parent class must have FSC.
2

The fix introduced a problem with existing network setup scripts for some users https://lore.kernel.org/all/297D84E3-736E-4AB4-B825-264279E2043C@flyingcircus.io/:

I upgraded from 6.1.38 to 6.1.55 this morning and it broke my traffic shaping script, leaving me with a non-functional uplink on a remote router.

It was decided to fix the problem without breaking backwards compatibility https://lore.kernel.org/all/20231013151057.2611860-1-pctammela@mojatatu.com/:

Setting 'rt' as a parent is incorrect and the man page is explicit about
it as it doesn't make sense 'qdisc wise'. Being able to set it has
always been wrong unfortunately…

Sure but unfortunately "we don't break backward compat" means
we can't really argue. It will take us more time to debate this
than to fix it (assuming we understand the initial problem).

Frankly one can even argue whether "exploitable by root / userns"
is more important than single user's init scripts breaking.
The "security" issues for root are dime a dozen.

The solution was to change the erroneous qdisc hierarchy to a correct one when the possible UAF condition was detected https://lore.kernel.org/all/20231013151057.2611860-1-pctammela@mojatatu.com/:

As reported [1] disallowing 'rt' inner curves breaks already existing
scripts. Even though it doesn't make sense 'qdisc wise' users have been
relying on this behaviour since the qdisc inception.

We need users to update the scripts/applications to use 'sc' or 'ls'
as a inner curve instead of 'rt', but also avoid the UAF found by
Budimir, which was present since the qdisc inception.

The fix of the fix is given in the commit a13b67c9a015c4e21601ef9aa4ec9c5d972df1b4

Of these two commits the first one is already backported in 1b3b94f375c5a1161a0d8669ba27243173ae8b6f

git --no-pager log --pretty=fuller --name-status -n 1 1b3b94f37

commit 1b3b94f375c5a1161a0d8669ba27243173ae8b6f
Author:     David Gomez <dgomez@ciq.com>
AuthorDate: Mon Mar 25 14:54:15 2024 -0700
Commit:     Jonathan Maple <jmaple@ciq.com>
CommitDate: Fri Sep 13 15:19:00 2024 -0400

    net/sched: sch_hfsc: Ensure inner classes have fsc curve
    
    jira LE-588
    cve CVE-2023-4623
    commit b3d26c5702c7d6c45456326e56d2ccf3f103e60f
    
    HFSC assumes that inner classes have an fsc curve, but it is currently
    possible for classes without an fsc curve to become parents. This leads
    to bugs including a use-after-free.
    
    Don't allow non-root classes without HFSC_FSC to become parents.
    
        Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
        Reported-by: Budimir Markovic <markovicbudimir@gmail.com>
        Signed-off-by: Budimir Markovic <markovicbudimir@gmail.com>
        Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
        Link: https://lore.kernel.org/r/20230824084905.422-1-markovicbudimir@gmail.com
        Signed-off-by: Jakub Kicinski <kuba@kernel.org>
        (cherry picked from commit b3d26c5702c7d6c45456326e56d2ccf3f103e60f)
    
    Signed-off-by: David Gomez <dgomez@ciq.com>

M	net/sched/sch_hfsc.c

The other one, however, is misisng. Therefore the solution for CVE-2023-4623 on ciqlts8_6 seems to be a backport of a single commit a13b67c9a015c4e21601ef9aa4ec9c5d972df1b4 net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve.

The same solution was used already in other "version 8" branches centos8, fips-8-complaint/4.18.0-553.16.1, fips-8/4.18.0-553.16.1, rocky8_10, sig-cloud-8/4.18.0-553.22.1.el8_10, sig-cloud-8/4.18.0-553.33.1.el8_10, sig-cloud-8/4.18.0-553.36.1.el8_10:

git --no-pager log --decorate  --format="%h %cd %d %s" --date=short -n 2 ae71642ce

ae71642ce 2024-09-11  net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve
1a61f0a5d 2024-09-11  net/sched: sch_hfsc: Ensure inner classes have fsc curve

as well as in ciqcbr7.9:

git --no-pager log --decorate  --format="%h %cd %d %s" --date=short -n 2 01ce15bfa

01ce15bfa 2024-10-09  net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve
39170ba01 2024-10-09  net/sched: sch_hfsc: Ensure inner classes have fsc curve

kABI check: passed

RELAXED_DEPS=1 CVE=CVE-2023-4623 ./ninja.sh _kabi_checked_ciqlts8_6-CVE-2023-4623

[kapitox@localhost rocky-patching]$ RELAXED_DEPS=1 CVE=CVE-2023-4623 ./ninja.sh _kabi_checked_ciqlts8_6-CVE-2023-4623
ninja: Entering directory `/data/build/rocky-patching'
[0/2] Create worktree for branch 'el-8.6': '/data/src/ctrliq-github/kernel-dist-git-el-8.6' (main repo: '/data/src/ctrliq-github/kernel-dist-git')
+ rm -rf /data/src/ctrliq-github/kernel-dist-git-el-8.6
+ git -C /data/src/ctrliq-github/kernel-dist-git worktree prune
+ git -C /data/src/ctrliq-github/kernel-dist-git worktree add /data/src/ctrliq-github/kernel-dist-git-el-8.6 el-8.6
Preparing worktree (checking out 'el-8.6')
HEAD is now at 9cb8f01 import kernel-4.18.0-372.32.1.el8_6.86ciq_lts.0.7
[1/2] Check ABI of kernel [ciqlts8_6-CVE-2023-4623]	_kabi_checked_ciqlts8_6-CVE-2023-4623
+ python3 /data/src/ctrliq-github/kernel-dist-git-el-8.6/SOURCES/check-kabi -k /data/src/ctrliq-github/kernel-dist-git-el-8.6/SOURCES/Module.kabi_x86_64 -s vms/build-ciqlts8_6/build_files/kernel-src-tree-ciqlts8_6-CVE-2023-4623/Module.symvers
kABI check passed
+ touch state/kernels/ciqlts8_6-CVE-2023-4623/kabi_checked

Boot test: passed

Refer to Specific tests for implicit boot test passing.

Kselftests: passed relative

Methodology

A mix of kernel-selftests-internal and source-compiled tests were used:

• kernel-selftests-internal: bpf tests, except:
  • bpf:test_kmod.sh: takes very long time to finish and always fails anyway,
  • bpf:test_progs: unstable, can crash the machine,
  • bpf:test_progs-no_alu32: unstable, can crash the machine.
• source-compiled: all the rest.

Coverage (including tests skipped during execution)

android, bpf, breakpoints, capabilities, core, cpu-hotplug, cpufreq, efivarfs, exec, filesystems, firmware, fpu, ftrace, futex, gpio, intel_pstate, ipc, kcmp, kvm, lib, livepatch, membarrier, memfd, memory-hotplug, mount, net, net/forwarding, net/mptcp, netfilter, nsfs, proc, pstore, ptrace, rseq, rtc, sgx, sigaltstack, size, splice, static_keys, sync, sysctl, tc-testing, timens, timers, tpm2, user, vm, x86, zram

Reference ciqlts8_6 (9db5d0cdfcfd06162ee479ad4f0864ac849a646c)

Three test runs were conducted on the reference kernel.
kselftests–mixed–ciqlts8_6–run1.log
kselftests–mixed–ciqlts8_6–run2.log
kselftests–mixed–ciqlts8_6–run3.log

Patch ciqlts8_6-CVE-2023-4623 (e8b218a20ff923b2b7eede2ea83c69d38b658f61)

Two test runs were conducted on the patched kernel.
kselftests–mixed–ciqlts8_6-CVE-2023-4623–run1.log
kselftests–mixed–ciqlts8_6-CVE-2023-4623–run2.log

Comparison

ktests.xsh  table --where "Summary = 'diff'"  kselftests*.log

Column    File
--------  ----------------------------------------------------
Status0   kselftests--mixed--ciqlts8_6--run1.log
Status1   kselftests--mixed--ciqlts8_6--run2.log
Status2   kselftests--mixed--ciqlts8_6--run3.log
Status3   kselftests--mixed--ciqlts8_6-CVE-2023-4623--run1.log
Status4   kselftests--mixed--ciqlts8_6-CVE-2023-4623--run2.log

TestCase                   Status0  Status1  Status2  Status3  Status4  Summary
bpf:test_xdp_veth.sh       pass     pass     skip     skip     skip     diff
net:gro.sh                 pass     fail     pass     pass     pass     diff
net:ip_defrag.sh           fail     fail     pass     pass     fail     diff
net:reuseport_addr_any.sh  pass     fail     fail     fail     fail     diff
net:xfrm_policy.sh         pass     pass     fail     fail     fail     diff

All of the tests with different results in the tested patch showed inconsistent behavior in the reference tests set itself.

The tests bpf:test_xdp_veth.sh, net:gro.sh, net:ip_defrag.sh, net:xfrm_policy.sh were known to display inconsistent behavior before. The net:reuseport_addr_any.sh test showed inconsistency before although only on the ciqlts8_6-rt platform. Added to the list of flappy tests for ciqlts8_6 as well. Failure: exceeding 300 seconds timeout

# PASS: policies with repeated htresh change
#
not ok 12 selftests: net: xfrm_policy.sh # TIMEOUT 300 seconds

Specific tests: passed

The potential UAF condition was found to be reproducible with the following tc commands sequence:

tc qdisc add dev lo root handle 1: hfsc default 10
# Inner hfsc class constructed with 'rt' - realtime service
tc class add dev lo parent 1: classid 1:1 hfsc rt m2 100kbps
# Leaf hfsc class constructed with 'ls' - linksharing service
tc class add dev lo parent 1:1 classid 1:10 hfsc ls m2 50kbps

The "100kbps", "50kbps" parts are arbitrary. What's important is the use of rt for the inner class and ls for the leaf class. While the exact UAF was not obtained the commands helped confirm the efficacy of the patch.

Reference

The incorrect qdisc hierarchy cannot be created - the tc command ends with an error and the leaf class is not added.

[root@ciqlts8_6 pvts]# (
    tc qdisc add dev lo root handle 1: hfsc default 10
    tc class add dev lo parent 1: classid 1:1 hfsc rt m2 100kbps
    tc class add dev lo parent 1:1 classid 1:10 hfsc ls m2 50kbps
    echo $?
)
Error: Invalid parent - parent class must have FSC.
2
[root@ciqlts8_6 pvts]# tc -g class show dev lo
tc -g class show dev lo
+---(1:) hfsc 
     +---(1:1) hfsc rt m1 0bit d 0us m2 800Kbit

Full logs:
fix-replicate–ciqlts8_6.log

Patch

Creating the incorrect qdisc hierarchy raises a warning, but succeeds - the leaf class is added. Notice the type of inner class being sc instead of rt as shown by tc -g class show dev lo command.

[root@ciqlts8_6 pvts]# (
    tc qdisc add dev lo root handle 1: hfsc default 10
    tc class add dev lo parent 1: classid 1:1 hfsc rt m2 100kbps
    tc class add dev lo parent 1:1 classid 1:10 hfsc ls m2 50kbps
    echo $?
)
Warning: Forced curve change on parent 'rt' to 'sc'.
0
[root@ciqlts8_6 pvts]# tc -g class show dev lo
tc -g class show dev lo
+---(1:) hfsc 
     +---(1:1) hfsc sc m1 0bit d 0us m2 800Kbit 
          +---(1:10) hfsc ls m1 0bit d 0us m2 400Kbit 

Full logs:
fix-replicate–ciqlts8_6-CVE-2023-4623.log

[bmastbergen]

🥌

[gvrose8192]

Similar to others in this series - LGTM.

[gvrose8192]

Passes checks and is reviewed and approved.