Message-ID: <194c3e9fb98.28a7.85c95baa4474aabc7814e68940a78392@paul-moore.com>
Date: Sat, 01 Feb 2025 18:48:00 -0500
From: Paul Moore <paul@...l-moore.com>
To: Mickaël Salaün <mic@...ikod.net>
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 February 1, 2025 5:28:37 AM Mickaël Salaün <mic@...ikod.net> 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@...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.

Landlock is a LSM.  Landlock properties *are* LSM properties/attributes, 
regardless of data types.  Look at a lot of the recent work to support 
arbitrary types in the LSM syscalls as well the very recent work to start 
moving away from the rigid secctx/secid concepts within the kernel.

Landlock is a LSM and just as all the other LSMs integrate with other areas 
of the kernel through the LSM framework, so should Landlock. Instead of 
adding landlock_* calls in the kernel, you need to adding security_* calls, 
or better yet, finding ways to work with existing security_* calls.

>>
>> 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 disagree that including LSM info in the existing PID_GET_INFO API 
would be preferable, see my request at the 2024 LPC session on this as well 
as some discussions with Luca, but you will see that Christian has blocked 
this idea and forced us into a LSM specific API. It's not ideal in my 
opinion, but it is better than nothing or having to do our solution in the 
LSM 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