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

[Bryam Vargas <hexlabsecurity@proton.me>]

Hi Günther,

> 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 */

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.

> 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.

> 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.

Bryam