Messages in this thread | | | Date | Thu, 7 Jul 2022 10:14:03 -0400 | Subject | Re: [PATCH v12 13/26] userns: Add pointer to ima_namespace to user_namespace | From | Stefan Berger <> |
| |
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.
| |