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.

Sign up for GitHub

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

Jump to bottom

Fix a couple of unsafe zfsvfs dereferences #15752

Closed
wants to merge 2 commits into from
Closed

Fix a couple of unsafe zfsvfs dereferences #15752

wants to merge 2 commits into from

Conversation

markjdb
Copy link
Contributor

@markjdb markjdb commented Jan 10, 2024

Fix some narrow use-after-frees in ZFS VOP implementations.

Motivation and Context

VOPs must not load certain fields from the zfsvfs structure without having called zfs_enter(), which acquires the teardown lock for the dataset. Without that lock held, the dataset may be rolled back, in which case some associated structures are freed, so the problematic scenario can lead to a use-after-free.

Description

The changes ensure that the z_os and z_log fields are loaded only after calling zfs_enter(). Ideally the compiler would warn about such bugs but I don't have any good ideas along these lines.

How Has This Been Tested?

I only compile-tested these changes.

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:

markjdb added 2 commits January 9, 2024 18:59
@markjdb
Linux: Defer loading the object set in zfs_setattr()
ad0a7c8 
We need to wait until after having done a zfs_enter() to load some
fields from the zfsvfs structure.  Otherwise a use-after-free is
possible in the face of a concurrent rollback.

Other functions in this file are careful to avoid this bug, I believe
this is the only instance.

Signed-off-by: Mark Johnston <markj@FreeBSD.org>
@markjdb
Fix a potential use-after-free in zfs_setsecattr()
c34c90e 
In general, VOPs must not load the "z_log" field until having called
zfs_enter_verify_zp().

Signed-off-by: Mark Johnston <markj@FreeBSD.org>
@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Jan 12, 2024
behlendorf pushed a commit that referenced this pull request Jan 12, 2024
@markjdb @behlendorf
Fix a potential use-after-free in zfs_setsecattr()
1a11ad9 
In general, VOPs must not load the "z_log" field until having called
zfs_enter_verify_zp().

Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Mark Johnston <markj@FreeBSD.org>
Closes #15752
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jan 29, 2024
@markjdb @behlendorf
Linux: Defer loading the object set in zfs_setattr()
d61bd70 
We need to wait until after having done a zfs_enter() to load some
fields from the zfsvfs structure.  Otherwise a use-after-free is
possible in the face of a concurrent rollback.

Other functions in this file are careful to avoid this bug, I believe
this is the only instance.

Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Mark Johnston <markj@FreeBSD.org>
Closes openzfs#15752
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jan 29, 2024
@markjdb @behlendorf
Fix a potential use-after-free in zfs_setsecattr()
a61f0b0 
In general, VOPs must not load the "z_log" field until having called
zfs_enter_verify_zp().

Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Mark Johnston <markj@FreeBSD.org>
Closes openzfs#15752
behlendorf pushed a commit that referenced this pull request Jan 29, 2024
@markjdb @behlendorf
Linux: Defer loading the object set in zfs_setattr()
22e4f08 
We need to wait until after having done a zfs_enter() to load some
fields from the zfsvfs structure.  Otherwise a use-after-free is
possible in the face of a concurrent rollback.

Other functions in this file are careful to avoid this bug, I believe
this is the only instance.

Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Mark Johnston <markj@FreeBSD.org>
Closes #15752
behlendorf pushed a commit that referenced this pull request Jan 29, 2024
@markjdb @behlendorf
Fix a potential use-after-free in zfs_setsecattr()
8b1c6db 
In general, VOPs must not load the "z_log" field until having called
zfs_enter_verify_zp().

Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Mark Johnston <markj@FreeBSD.org>
Closes #15752
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
@markjdb @lundman
Linux: Defer loading the object set in zfs_setattr()
dc8b150 
We need to wait until after having done a zfs_enter() to load some
fields from the zfsvfs structure.  Otherwise a use-after-free is
possible in the face of a concurrent rollback.

Other functions in this file are careful to avoid this bug, I believe
this is the only instance.

Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Mark Johnston <markj@FreeBSD.org>
Closes openzfs#15752
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
@markjdb @lundman
Fix a potential use-after-free in zfs_setsecattr()
c96b664 
In general, VOPs must not load the "z_log" field until having called
zfs_enter_verify_zp().

Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Mark Johnston <markj@FreeBSD.org>
Closes openzfs#15752
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
@markjdb @lundman
Linux: Defer loading the object set in zfs_setattr()
7045d44 
We need to wait until after having done a zfs_enter() to load some
fields from the zfsvfs structure.  Otherwise a use-after-free is
possible in the face of a concurrent rollback.

Other functions in this file are careful to avoid this bug, I believe
this is the only instance.

Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Mark Johnston <markj@FreeBSD.org>
Closes openzfs#15752
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
@markjdb @lundman
Fix a potential use-after-free in zfs_setsecattr()
127ead6 
In general, VOPs must not load the "z_log" field until having called
zfs_enter_verify_zp().

Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Mark Johnston <markj@FreeBSD.org>
Closes openzfs#15752
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwatkinson bwatkinson bwatkinson approved these changes

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@markjdb @behlendorf @bwatkinson