rose: fix use-after-free, dev_put leak, and timer race in rose module#1
Open
f6bvp wants to merge 6 commits into
Open
rose: fix use-after-free, dev_put leak, and timer race in rose module#1f6bvp wants to merge 6 commits into
f6bvp wants to merge 6 commits into
Conversation
af_rose.c uses struct sockaddr_unsized in its .bind and .connect proto_ops callbacks. That type was introduced in Linux 7.0; on older kernels it does not exist. Add a compat shim to hamradio_compat.h that maps sockaddr_unsized to the traditional struct sockaddr when LINUX_VERSION_CODE < 7.0, and force-include the header for all rose sources via Kbuild so no per-file change is needed.
rose_rx_call_request() always consumes or returns the skb but never
releases the device reference obtained from rose_dev_get(). When
rose_rx_call_request() succeeds (returns non-zero) dev_put() was never
called, leaking one reference per loopback CALL_REQUEST.
Move dev_put() outside the conditional so it is called unconditionally
after rose_rx_call_request() in all cases.
Also remove the dead check (!rose_loopback_neigh->dev &&
!rose_loopback_neigh->loopback) that immediately precedes it: the
loopback neighbour always has loopback=1 so this condition can never
be true.
Fixes: 0453c6824595 ("net/rose: fix unbound loop in rose_loopback_timer()")
Signed-off-by: Bernard Pidoux <f6bvp@free.fr>
rose_loopback_timer() dereferences rose_loopback_neigh throughout its
body but holds no reference on it. A concurrent rose_loopback_clear()
followed by rose_add_loopback_neigh() could free and reallocate the
neighbour while the timer body is running, causing a use-after-free.
Take a reference with rose_neigh_hold() at the start of the callback
(bailing out if the pointer is already NULL) and release it with
rose_neigh_put() at the single exit point. The neigh cannot be freed
while the callback holds a reference.
Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
Signed-off-by: Bernard Pidoux <f6bvp@free.fr>
rose_loopback_clear() called timer_delete() which returns immediately
without waiting for any running callback to complete. If the timer
fired concurrently with module removal, rose_loopback_timer() could
re-arm the timer after timer_delete() returned and then access
rose_loopback_neigh after it was freed.
Two complementary changes close the race:
1. Add a loopback_stopping atomic flag. rose_loopback_timer() checks
it at entry (before acquiring a reference) and again inside the
loop; when set it drains the queue and exits without re-arming the
timer.
2. Switch rose_loopback_clear() to timer_delete_sync() so it blocks
until any in-flight callback has returned before freeing resources.
The smp_mb() between setting the flag and calling timer_delete_sync()
ensures the flag is visible to any callback that is about to run.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Bernard Pidoux <f6bvp@free.fr>
After calling rose_neigh_put() in rose_state1_machine() through
rose_state5_machine(), rose->neighbour was left pointing at the
potentially freed neighbour structure. A subsequent timer expiry or
concurrent teardown path could dereference the stale pointer, causing
a use-after-free.
Set rose->neighbour to NULL immediately after each rose_neigh_put()
call in the state machine functions.
Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
Signed-off-by: Bernard Pidoux <f6bvp@free.fr>
In rose_timer_expiry(), the ROSE_STATE_2 branch calls
rose_neigh_put(rose->neighbour) without first checking whether the
pointer is NULL. After commit 5de7665e0a07 ("net: rose: fix timer
races against user threads") the timer is re-armed when the socket is
owned by a user thread; between the re-arm and the next firing, a
device-down event or concurrent teardown via rose_kill_by_device() can
set rose->neighbour to NULL, leading to a NULL-pointer dereference
inside rose_neigh_put().
Add a NULL check before the put and clear the pointer afterwards.
Fixes: 5de7665e0a07 ("net: rose: fix timer races against user threads")
Signed-off-by: Bernard Pidoux <f6bvp@free.fr>
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
1 participant
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.
Summary
Five bug fixes for the rose protocol module, backported from net-next.
All tested on kernel 6.17.0-23-generic (pre-7.0) with successful
insmod, operation, and rmmod.
A prerequisite compat commit is included to allow building on kernels
older than 7.0, which introduced
struct sockaddr_unsized.rose: build with hamradio_compat.h on pre-7.0 kernels — add
sockaddr_unsizedcompat shim and force-include the header via Kbuild,required because af_rose.c uses
sockaddr_unsizedin its proto_opscallbacks
rose: fix dev_put() leak in rose_loopback_timer() — move dev_put()
outside the conditional; remove dead loopback_neigh check
rose: hold loopback neighbour reference across timer callback —
prevent use-after-free when rose_loopback_clear() runs concurrently
with the timer
rose: fix race between loopback timer and module removal — add
loopback_stoppingatomic flag and switch totimer_delete_sync()rose: clear neighbour pointer after rose_neigh_put() in state
machines — set
rose->neighbour = NULLafter each put to preventstale pointer dereference
rose: guard rose_neigh_put() against NULL in timer expiry — add
NULL check in
ROSE_STATE_2branch ofrose_timer_expiry()Test plan
sudo insmod rose.ko— NET: Registered PF_ROSE protocol familysudo rmmod rose— clean removal, no crash, no leak