Message-ID: <20250201.quaizoh3taeV@digikod.net>
Date: Sat, 1 Feb 2025 11:28:28 +0100
From: Mickaël Salaün <mic@...ikod.net>
To: Paul Moore <paul@...l-moore.com>
Cc: Christian Brauner <brauner@...nel.org>, 
	Günther Noack <gnoack@...gle.com>, Charles Zaffery <czaffery@...lox.com>, 
	Daniel Burgener <dburgener@...ux.microsoft.com>, Francis Laniel <flaniel@...ux.microsoft.com>, 
	James Morris <jmorris@...ei.org>, Jann Horn <jannh@...gle.com>, Jeff Xu <jeffxu@...gle.com>, 
	Kees Cook <kees@...nel.org>, Luca Boccassi <luca.boccassi@...il.com>, 
	Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>, Praveen K Paladugu <prapal@...ux.microsoft.com>, 
	Robert Salvet <robert.salvet@...lox.com>, Shervin Oloumi <enlightened@...gle.com>, 
	Tyler Hicks <code@...icks.com>, linux-kernel@...r.kernel.org, 
	linux-security-module@...r.kernel.org
Subject: Re: [RFC PATCH v1 2/3] pidfd: Extend PIDFD_GET_INFO with
 PIDFD_INFO_LANDLOCK_*_DOMAIN

On Fri, Jan 31, 2025 at 02:02:49PM -0500, Paul Moore wrote:
> On Fri, Jan 31, 2025 at 11:43 AM Mickaël Salaün <mic@...ikod.net> wrote:
> >
> > Because Landlock enables users to create nested sandboxes (i.e.
> > domains), we might need to identify the domain with all restrictions
> > (latest), or the domain we created (i.e. closest domain).  Indeed,
> > because any process can create its own domain, the latest domain may not
> > be known by the requester.
> >
> > The PIDFD_INFO_LANDLOCK_LAST_DOMAIN flag enables user space to get the
> > latest (i.e. most nested) Landlock domain ID related to a sandboxed
> > task, if any.  The domain ID is set in the pidfd_info's
> > landlock_last_domain field according to the related mask.
> >
> > The PIDFD_INFO_LANDLOCK_FIRST_DOMAIN flag enables user space to get the
> > closest (i.e. first hierarchy relative to the pidfd's credentials)
> > Landlock domain ID related to a sandboxed task, if any.  The domain ID
> > is set in the pidfd_info's landlock_first_domain field according to the
> > related mask.
> >
> > It is only allowed to get information about a Landlock domain if the
> > task's domain that created the pidfd is a parent of the PID's domain.
> > Following the object-capability model, the pidfd's credentials are used
> > instead of the caller's credentials.  This makes this command
> > idenmpotent wrt the referenced process's state.
> >
> > If Landlock is not supported or if access to this information is denied,
> > then the IOCTL does not set the PIDFD_INFO_LANDLOCK_*_DOMAIN flag in the
> > returned mask.
> >
> > If PIDFD_INFO_LANDLOCK_LAST_DOMAIN or PIDFD_INFO_LANDLOCK_FIRST_DOMAIN
> > is specified but the provided struct pidfd_info is not large enough to
> > contain the related field, then -EINVAL is returned.
> >
> > Cc: Christian Brauner <brauner@...nel.org>
> > Cc: Günther Noack <gnoack@...gle.com>
> > Cc: Luca Boccassi <luca.boccassi@...il.com>
> > Cc: Praveen K Paladugu <prapal@...ux.microsoft.com>
> > Closes: https://github.com/landlock-lsm/linux/issues/26
> > Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> > Link: https://lore.kernel.org/r/20250131163447.1140564-3-mic@digikod.net
> > ---
> >  fs/pidfs.c                 | 24 ++++++++++++++++++++++--
> >  include/uapi/linux/pidfd.h |  4 ++++
> >  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> While there are exceptions, mostly for legacy things, we try very hard
> to avoid having the kernel call directly into a specific LSM,
> preferring to use LSM interfaces, both so that all LSMs can benefit
> from the change and also so that we can avoid having a lot of very
> similar, but LSM-specific, calls in various parts in the kernel.

Making life easier for LSMs by sharing common code is a good thing, but
making life easier for all kernel components by sharing common code is
even better.  The PIDFD_GET_INFO IOCTL was design to be very flexible,
and it follows the principle of "pay for what you request" thanks to the
"mask" bitfield.

Users specify a set of properties they want, and the kernel returns
these properties if they are supported and allowed.  Each of this
property is well-specified and has a clear semantic.  This patch series
implements two Landlock properties, each clearly identified and
independent.

One important difference between the current LSMs attributes and these
two new Landlock properties, is that the Landlock domain IDs are u64
values instead of strings.  This makes the implementation quite forward
and it fits well with how PIDFD_GET_INFO currently works, so there is no
need for a new (PIDFD_GET_SECURITY) IOCTL handling complex data
structure composing a set of strings such as what is required for
current LSMs' attributes.

> 
> There is an effort, albeit a slowly moving effort due to interrupts,
> to add LSM support via a PIDFS_GET_SECURITY API:
> 
> https://lore.kernel.org/linux-security-module/CAHC9VhRV3KcNGRw6_c-97G6w=HKNuEQoUGrfKhsQdWywzDDnBQ@mail.gmail.com/

This effort is good, but it is a separate effort which is independent
from this patch series.  This will be useful for LSMs (or hopefully
other parts of the kernel as well) that deal with string-based
attributes.

Even with a common hook and data structure, any LSM need to implement
their own attribute management.  This patch series just makes a call to
the Landlock implementation the same way UID, cgroupid, and other
properties are retrieved.  There is no need for a wrapper interface for
simple data types that are already handled by PIDFD_GET_INFO.

Simple property types should all be queryable with the PIDFD_GET_INFO
IOCTL (compared to a dedicated LSM's PIDFD_GET_SECURITY IOCTL), which
can batch queries, making it more efficient and easier to implement for
user space.

> 
> I don't see any reason why we couldn't support Landlock's domain info
> as part of that, the lsm_ctx struct was created to support various
> different LSM contexts/attributes.  You could either add multiple
> lsm_ctx array entries for Landlock, one for each of the domain IDs, or
> you could place all of the domain IDs in an expanded lsm_ctx.
> Remember the lsm_ctx->ctx field can hold binary blobs and/or you can
> expand past the end of lsm_ctx->ctx so long as lsm_ctx->{len,ctx_len}
> are set accordingly.
> 
> If you want to work on the PIDFS_GET_SECURITY patch(set) as a means to
> add Landlock domain ID support, I think that would be great.  Luca
> provided a basic skeleton in the link above, and I expect you would
> have no issue adding the missing LSM bits.
> 
> -- 
> paul-moore.com
> 

Powered by blists - more mailing lists