ksmbd-tools: fix race conditions and thread safety in user management#337
Open
yskzalloc wants to merge 3 commits into
Open
ksmbd-tools: fix race conditions and thread safety in user management#337yskzalloc wants to merge 3 commits into
yskzalloc wants to merge 3 commits into
Conversation
usm_handle_logout_request() updates user->failed_login_count and user->flags when processing a logout request. However, it modifies these fields without holding the user->update_lock. Since the ksmbd_user object can be accessed concurrently by multiple threads handling simultaneous login or logout requests for the same user, this lockless modification leads to a data race, potentially corrupting the login failure count or user state flags. Fix this by acquiring the user->update_lock writer lock before updating these fields to ensure thread safety. Fixes: a114451 ("ksmbd-tools: throttle session setup failures to avoid dictionary attacks") Signed-off-by: Yunseong Kim <yunseong.kim@est.tech>
usm_add_guest_account() sets the KSMBD_USER_FLAG_GUEST_ACCOUNT flag for a newly created guest user. However, it calls set_user_flag() without holding the user->update_lock. Modifying the user's flags locklessly at this stage can result in a data race if another thread accesses the user object simultaneously. Fix this by acquiring the user->update_lock writer lock before setting the guest account flag to ensure thread-safe flag modification. Fixes: 54d2a02 ("ksmbd-tools: simplify config parser") Signed-off-by: Yunseong Kim <yunseong.kim@est.tech>
__handle_login_request() reads user->flags and user->ngroups to construct the login response status. However, these reads are performed without holding the user->update_lock. Since user->flags can be modified concurrently by other threads (for example, during a logout request processing or when setting guest account flags), this lockless access can lead to reading an inconsistent or stale user state. Fix this by acquiring the user->update_lock reader lock before accessing these fields to ensure thread safety. As a minor cleanup, combine the status flag assignments into a single line. Fixes: c2b6ac1 ("cifsd-tools: factor out user login request handling") Signed-off-by: Yunseong Kim <yunseong.kim@est.tech>
Member
|
@kzall0c Okay. How did you reproduce this? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch series addresses several race conditions and thread safety issues discovered in the user management subsystem
tools/management/user.cof ksmbd-tools.Since the
ksmbd_userobject can be accessed concurrently by multiple threads processing simultaneous login/logout requests or administrative actions, it is crucial that all modifications and reads of shared fields (such as flags, failed_login_count, and ref_count) are properly synchronized using the object's update_lock.The series resolves these thread safety issues.
Signed-off-by: Yunseong Kim yunseong.kim@est.tech