Re: [RFC PATCH v1 2/3] pidfd: Extend PIDFD_GET_INFO with PIDFD_INFO_LANDLOCK_*_DOMAIN

[Christian Brauner]

On Sat, Feb 01, 2025 at 11:28:28AM +0100, Mickaël Salaün wrote:
> 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@xxxxxxxxxxx> 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@xxxxxxxxxx>
> > > Cc: Günther Noack <gnoack@xxxxxxxxxx>
> > > Cc: Luca Boccassi <luca.boccassi@xxxxxxxxx>
> > > Cc: Praveen K Paladugu <prapal@xxxxxxxxxxxxxxxxxxx>
> > > Closes: https://github.com/landlock-lsm/linux/issues/26
> > > Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx>
> > > Link: https://lore.kernel.org/r/20250131163447.1140564-3-mic@xxxxxxxxxxx
> > > ---
> > >  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@xxxxxxxxxxxxxx/
> 
> 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.

Hm, I agree with Paul here. I'd rather see a unified PIDFD_GET_SECURITY
ioctl rather than plumbing bits of some LSMs into PIDFD_GET_INFO
directly. You can design the PIDFD_GET_SECURITY in a way that you can
get properties such as the landlock ids without any string handling.
There must be other security properties that don't want to be strings.