Re: [PATCH] landlock: Fix deadlock in restrict_one_thread_callback

[Günther Noack]

Hello!

Thanks for sending the patch!

On Tue, Feb 24, 2026 at 02:27:29PM +0800, Yihan Ding wrote:
> syzbot found a deadlock in landlock_restrict_sibling_threads().
> When multiple threads concurrently call landlock_restrict_self() with
> sibling thread restriction enabled, they can deadlock by mutually
> queueing task_works on each other and then blocking in kernel space
> (waiting for the other to finish).
> 
> Fix this by serializing the TSYNC operations within the same process
> using the exec_update_lock. This prevents concurrent invocations
> from deadlocking.
> 
> Additionally, update the comments in the interrupt recovery path to
> clarify that cancel_tsync_works() is an opportunistic cleanup, and
> waiting for completion is strictly necessary to prevent a Use-After-Free
> of the stack-allocated shared_ctx.
> 
> Fixes: 42fc7e6543f6 ("landlock: Multithreading support for landlock_restrict_self()")
> Reported-by: syzbot+7ea2f5e9dfd468201817@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=7ea2f5e9dfd468201817
> Signed-off-by: Yihan Ding <dingyihan@xxxxxxxxxxxxx>
> ---
>  security/landlock/tsync.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/security/landlock/tsync.c b/security/landlock/tsync.c
> index de01aa899751..4e91af271f3b 100644
> --- a/security/landlock/tsync.c
> +++ b/security/landlock/tsync.c
> @@ -447,6 +447,12 @@ int landlock_restrict_sibling_threads(const struct cred *old_cred,
>  	shared_ctx.new_cred = new_cred;
>  	shared_ctx.set_no_new_privs = task_no_new_privs(current);
>  
> +	/*
> +	 * Serialize concurrent TSYNC operations to prevent deadlocks
> +	 * when multiple threads call landlock_restrict_self() simultaneously.
> +	 */
> +	down_write(&current->signal->exec_update_lock);

Should we use the *_killable variant of this lock acquisition?


>  	/*
>  	 * We schedule a pseudo-signal task_work for each of the calling task's
>  	 * sibling threads.  In the task work, each thread:
> @@ -527,14 +533,17 @@ int landlock_restrict_sibling_threads(const struct cred *old_cred,
>  					   -ERESTARTNOINTR);
>  
>  				/*
> -				 * Cancel task works for tasks that did not start running yet,
> -				 * and decrement all_prepared and num_unfinished accordingly.
> +				 * Opportunistic improvement: try to cancel task works
> +				 * for tasks that did not start running yet. We do not
> +				 * have a guarantee that it cancels any of the enqueued
> +				 * task works (because task_work_run() might already have
> +				 * dequeued them).
>  				 */
>  				cancel_tsync_works(&works, &shared_ctx);
>  
>  				/*
> -				 * The remaining task works have started running, so waiting for
> -				 * their completion will finish.
> +				 * We must wait for the remaining task works to finish to
> +				 * prevent a use-after-free of the local shared_ctx.
>  				 */
>  				wait_for_completion(&shared_ctx.all_prepared);

I do not think that we must wait for all_prepared here, as your
updated comment says: The landlock_restrict_sibling_threads() function
still waits for all of these task works to finish at the bottom where
it waits for "all_finished", so there is no UAF on the local shared
context?

I would recommend replacing the
wait_for_completion(&shared_ctx.all_prepared) call and its comment
with an explicit "break":

/*
 * Break the loop with error.  The cleanup code after the loop
 * unblocks the remaining task_works.
 */
break;

Please also update the comment above the complete_all(ready_to_commit):

  We now have either (a) all sibling threads blocking and in
  "prepared" state in the task work, or (b) the preparation error is
  set.  Ask all threads to commit (or abort).

Then it is a bit more explicit about the error handling variant of this.


(FYI, I have tested the patch variant where I only removed the
wait_for_completion(all_prepared) call, and where I did *not* add the
additional lock at the top.  In this configuration, I was unable to
get it to hang any more, even with added mdelays.  But as discussed in
section 2.2 of [1], there are still difficult to reproduce scenarios
where this can theoretically fail, and it is better to use the lock at
the top.)

[1] https://lore.kernel.org/all/20260223.52c45aed20f8@xxxxxxxxxx/

Please also feel free to split up the change into a part that adds the
exec_guard_lock and a part that changes the path where the calling
thread gets interrupted.  Strictly speaking, the part where we change
the interruption logic is only a nicety once we have the
exec_guard_lock in place.

>  			}
> @@ -557,5 +566,7 @@ int landlock_restrict_sibling_threads(const struct cred *old_cred,
>  
>  	tsync_works_release(&works);
>  
> +	up_write(&current->signal->exec_update_lock);
> +
>  	return atomic_read(&shared_ctx.preparation_error);
>  }
> -- 
> 2.51.0
> 

Thanks,
–Günther