lkml.org 
[lkml]   [2022]   [Jul]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v12 13/26] userns: Add pointer to ima_namespace to user_namespace
    From


    On 5/23/22 10:25, Serge E. Hallyn wrote:
    > On Mon, May 23, 2022 at 02:41:59PM +0200, Christian Brauner wrote:
    >> On Mon, May 23, 2022 at 07:31:29AM -0400, Stefan Berger wrote:
    >>>
    >>>
    >>> On 5/23/22 05:59, Christian Brauner wrote:
    >>>> On Sun, May 22, 2022 at 01:24:26PM -0500, Serge Hallyn wrote:
    >>>>> On Wed, Apr 20, 2022 at 10:06:20AM -0400, Stefan Berger wrote:
    >>>>>> Add a pointer to ima_namespace to the user_namespace and initialize
    >>>>>> the init_user_ns with a pointer to init_ima_ns. We need a pointer from
    >>>>>> the user namespace to its associated IMA namespace since IMA namespaces
    >>>>>> are piggybacking on user namespaces.
    >>>>>>
    >>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
    >>>>>> Acked-by: Christian Brauner <brauner@kernel.org>
    >>>>>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
    >>>>>>
    >>>>>> ---
    >>>>>> v11:
    >>>>>> - Added lost A-b from Christian back
    >>>>>> - Added sentence to patch description explaining why we need the pointer
    >>>>>>
    >>>>>> v9:
    >>>>>> - Deferred implementation of ima_ns_from_user_ns() to later patch
    >>>>>> ---
    >>>>>> include/linux/ima.h | 2 ++
    >>>>>> include/linux/user_namespace.h | 4 ++++
    >>>>>> kernel/user.c | 4 ++++
    >>>>>> 3 files changed, 10 insertions(+)
    >>>>>>
    >>>>>> diff --git a/include/linux/ima.h b/include/linux/ima.h
    >>>>>> index 426b1744215e..fcb60a44e05f 100644
    >>>>>> --- a/include/linux/ima.h
    >>>>>> +++ b/include/linux/ima.h
    >>>>>> @@ -14,6 +14,8 @@
    >>>>>> #include <crypto/hash_info.h>
    >>>>>> struct linux_binprm;
    >>>>>> +extern struct ima_namespace init_ima_ns;
    >>>>>> +
    >>>>>> #ifdef CONFIG_IMA
    >>>>>> extern enum hash_algo ima_get_current_hash_algo(void);
    >>>>>> extern int ima_bprm_check(struct linux_binprm *bprm);
    >>>>>> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
    >>>>>> index 33a4240e6a6f..019e8cf7b633 100644
    >>>>>> --- a/include/linux/user_namespace.h
    >>>>>> +++ b/include/linux/user_namespace.h
    >>>>>> @@ -36,6 +36,7 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */
    >>>>>> #define USERNS_INIT_FLAGS USERNS_SETGROUPS_ALLOWED
    >>>>>> struct ucounts;
    >>>>>> +struct ima_namespace;
    >>>>>> enum ucount_type {
    >>>>>> UCOUNT_USER_NAMESPACES,
    >>>>>> @@ -99,6 +100,9 @@ struct user_namespace {
    >>>>>> #endif
    >>>>>> struct ucounts *ucounts;
    >>>>>> long ucount_max[UCOUNT_COUNTS];
    >>>>>> +#ifdef CONFIG_IMA_NS
    >>>>>
    >>>>> It's probably worth putting a comment here saying that user_ns does not
    >>>>> pin ima_ns.
    >>>>>
    >>>>> That the only time the ima_ns will be freed is when user_ns is freed,
    >>>>> and only time it will be changed is when user_ns is freed, or during
    >>>>> ima_fs_ns_init() (under smp_load_acquire) during a new mount.
    >>>>>
    >>>>>> + struct ima_namespace *ima_ns;
    >>>>>
    >>>>> So, if I create a new user_ns with a new ima_ns, and in there I
    >>>>> create a new user_ns again, it looks like ima_ns will be NULL in
    >>>>> the new user_ns? Should it not be set to the parent->ima_ns?
    >>>>> (which would cause trouble for the way it's currently being
    >>>>> freed...)
    >>>>
    >>>> Would also work and wouldn't be difficult to do imho.
    >>>
    >>> We previously decide to create an ima_namespace when securityfs is mounted.
    >>> This now also applies to nested containers where an IMA namespace is first
    >>> configured with the hash and template to use in a particular container and
    >>> then activated. If no configuration is done it will inherit the hash and
    >>> template from the first ancestor that has been configure when it is
    >>> activated. So the same steps and behavior applies to *all* containers, no
    >>> difference at any depth of nesting. Besides that, we don't want nested
    >>> containers to share policy and logs but keep them isolated from each other,
    >>> or do we not?
    >>>
    >>> Further, how should it work if we were to apply this even to the first
    >>> container? Should it just inherit the &init_ima_namespace and we'd have no
    >>> isolation at all? Why would we start treating containers at deeper nesting
    >>> levels differently?
    >>
    >> Valid points. I understood Serge as suggesting an implementation detail
    >> change not a design change but might misunderstand him here.
    >>
    >> # Currently
    >>
    >> 1. create new userns -> imans set to NULL
    >> 2. mount securityfs and configure imans -> set imans to &new_ima_ns
    >>
    >> When 2. hasn't been done then we find the relevant imans by walking
    >> the userns hierarchy upwards finding the first parent userns that has a
    >> non-NULL imans.
    >
    > Ah, right, thanks Christian.
    >
    > But so the code - I think where the ima_ns is defined in the user_ns
    > struct, needs to make this clear. Just something like
    >
    > // Pointer to ima_ns which this user_ns created. Can be null.
    > // Access checks will walk the userns->parent chain and check
    > // against all active ima_ns's. Note that when the user_ns is
    > // freed, the ima_ns is guaranteed to be free-able.
    > struct ima_namespace *ima_ns;

    I added this comment now. Thanks.

    >
    >> # Serge's suggestion
    >>
    >> 1. create new userns -> imans is set to parent imans
    >> 2. mount securityfs and configure imans -> replace parent with &new_ima_ns
    >>
    >> So when 2. hasn't been done we don't need to walk the userns hierarchy
    >> upwards. We always find the relevant imans directly. Some massaging
    >> would be needed in process_measurement() probably but it wouldn't need
    >> to change semantics per se.
    >>
    >> But I think I misunderstood something in any case. So looking at an
    >> example like ima_post_path_mknod(). You seem to not call into
    >> ima_must_appraise() if the caller's userns doesn't have an imans
    >> enabled. I somehow had thought that the same logic applied as in
    >> process_measurement. But if it isn't then it might make sense to keep
    >> the current implementation.

    \
     
     \ /
      Last update: 2022-07-07 16:16    [W:3.943 / U:0.400 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site