Messages in this thread Patch in this message | | | From | Thomas Gleixner <> | Subject | [PATCH] signal: Prevent sigqueue caching after task got released | Date | Tue, 22 Jun 2021 01:08:30 +0200 |
| |
syzbot reported a memory leak related to sigqueue caching. This happens when a thread group leader with child tasks is reaped.
The group leader's sigqueue_cache is correctly freed. The group leader then reaps the child tasks and if any of them has a signal pending it caches that signal. That's obviously bogus because nothing will free the cached signal of the reaped group leader anymore.
Prevent this by setting tsk::sigqueue_cache to an error pointer value in exit_task_sigqueue_cache().
Add comments to all relevant places.
Fixes: 4bad58ebc8bc ("signal: Allow tasks to cache one sigqueue struct") Reported-by: syzbot+0bac5fec63d4f399ba98@syzkaller.appspotmail.com Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- kernel/signal.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
--- a/kernel/signal.c +++ b/kernel/signal.c @@ -435,6 +435,12 @@ static struct sigqueue * * Preallocation does not hold sighand::siglock so it can't * use the cache. The lockless caching requires that only * one consumer and only one producer run at a time. + * + * For the regular allocation case it is sufficient to + * check @q for NULL because this code can only be called + * if the target task @t has not been reaped yet; which + * means this code can never observe the error pointer which is + * written to @t->sigqueue_cache in exit_task_sigqueue_cache(). */ q = READ_ONCE(t->sigqueue_cache); if (!q || sigqueue_flags) @@ -463,13 +469,18 @@ void exit_task_sigqueue_cache(struct tas struct sigqueue *q = tsk->sigqueue_cache; if (q) { - tsk->sigqueue_cache = NULL; /* * Hand it back to the cache as the task might * be self reaping which would leak the object. */ kmem_cache_free(sigqueue_cachep, q); } + + /* + * Set an error pointer to ensure that @tsk will not cache a + * sigqueue when it is reaping it's child tasks + */ + tsk->sigqueue_cache = ERR_PTR(-1); } static void sigqueue_cache_or_free(struct sigqueue *q) @@ -481,6 +492,10 @@ static void sigqueue_cache_or_free(struc * is intentional when run without holding current->sighand->siglock, * which is fine as current obviously cannot run __sigqueue_free() * concurrently. + * + * The NULL check is safe even if current has been reaped already, + * in which case exit_task_sigqueue_cache() wrote an error pointer + * into current->sigqueue_cache. */ if (!READ_ONCE(current->sigqueue_cache)) WRITE_ONCE(current->sigqueue_cache, q);
| |