Skip to content
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

Fix an uninitialized data access #16511

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Conversation

asomers
Copy link
Contributor

@asomers asomers commented Sep 5, 2024

zfs_acl_node_alloc allocates an uninitialized data buffer, but upstack zfs_acl_chmod only partially initializes it. KMSAN reported that this memory remained uninitialized at the point when it was read by lzjb_compress, which suggests a possible kernel memory disclosure bug.

The full KMSAN warning is:

panic: MSan: Uninitialized malloc memory from zfs_acl_chmod+0x2cd
cpuid = 6
time = 1725502132
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x99/frame 0xfffffe00b4c9c540
vpanic() at vpanic+0x56e/frame 0xfffffe00b4c9c6d0
panic() at panic+0x1dd/frame 0xfffffe00b4c9c7e0
__msan_warning() at __msan_warning+0x244/frame 0xfffffe00b4c9c930
lzjb_compress() at lzjb_compress+0x9f6/frame 0xfffffe00b4c9ca70
zio_compress_data() at zio_compress_data+0x388/frame 0xfffffe00b4c9cb40
zio_write_compress() at zio_write_compress+0x12bd/frame 0xfffffe00b4c9cca0
zio_execute() at zio_execute+0x4e0/frame 0xfffffe00b4c9cd30
taskqueue_run_locked() at taskqueue_run_locked+0x607/frame 0xfffffe00b4c9ce30
taskqueue_thread_loop() at taskqueue_thread_loop+0x29e/frame 0xfffffe00b4c9cea0
fork_exit() at fork_exit+0x1ee/frame 0xfffffe00b4c9cf30
fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe00b4c9cf30
--- trap 0x5a5a5a5a, rip = 0x5a5a5a5a5a5a5a5a, rsp = 0x5a5a5a5a5a5a5a5a, rbp = 0x5a5a5a5a5a5a5a5a ---
KDB: enter: panic

Signed-off-by: Alan Somers [email protected]
Sponsored by: Axcient

Motivation and Context

Kernel memory disclosure

Description

Zero-initialize the memory buffer

How Has This Been Tested?

Tested with bootfs_test:bootfs_005_neg from FreeBSD's ZFS test suite.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the OpenZFS code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes.
  • I have run the ZFS Test Suite with this change applied. (partially)
  • All commit messages are properly formatted and contain Signed-off-by.

Copy link
Contributor

@tonyhutter tonyhutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix

@asomers
Copy link
Contributor Author

asomers commented Sep 5, 2024

The checkstyle failure is due to the fact that I copied terminal output into the commit message. IMHO that should be allowed.

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see exactly the same code on Linux, that makes me wonder: either the same should be applied there also, or in general initialization of the memory should be up to the caller, and again on both platforms. I am not saying it is a big deal to initialize the memory here, since hopefully it is not big, but I generally don't like extra/double initializations.

@asomers
Copy link
Contributor Author

asomers commented Sep 6, 2024

I see exactly the same code on Linux, that makes me wonder: either the same should be applied there also, or in general initialization of the memory should be up to the caller, and again on both platforms. I am not saying it is a big deal to initialize the memory here, since hopefully it is not big, but I generally don't like extra/double initializations.

I'll apply the change to Linux, too. I initially thought it would be better to initialize it in the caller, too. But the caller's code is very complicated, and zeroing out the otherwise-uninitialized bytes would be a very error-prone process.

@asomers asomers requested a review from amotin September 6, 2024 15:41
@tonyhutter
Copy link
Contributor

The checkstyle failure is due to the fact that I copied terminal output into the commit message. IMHO that should be allowed.

@asomers I understand, but could you remove it anyway to make checkstyle happy? This PR will still contain the text of the backtrace if we ever need to look it up again.

zfs_acl_node_alloc allocates an uninitialized data buffer, but upstack
zfs_acl_chmod only partially initializes it.  KMSAN reported that this
memory remained uninitialized at the point when it was read by
lzjb_compress, which suggests a possible kernel memory disclosure bug.

The full KMSAN warning may be found in the PR.
openzfs#16511

Signed-off-by:	Alan Somers <[email protected]>
Sponsored by:	Axcient
@asomers
Copy link
Contributor Author

asomers commented Sep 10, 2024

Done, @tonyhutter

@tonyhutter tonyhutter merged commit 308f7c2 into openzfs:master Sep 10, 2024
20 of 21 checks passed
@tonyhutter
Copy link
Contributor

@asomers thanks, merged.

@AllKind AllKind mentioned this pull request Nov 5, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants