Re: [PATCH v4 1/2] landlock: fix LANDLOCK_SCOPE_SIGNAL bypass on the SIGIO path

["Günther Noack" <gnoack3000@gmail.com>]

Hello Bryam,

Just a brief mail to confirm the approach; this makes sense to me.

On Thu, Jun 04, 2026 at 10:27:13AM +0000, Bryam Vargas wrote:
> > I believe the result after this patch is:
> >  - No threads receive the SIGIO at all.
> >
> > This is because we have been setting T2.2's Landlock domain as the
> > "sending domain" for the hook_file_sigiotask(), and that hook does on
> > its own not do the "same_thread_group()" check [...]
> 
> Confirmed -- I traced the delivery path and your analysis holds.
> 
> For a PGID owner the signal is anchored per process on its thread-group
> leader: a task is attached to pid->tasks[PIDTYPE_PGID] only in the
> thread_group_leader() branch of copy_process(), so send_sigio()'s
> do_each_pid_task(pid, PIDTYPE_PGID, p) walk visits exactly T2.1 for P2,
> never the non-leader T2.2.  hook_file_send_sigiotask() then runs
> domain_is_scoped(recorded T2.2 domain, T2.1's live domain, SIGNAL) and,
> having no same_thread_group() exemption of its own (unlike
> hook_task_kill()), denies it -- even though T2.1 and T2.2 share P2's
> signal_struct and 18eb75f3af40 mandates that same-process delivery always
> be allowed.  T2.1 is P2's only entry on the PGID list, so P2 receives
> nothing.  You are right.
> 
> One thing worth putting on the record: this over-block is not introduced
> by the patch.  In unpatched control_current_fowner() the PGID case already
> resolves through pid_task(fown->pid, PIDTYPE_PGID), which returns an
> arbitrary hlist head -- one representative leader.  Whenever that head is
> outside the caller's thread group, the domain is already recorded today and
> the same delivery-time denial of the registrant's own leader already fires.
> The patch only makes domain recording for PGID unconditional, i.e. it turns
> that order-dependent behaviour into a deterministic one while closing the
> order-dependent bypass.  So the corner you describe is a pre-existing gap in
> the delivery hook, not a regression in v4.
> 
> That points at the real root cause: same_thread_group is a *per-recipient*
> property, but control_current_fowner() approximates it once, at F_SETOWN
> time, against a single pid_task() representative.  hook_task_kill() gets
> this right because it evaluates same_thread_group(p, current) live, per
> actual recipient.  hook_file_send_sigiotask() is the SIGIO analogue but
> delegates the whole thread-group decision to that one registration-time
> check, which a PGID delivery set simply cannot be captured by.
> 
> So the fully-correct fix is to move the same-process exemption to delivery
> time, keyed to the *registrant* rather than to current (at SIGIO time
> current is the fd writer, not the task that armed F_SETOWN).  Concretely:
> when hook_file_set_fowner() records the domain, also pin
> get_pid(task_tgid(current)) in struct landlock_file_security; in
> hook_file_send_sigiotask(), before domain_is_scoped(), return 0 when
> task_tgid(tsk) == that recorded pid.  PGID owners still record the domain
> (so P1 stays blocked -- the bypass fix), but the registrant's own process,
> including T2.1, is always allowed -- restoring 18eb75f3af40 exactly.  The
> new pid is taken/put in lockstep with fown_subject.domain under the same
> file->f_owner->lock and freed in hook_file_free_security(); the equality
> test follows neither pid, so there is no extra RCU surface.  Sketch:
> 
>     /* struct landlock_file_security */
>     struct pid *fown_tg;   /* registrant's thread group; NULL if no domain */
> 
>     /* hook_file_set_fowner(), where fown_subject is recorded */
>     fown_tg = get_pid(task_tgid(current));
>     ...
>     put_pid(landlock_file(file)->fown_tg);     /* release previous */
>     landlock_file(file)->fown_tg = fown_tg;
> 
>     /* hook_file_free_security() */
>     put_pid(landlock_file(file)->fown_tg);
> 
>     /* hook_file_send_sigiotask(), after the !subject->domain quick return */
>     if (task_tgid(tsk) == landlock_file(fown->file)->fown_tg)
>             return 0;   /* same process as the registrant: always allowed */
n> 
> I do not see a correct fix that avoids recording the registrant's identity:
> the registrant task is deliberately discarded after set_fowner (only its
> domain is kept), and exempting on a shared *domain* instead would be
> insecure -- sibling threads can hold different domains, and a different
> process could share one.

Yes, your approach checks out for me; I also think that storing this
additional information is the best approach; we need to know during
hook_file_send_sigiotask() what the TGID of the registering task was,
in order to tell apart signals within the same process from signals
going outwards of that process.


> > To be clear, the patch is still obviously an improvement [...] it just
> > seems to block it slightly too broadly in this corner scenario?
> > [...] Mickaël, maybe you have some thoughts on the tradeoff?
> 
> Agreed on both counts.  Mickaël -- two ways to land this:
> 
>   (a) keep v4 as is.  It closes the bypass; the residual same-process
>       over-block is pre-existing, deterministic only under the stacked
>       conditions Günther listed (already-multithreaded enforce, no TSYNC,
>       SIGIO to a PGID that includes self, registered from a non-leader
>       thread in a per-thread signal-scoped domain), and arguably tolerable.
> 
>   (b) v5 = v4 + the delivery-time exemption above.  Strictly more correct:
>       it also closes the pre-existing delivery-hook gap and restores
>       18eb75f3af40's same-process invariant, at the cost of one struct pid*
>       in landlock_file_security.
> 
> I lean (b) -- it fixes the actual root cause rather than the one reachable
> instance -- and I am happy to spin it (with an added selftest covering the
> PGID-includes-self / non-leader-registrant case, A/B verified) or to hold at
> v4 if you would rather keep the change minimal.  Your call on whether the
> corner warrants the extra state.

+1, I also think that the approach is quite clean.  Some checks would
happen at a later time, but it seems unavoidable in the generic case.
Checking TGID during hook_file_send_sigiotask() sounds reasonably
cheap.  (I suspect that trying to do that check early during
hook_file_set_fowner() would not save us much.)


> > P.S: [...] new patchset versions are posted at the top (no Reply-To
> >      header in the cover letter) [...]
> 
> Will do -- v5 (whichever option) goes out as a fresh top-level thread, no
> In-Reply-To/Reply-To pointing back at this review.

Awesome, thank you very much for looking into patching this! :)

–Günther