-
Notifications
You must be signed in to change notification settings - Fork 12
[LTS 9.2] wifi: mac80211: Avoid address calculations via out of bounds array in… #103
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
…dexing jira VULN-6991 cve CVE-2024-41071 commit-author Kenton Groombridge <concord@gentoo.org> commit 2663d04 req->n_channels must be set before req->channels[] can be used. This patch fixes one of the issues encountered in [1]. [ 83.964255] UBSAN: array-index-out-of-bounds in net/mac80211/scan.c:364:4 [ 83.964258] index 0 is out of range for type 'struct ieee80211_channel *[]' [...] [ 83.964264] Call Trace: [ 83.964267] <TASK> [ 83.964269] dump_stack_lvl+0x3f/0xc0 [ 83.964274] __ubsan_handle_out_of_bounds+0xec/0x110 [ 83.964278] ieee80211_prep_hw_scan+0x2db/0x4b0 [ 83.964281] __ieee80211_start_scan+0x601/0x990 [ 83.964291] nl80211_trigger_scan+0x874/0x980 [ 83.964295] genl_family_rcv_msg_doit+0xe8/0x160 [ 83.964298] genl_rcv_msg+0x240/0x270 [...] [1] https://bugzilla.kernel.org/show_bug.cgi?id=218810 Co-authored-by: Kees Cook <keescook@chromium.org> Signed-off-by: Kees Cook <kees@kernel.org> Signed-off-by: Kenton Groombridge <concord@gentoo.org> Link: https://msgid.link/20240605152218.236061-1-concord@gentoo.org Signed-off-by: Johannes Berg <johannes.berg@intel.com> (cherry picked from commit 2663d04) Signed-off-by: Marcin Wcisło <marcin.wcislo@conclusive.pl>
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.
No diff from upstream, the code does not appear to have any side effects outside the local function - LGTM. Thanks!
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.
This code looks good and things are in place, I really appreciate your testing when it comes to the individual test results.
WRT to the Analysis and the Important 7.8
rating, I've reached out to one of our key security engineers that might be able to help shed some light. My guess is that even though there are not a lot of remote aspects but Impact ratings are all really high.
It could also be that for the specific code base we're working its not quite as high and the max value is Important - 7.8
Does that make sense?
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 for @pvts-mat for your analysis. My current understanding is that there were two related commits, the CVE captures only one of them, and it shouldn't have been a CVE. https://bugzilla.kernel.org/show_bug.cgi?id=218810 lists 3 distinct UBSAN crashes referencing 2 source files. It also links to a commit fixing 1 of the files (net/wireless/nl80211.c): Then the CVE assignment message references stable tree commits for the other 1 file (net/mac80211/scan.c): Now, why would UBSAN be triggered here? There's this commit: - struct ieee80211_channel *channels[];
+ struct ieee80211_channel *channels[] __counted_by(n_channels); Both fix commits for the UBSAN detection issues also mention Another issue is that the flexible array is used as two concatenated arrays - it's also followed by cc: @kees @0xC0ncord in case you guys have any comments as you were involved in making these fixes. Thanks!
It would actually be a reasonable requirement of UBSAN to require that pointers stay within their target objects at all times (or point just past end of the object, which is a special case allowed by the C standards), because the compiler is free to make this assumption when optimizing. So it would be reasonable for it insisting on that invariant satisfied at all times (although only for operations giving pointers, not indices) rather than only at the end. However, this would only make a difference if the code added something and then subtracted something else from a pointer (or did the equivalent via array indexing and taking address). This doesn't appear to have been the case here. There are only (implied) additions and multiplications. |
I also looked at nearby CVE assignment messages to see if the other commit possibly got its own CVE - but it seems not. |
Yes! I was missing that part as well. It's obviously not a part of C language definition or something that a variable named "n_‹array›" dictates the size of "‹array›" so I looked for annotations as well, didn't find them, assumed that UBSAN uses some other methods I'm not aware of and dropped the issue as investigating how UBSAN works was not part of the task at hand. The doubts raised about the 7.8 rating were in part motivated by wanting to solve this mystery in a discusison. Thanks! |
Note that we do not even have the array size annotation in there, and indeed the commit you reference is a recent one in our LTS 9.2 https://github.com/ctrliq/kernel-src-tree/commits/ciqlts9_2/ - so there's nothing to fix for this product. |
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.
Blocking accidental merges while we wait for comment.
The way we are approaching the ordering of CVEs may not line up with our upstream fork order of commits so they may have brought something back for another CVE that then makes this valid after that change so I don't necessarily want to cancel this at the moment
@solardiz Your analysis of the patch is way better than anything I could have written. This is the first time I've heard of CVE-2024-41071, which should never have been filed for this. The patch doesn't fix any security issue or even alter runtime behavior in any way. It just, as you wrote, satisfies UBSAN by making sure to update EDIT: It looks like the CVE was withdrawn a few moments ago. |
@0xC0ncord I appreciate you jumping in and reviewing @solardiz 's statements after @pvts-mat looked deeper into this CVE during their analysis and work. I'm going to close this commit and set all our internal VULN's marked as such. |
CVE-2024-41071
VULN-6991
Problem
The bug was submitted https://bugzilla.kernel.org/show_bug.cgi?id=218810 about array-index-out-of-bounds errors in the
mac80211
module when the kernel was compiled with UBSAN (Undefined Behavior Sanitizer) enabled.Solution
The bug was fixed in upstream with the 2663d0462eb32ae7c9b035300ab6b1523886c718 commit - see https://lore.kernel.org/linux-cve-announce/2024072909-CVE-2024-41071-4eb6@gregkh/T/#u
No additional commits that weren't derivative of this one were identified to be related to this issue.
Analysis
The problem doesn't seem to be a vulnerability really, and the solution is less about the fix of erroneous code and more about satisfying the UBSAN runtime checker, which apparently needs to have the index < data_size invariant satisfied at all times instead of only at the end of data construction. This looks like overzealousness on the UBSAN part which should be checking index < allocated_size which is satisfied all right (see
cfg80211_scan_request
definition andlocal->hw_scan_req
allocation withreq->n_channels * sizeof(req->channels[0])
part to fit all the elements inserted inside the later calledieee80211_prep_hw_scan(…)
at lines 371 and 387 at indexes upper-bound in both cases byreq->n_channels
, the samereq->n_channels
used to allocate thelocal->hw_scan_req->req.channels
). The 7.8 CVSS score doesn't seem to be justified.kABI check: passed
kernel-dist-git
state:Boot test: passed
boot-test.log
Kselftests: passed relative
Methodology
A mix of
kernel-selftests-internal
and source-compiled (f08be21dd6d4b10d0da6f60abbf8c20337f8b836
) 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.Machine preparation:
Coverage (including tests skipped during execution)
bpf
,breakpoints
,capabilities
,cgroup
,clone3
,core
,cpu-hotplug
,cpufreq
,drivers/dma-buf
,drivers/net/bonding
,drivers/net/team
,efivarfs
,filesystems
,filesystems/binderfs
,filesystems/epoll
,firmware
,fpu
,ftrace
,futex
,gpio
,intel_pstate
,ipc
,ir
,kcmp
,landlock
,lib
,livepatch
,membarrier
,memfd
,memory-hotplug
,mincore
,mount
,mqueue
,nci
,net
,net/forwarding
,net/mptcp
,netfilter
,nsfs
,openat2
,pid_namespace
,pidfd
,pstore
,ptrace
,rlimits
,rseq
,rtc
,seccomp
,sgx
,sigaltstack
,size
,splice
,static_keys
,sync
,syscall_user_dispatch
,sysctl
,tc-testing
,tdx
,timens
,timers
,tmpfs
,tpm2
,user
,vDSO
,vm
,x86
,zram
Reference
ciqlts9_2
(f08be21dd6d4b10d0da6f60abbf8c20337f8b836
)kselftest–mixed–ciqlts9_2–run1.log
kselftest–mixed–ciqlts9_2–run2.log
kselftest–mixed–ciqlts9_2–run3.log
kselftest–mixed–ciqlts9_2–run4.log
kselftest–mixed–ciqlts9_2–run5.log
Patch (
c8c91992be998a22058aef656a0e785be34f332c
)kselftest–mixed–ciqlts9_2-CVE-2024-41071–run1.log
kselftest–mixed–ciqlts9_2-CVE-2024-41071–run2.log
Comparison
The only differences are for
mqueue:mq_perf_tests
,rtc:rtctest
which pass and fail within the reference kernel check as well. Added to the list of unreliable tests at https://docs.google.com/spreadsheets/d/1tUwJ2rV57cYZXh7momPtraSjZcHDjMYHLeHA3DYWrUU/edit?pli=1&gid=0#gid=0&range=J:J.Specific tests: inconclusive
An attempt was made to replicate the errors mentioned in https://bugzilla.kernel.org/show_bug.cgi?id=218810. The reference kernel
ciqlts9_2
was compiled withCONFIG_UBSAN=y
option set and a naiveiw
call was used in hope of triggering theieee80211_scan_work(…)
function, but the replication failed with the VM machine not having any wireless devices installed, the test confirming only themac80211
module being loaded.No further attempts were made to replicate this issue.