rose: use timer_shutdown_sync() for t0timer teardown - #6
Merged
Conversation
rose_t0timer_expiry() re-arms itself via rose_start_t0timer() at its
own tail. rose_neigh_put() and rose_remove_neigh() stop it with
timer_delete_sync() before freeing (or unlinking) the neighbour, but
that only guarantees the callback is not running *at the moment the
call returns* -- it does nothing to stop the very invocation that was
just waited out from re-arming the timer on its way out. That re-arm
races the kfree() in rose_neigh_put(): the timer can fire again on
freed memory, and rose_t0timer_expiry() -> rose_transmit_restart_request()
-> rose_send_frame() -> ax25_send_frame() dereferences the freed
neigh->digipeat, use-after-free.
This matches a syzbot report (KASAN slab-use-after-free read in
ax25_find_cb()) that stayed open with both cause and fix bisection
failing -- consistent with a hole that timer_delete_sync() alone
cannot close for a self-rearming timer, a case documented in its own
kerneldoc ("there is no way to get this correct with
timer_delete_sync()").
Use timer_shutdown_sync() instead in both call sites. Unlike
timer_delete_sync(), it also marks the timer so that any further
add_timer()/mod_timer() on it is silently ignored, which closes the
window regardless of how the self-rearm and the free are interleaved.
ftimer's handler is a no-op and never re-arms, but it is switched the
same way for consistency: both timers are being torn down for good in
both of these call sites.
Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=caa052a0958a9146870d
Signed-off-by: Bernard Pidoux <bernard.f6bvp@gmail.com>
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
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.
rose_t0timer_expiry() re-arms itself via rose_start_t0timer() at its
own tail. rose_neigh_put() and rose_remove_neigh() stop it with
timer_delete_sync() before freeing (or unlinking) the neighbour, but
that only guarantees the callback is not running at the moment the
call returns -- it does nothing to stop the very invocation that was
just waited out from re-arming the timer on its way out. That re-arm
races the kfree() in rose_neigh_put(): the timer can fire again on
freed memory, and rose_t0timer_expiry() -> rose_transmit_restart_request()
-> rose_send_frame() -> ax25_send_frame() dereferences the freed
neigh->digipeat, use-after-free.
This matches a syzbot report (KASAN slab-use-after-free read in
ax25_find_cb()) that stayed open with both cause and fix bisection
failing -- consistent with a hole that timer_delete_sync() alone
cannot close for a self-rearming timer, a case documented in its own
kerneldoc ("there is no way to get this correct with
timer_delete_sync()").
Fix: use timer_shutdown_sync() instead in both call sites. Unlike
timer_delete_sync(), it also marks the timer so that any further
add_timer()/mod_timer() on it is silently ignored, which closes the
window regardless of how the self-rearm and the free are interleaved.
ftimer's handler is a no-op and never re-arms, but it is switched the
same way for consistency: both timers are being torn down for good in
both of these call sites.
Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=caa052a0958a9146870d
Single commit, applies cleanly to current main.