From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752454AbeCaVe6 (ORCPT ); Sat, 31 Mar 2018 17:34:58 -0400 Received: from sonic316-20.consmr.mail.bf2.yahoo.com ([74.6.130.194]:35461 "EHLO sonic316-20.consmr.mail.bf2.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751305AbeCaVe4 (ORCPT ); Sat, 31 Mar 2018 17:34:56 -0400 X-YMail-OSG: kvW5sD4VM1n.y8ecBHJegpZ6JxtfZQuoPws8GtKwsFOxfjC1nhPvC4FGhr919PM CTzwm.rik0Q2ZdTBXb1pxyAJqo_DNyYPLGBxpgPd5EYM5q6id1lT60XD9AO3rBPLmaif0YmJMgYn GGVwHIyJ27fXHCiqRZk4H.zRyzlosDVD2adx.SiG7aoNEOMTaVdyxI.U_kN6zy1SY3_eRmZ_.Xjv jqASvV7zBm4ihiVFBbk8wF4lZSCkgI2lN9ljvpbg8BNa7tKabwj44qisq0d7ji90AQdNvPA6Srd4 Uopbaa0yh5mGuouInceTexp5mG0rWIBxiLvQHrjo_nB4G2TjaswCgvohw_Z5fLqJaG2kGA65Zi.A GjA9gd4Gzc0sn0jLr_8_0MtAso9QmpAFhno_sweQNp64r5mb1nttZukzqOAUIj0Y1eprjh.mAuKC kvgpe9HBxnLsclqs_WyfxHwN3htubRpOSYq_D1Hhutic2T3z0LwF1PkhVBkqmzgR32NNIQHVMGm6 I5ScoDQkgq48onL3s7zPw2ZcJibpeIMei_hprDNgOtSGj2xIV1cY- Subject: Re: [PATCH v3 1/1] security: Add mechanism to safely (un)load LSMs after boot time To: Sargun Dhillon Cc: LSM , LKML , Tetsuo Handa , Kees Cook , Igor Stoppa , James Morris , Stephen Smalley , Paul Moore , plautrba@redhat.com References: <5240a81494bbe5be5651379cd9828bda34d764e2.1522357211.git.sargun@sargun.me> <3292bbb3-7c9d-f18d-05f3-9b69b3cac922@schaufler-ca.com> <20180330023331.GA10833@ircssh-2.c.rugged-nimbus-611.internal> <8b015e9e-dccb-aefd-f5b2-c66eed3c6815@schaufler-ca.com> From: Casey Schaufler Message-ID: <6e9cf9f5-dec9-3e8f-6439-418be708da67@schaufler-ca.com> Date: Sat, 31 Mar 2018 14:34:52 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 3/30/2018 11:16 PM, Sargun Dhillon wrote: > On Fri, Mar 30, 2018 at 2:39 PM, Casey Schaufler wrote: >> On 3/29/2018 7:33 PM, Sargun Dhillon wrote: >>> On Thu, Mar 29, 2018 at 02:37:10PM -0700, Casey Schaufler wrote: >>>> On 3/29/2018 2:14 PM, Sargun Dhillon wrote: >>>>> This patch introduces a mechanism to add mutable hooks and immutable >>>>> hooks to the callback chain. It adds an intermediary item to the >>>>> chain which separates mutable and immutable hooks. Immutable hooks >>>>> are then marked as read-only, as well as the hook heads. This does >>>>> not preclude some hooks being able to be mutated (removed). >>>>> >>>>> It also wraps the hook unloading, and execution with an SRCU. One >>>>> SRCU is used across all hooks, as the SRCU struct can be memory >>>>> intensive, and hook execution time in general should be relatively >>>>> short. >>>>> >>>>> Signed-off-by: Sargun Dhillon >>>>> Signed-off-by: Tetsuo Handa >>>>> --- >>>>> include/linux/lsm_hooks.h | 23 ++--- >>>>> security/Kconfig | 2 +- >>>>> security/apparmor/lsm.c | 2 +- >>>>> security/commoncap.c | 2 +- >>>>> security/security.c | 210 ++++++++++++++++++++++++++++++++++++++------- >>>>> security/selinux/hooks.c | 5 +- >>>>> security/smack/smack_lsm.c | 3 +- >>>>> security/tomoyo/tomoyo.c | 3 +- >>>>> security/yama/yama_lsm.c | 2 +- >>>>> 9 files changed, 196 insertions(+), 56 deletions(-) >>>>> >>>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>>>> index 09bc60fb35f1..689e5e72fb38 100644 >>>>> --- a/include/linux/lsm_hooks.h >>>>> +++ b/include/linux/lsm_hooks.h >>>>> @@ -1981,9 +1981,12 @@ extern struct security_hook_heads security_hook_heads; >>>>> extern char *lsm_names; >>>>> >>>>> extern void security_add_hooks(struct security_hook_list *hooks, int count, >>>>> - char *lsm); >>>>> + char *lsm, bool is_mutable); >>>>> >>>>> -#ifdef CONFIG_SECURITY_SELINUX_DISABLE >>>>> +#define __lsm_ro_after_init __ro_after_init >>>>> +/* Currently required to handle SELinux runtime hook disable. */ >>>>> +#ifdef CONFIG_SECURITY_WRITABLE_HOOKS >>>>> +#define __lsm_mutable_after_init >>>>> /* >>>>> * Assuring the safety of deleting a security module is up to >>>>> * the security module involved. This may entail ordering the >>>>> @@ -1996,21 +1999,9 @@ extern void security_add_hooks(struct security_hook_list *hooks, int count, >>>>> * disabling their module is a good idea needs to be at least as >>>>> * careful as the SELinux team. >>>>> */ >>>>> -static inline void security_delete_hooks(struct security_hook_list *hooks, >>>>> - int count) >>>>> -{ >>>>> - int i; >>>>> - >>>>> - for (i = 0; i < count; i++) >>>>> - hlist_del_rcu(&hooks[i].list); >>>>> -} >>>>> -#endif /* CONFIG_SECURITY_SELINUX_DISABLE */ >>>>> - >>>>> -/* Currently required to handle SELinux runtime hook disable. */ >>>>> -#ifdef CONFIG_SECURITY_WRITABLE_HOOKS >>>>> -#define __lsm_ro_after_init >>>>> +extern void security_delete_hooks(struct security_hook_list *hooks, int count); >>>>> #else >>>>> -#define __lsm_ro_after_init __ro_after_init >>>>> +#define __lsm_mutable_after_init __ro_after_init >>>>> #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */ >>>>> >>>>> extern int __init security_module_enable(const char *module); >>>>> diff --git a/security/Kconfig b/security/Kconfig >>>>> index c4302067a3ad..a3b8b1142e6f 100644 >>>>> --- a/security/Kconfig >>>>> +++ b/security/Kconfig >>>>> @@ -32,7 +32,7 @@ config SECURITY >>>>> If you are unsure how to answer this question, answer N. >>>>> >>>>> config SECURITY_WRITABLE_HOOKS >>>>> - depends on SECURITY >>>>> + depends on SECURITY && SRCU >>>>> bool >>>>> default n >>>>> >>>>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c >>>>> index 9a65eeaf7dfa..d6cca8169df0 100644 >>>>> --- a/security/apparmor/lsm.c >>>>> +++ b/security/apparmor/lsm.c >>>>> @@ -1155,7 +1155,7 @@ static int __init apparmor_init(void) >>>>> goto buffers_out; >>>>> } >>>>> security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks), >>>>> - "apparmor"); >>>>> + "apparmor", false); >>>>> >>>>> /* Report that AppArmor successfully initialized */ >>>>> apparmor_initialized = 1; >>>>> diff --git a/security/commoncap.c b/security/commoncap.c >>>>> index 48620c93d697..fe4b0d9d44ce 100644 >>>>> --- a/security/commoncap.c >>>>> +++ b/security/commoncap.c >>>>> @@ -1363,7 +1363,7 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = { >>>>> void __init capability_add_hooks(void) >>>>> { >>>>> security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks), >>>>> - "capability"); >>>>> + "capability", false); >>>>> } >>>>> >>>>> #endif /* CONFIG_SECURITY */ >>>>> diff --git a/security/security.c b/security/security.c >>>>> index 3cafff61b049..2ddb64864e3e 100644 >>>>> --- a/security/security.c >>>>> +++ b/security/security.c >>>>> @@ -29,6 +29,11 @@ >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>> +#include >>>>> + >>>>> +#define SECURITY_HOOK_COUNT \ >>>>> + (sizeof(security_hook_heads) / sizeof(struct hlist_head)) >>>>> >>>>> #define MAX_LSM_EVM_XATTR 2 >>>>> >>>>> @@ -36,7 +41,10 @@ >>>>> #define SECURITY_NAME_MAX 10 >>>>> >>>>> struct security_hook_heads security_hook_heads __lsm_ro_after_init; >>>>> +EXPORT_SYMBOL_GPL(security_hook_heads); >>>>> + >>>>> static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain); >>>>> +static DEFINE_MUTEX(security_hook_mutex); >>>>> >>>>> char *lsm_names; >>>>> /* Boot-time LSM user choice */ >>>>> @@ -53,6 +61,103 @@ static void __init do_security_initcalls(void) >>>>> } >>>>> } >>>>> >>>>> +#ifdef CONFIG_SECURITY_WRITABLE_HOOKS >>>>> +DEFINE_STATIC_SRCU(security_hook_srcu); >>>>> +static struct security_hook_list null_hooks[SECURITY_HOOK_COUNT]; >>>>> +#define HAS_FUNC(SHL, FUNC) (SHL->hook.FUNC) >>>> The HAS_FUNC() macro will work, but it's awkward outside of the >>>> call_..._hook() macros. I think you should document how to use it >>>> properly somewhere in here. There are enough cases where the >>>> call_..._hook() macros aren't used that someone could have trouble >>>> figuring out how to use it. >>>> >>>> >>> What about something like: >>> >>> security/security.c | 58 ++++++++++++++++++++++++++++++++++++++--------------- >>> 1 file changed, 42 insertions(+), 16 deletions(-) >>> >>> diff --git a/security/security.c b/security/security.c >>> index 2ddb64864e3e..bc14125cfc78 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -62,9 +62,37 @@ static void __init do_security_initcalls(void) >>> } >>> >>> #ifdef CONFIG_SECURITY_WRITABLE_HOOKS >>> -DEFINE_STATIC_SRCU(security_hook_srcu); >>> +/* >>> + * With writable hooks, we setup a structure like this: >>> + * +------+ +-----------+ +-----------+ +-----------+ +--------------+ >>> + * | | | | | | | | | | >>> + * | HEAD +---> Immutable +---> Immutable +---> Null hook +---> Mutable Hook | >>> + * | | | Hook 1 | | Hook 2 | | | | | >>> + * +------+ +-----------+ +-----------+ +-----------+ +--------------+ >>> + * | | | >>> + * v v v >>> + * Callback Callback Callback >>> + * >>> + * The hooks before to null hook are marked only after kernel initialization. >>> + * The null hook, as well as the hooks succeeding it are not marked read only, >>> + * therefore allowing them be (un)loaded after initialization time. >>> + * >>> + * Since the null hook doesn't have a callback, we need to check if a hook >>> + * is the null hook prior to invoking it. >>> + */ >> I think a comment like this is helpful. >> >> Why not have two hook list heads, one for regular hooks and >> one for mutable hooks? You can dispense with the "null hook" >> handling. >> > The iterations gets really messy. The patch earlier had a lot of > awkward gunk in it. The issue primarily came with deciding to iterate > over the second set of hooks iff the first set of hooks failed. With > the way the calls are setup, it's messy. Hang on. I would think you'd only call the 2nd list if the first list was completed successfully. And of course the void hooks always get called regardless. > What's your primary issue with the "null hooks"? If it's the null hook > checks, I think I can actually remove those. In the common case of one LSM and no mutable modules you are making a check on every hook that will always have the same answer. Performance impact *of any amount* that does not add value in the common case has to be avoided. This is a clever implementation. But it does not generalize. If I wanted to add a third kind of hooks the mechanism would not support it. If you add a 2nd list it's obvious what you'd want to do to add a 3rd. > >>> static struct security_hook_list null_hooks[SECURITY_HOOK_COUNT]; >>> -#define HAS_FUNC(SHL, FUNC) (SHL->hook.FUNC) >>> +DEFINE_STATIC_SRCU(security_hook_srcu); >>> + >>> +static inline bool is_null_hook(struct security_hook_list *shl) >>> +{ >>> + union { >>> + void *cb_ptr; >>> + union security_list_options slo; >>> + } hook_options; >>> + >>> + hook_options.slo = shl->hook; >>> + return !hook_options.cb_ptr; >>> +} >> I like the HAS_FUNC() approach better. > Just curious, why? I personally prefer small static inline functions > over macros, if possible. Minimize the complexity, even inside a macro/function. Your is_null_hook() function is massively more complicated than the HAS_FUNC() macro. I'm still not 100% sure I understand why you have the wacky union, and how you can know that it will get you the right result on all architectures. > >> What I think would work best is to have a separate list head >> for the mutable hooks. >> >> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >