[PATCH 3/9] LSM: Manage file security blobs
John Johansen
john.johansen at canonical.com
Tue Oct 31 17:32:28 UTC 2017
On 10/31/2017 09:16 AM, Casey Schaufler wrote:
> On 10/31/2017 8:25 AM, Stephen Smalley wrote:
>> On Fri, 2017-10-27 at 14:45 -0700, Casey Schaufler wrote:
>>> Subject: [PATCH 3/9] LSM: Manage file security blobs
>>>
>>> Move the management of file security blobs from the individual
>>> security modules to the security infrastructure. The security modules
>>> using file blobs have been updated accordingly. Modules are required
>>> to identify the space they need at module initialization. In some
>>> cases a module no longer needs to supply a blob management hook, in
>>> which case the hook has been removed.
>>>
>>> Signed-off-by: Casey Schaufler <casey at schaufler-ca.com>
>>> ---
>>> include/linux/lsm_hooks.h | 1 +
>>> security/apparmor/include/context.h | 5 +++++
>>> security/apparmor/include/file.h | 2 +-
>>> security/apparmor/lsm.c | 19 ++++++++--------
>>> security/security.c | 43
>>> +++++++++++++++++++++++++++++++++++++
>>> security/selinux/hooks.c | 41 +++++++++----------------
>>> ----------
>>> security/selinux/include/objsec.h | 5 +++++
>>> security/smack/smack.h | 5 +++++
>>> security/smack/smack_lsm.c | 26 ++++++++--------------
>>> 9 files changed, 89 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index ee4fcc51fa91..e5d0f1e01b81 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -1919,6 +1919,7 @@ struct security_hook_list {
>>> */
>>> struct lsm_blob_sizes {
>>> int lbs_cred;
>>> + int lbs_file;
>>> };
>>>
>>> /*
>>> diff --git a/security/apparmor/include/context.h
>>> b/security/apparmor/include/context.h
>>> index 301ab3a0dd04..c6e106a533e8 100644
>>> --- a/security/apparmor/include/context.h
>>> +++ b/security/apparmor/include/context.h
>>> @@ -87,6 +87,11 @@ static inline struct aa_label
>>> *aa_get_newest_cred_label(const struct cred *cred)
>>> return aa_get_newest_label(aa_cred_raw_label(cred));
>>> }
>>>
>>> +static inline struct aa_file_ctx *apparmor_file(const struct file
>>> *file)
>>> +{
>>> + return file->f_security;
>>> +}
>>> +
>>> /**
>>> * __aa_task_raw_label - retrieve another task's label
>>> * @task: task to query (NOT NULL)
>>> diff --git a/security/apparmor/include/file.h
>>> b/security/apparmor/include/file.h
>>> index 4c2c8ac8842f..b9efe6bc226b 100644
>>> --- a/security/apparmor/include/file.h
>>> +++ b/security/apparmor/include/file.h
>>> @@ -32,7 +32,7 @@ struct path;
>>> AA_MAY_CHMOD | AA_MAY_CHOWN |
>>> AA_MAY_LOCK | \
>>> AA_EXEC_MMAP | AA_MAY_LINK)
>>>
>>> -#define file_ctx(X) ((struct aa_file_ctx *)(X)->f_security)
>>> +#define file_ctx(X) apparmor_file(X)
>>>
>>> /* struct aa_file_ctx - the AppArmor context the file was opened in
>>> * @lock: lock to update the ctx
>>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>>> index d80293bde5bf..f2814ba84481 100644
>>> --- a/security/apparmor/lsm.c
>>> +++ b/security/apparmor/lsm.c
>>> @@ -402,21 +402,21 @@ static int apparmor_file_open(struct file
>>> *file, const struct cred *cred)
>>>
>>> static int apparmor_file_alloc_security(struct file *file)
>>> {
>>> - int error = 0;
>>> -
>>> - /* freed by apparmor_file_free_security */
>>> + struct aa_file_ctx *ctx = file_ctx(file);
>>> struct aa_label *label = begin_current_label_crit_section();
>>> - file->f_security = aa_alloc_file_ctx(label, GFP_KERNEL);
>>> - if (!file_ctx(file))
>>> - error = -ENOMEM;
>>> - end_current_label_crit_section(label);
>>>
>>> - return error;
>>> + spin_lock_init(&ctx->lock);
>>> + rcu_assign_pointer(ctx->label, aa_get_label(label));
>>> + end_current_label_crit_section(label);
>>> + return 0;
>>> }
>>>
>>> static void apparmor_file_free_security(struct file *file)
>>> {
>>> - aa_free_file_ctx(file_ctx(file));
>>> + struct aa_file_ctx *ctx = file_ctx(file);
>>> +
>>> + if (ctx)
>>> + aa_put_label(rcu_access_pointer(ctx->label));
>>> }
>>>
>>> static int common_file_perm(const char *op, struct file *file, u32
>>> mask)
>>> @@ -1078,6 +1078,7 @@ static void apparmor_sock_graft(struct sock
>>> *sk, struct socket *parent)
>>>
>>> struct lsm_blob_sizes apparmor_blob_sizes = {
>>> .lbs_cred = sizeof(struct aa_task_ctx),
>>> + .lbs_file = sizeof(struct aa_file_ctx),
>>> };
>>>
>>> static struct security_hook_list apparmor_hooks[]
>>> __lsm_ro_after_init = {
>>> diff --git a/security/security.c b/security/security.c
>>> index 6fadc3860fb0..4d8e702fa22f 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -37,6 +37,8 @@
>>> struct security_hook_heads security_hook_heads __lsm_ro_after_init;
>>> static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
>>>
>>> +static struct kmem_cache *lsm_file_cache;
>>> +
>>> char *lsm_names;
>>> static struct lsm_blob_sizes blob_sizes;
>>>
>>> @@ -83,6 +85,13 @@ int __init security_init(void)
>>> do_security_initcalls();
>>>
>>> /*
>>> + * Create any kmem_caches needed for blobs
>>> + */
>>> + if (blob_sizes.lbs_file)
>>> + lsm_file_cache = kmem_cache_create("lsm_file_cache",
>>> + blob_sizes.lbs_fi
>>> le, 0,
>>> + SLAB_PANIC,
>>> NULL);
>>> + /*
>>> * The second call to a module specific init function
>>> * adds hooks to the hook lists and does any other early
>>> * initializations required.
>>> @@ -91,6 +100,7 @@ int __init security_init(void)
>>>
>>> #ifdef CONFIG_SECURITY_LSM_DEBUG
>>> pr_info("LSM: cred blob size = %d\n",
>>> blob_sizes.lbs_cred);
>>> + pr_info("LSM: file blob size = %d\n",
>>> blob_sizes.lbs_file);
>>> #endif
>>>
>>> return 0;
>>> @@ -267,6 +277,26 @@ static void __init lsm_set_size(int *need, int
>>> *lbs)
>>> void __init security_add_blobs(struct lsm_blob_sizes *needed)
>>> {
>>> lsm_set_size(&needed->lbs_cred, &blob_sizes.lbs_cred);
>>> + lsm_set_size(&needed->lbs_file, &blob_sizes.lbs_file);
>>> +}
>>> +
>>> +/**
>>> + * lsm_file_alloc - allocate a composite file blob
>>> + * @file: the file that needs a blob
>>> + *
>>> + * Allocate the file blob for all the modules
>>> + *
>>> + * Returns 0, or -ENOMEM if memory can't be allocated.
>>> + */
>>> +int lsm_file_alloc(struct file *file)
>>> +{
>>> + if (!lsm_file_cache)
>>> + return 0;
>>> +
>>> + file->f_security = kmem_cache_zalloc(lsm_file_cache,
>>> GFP_KERNEL);
>>> + if (file->f_security == NULL)
>>> + return -ENOMEM;
>>> + return 0;
>>> }
>>>
>>> /*
>>> @@ -952,12 +982,25 @@ int security_file_permission(struct file *file,
>>> int mask)
>>>
>>> int security_file_alloc(struct file *file)
>>> {
>>> + int rc = lsm_file_alloc(file);
>>> +
>>> + if (rc)
>>> + return rc;
>>> return call_int_hook(file_alloc_security, 0, file);
>> Suppose that a module's file_alloc_security() hook returns an error.
>> What should happen to the blob allocated by lsm_file_alloc()? In
>> general, callers assumes that security_file_alloc() handles cleanup
>> internally if it returns an error and do not call security_file_free();
>> this is also true of other similar alloc hooks I believe. Further, if
>> we allow the module file_alloc_security() hooks to perform any
>> allocation themselves, then we have a similar problem with regard to
>> cleanup if any one of them fails; to be fully safe, we'd need to call
>> the file_free_security() hook on the ones that had previously returned
>> success. Either we need to handle such errors within
>> security_file_alloc(), or we need to dispense with the ability to
>> allocate and return an error code from the module's
>> file_alloc_security(), i.e. make them return void, and probably rename
>> them to file_init_security() or similar.
>
> I like the idea of changing file_alloc_security() to file_init_security()
> or maybe file_setup_security() and making the hook a void function. If a
> module wants to allocate space on its own it will need to deal with the
> fact that it may have been unable to do so. I hesitate to prohibit modules
> from allocating their own space because someone is going to want to have a
> list of attributes. Trying to manage memory that you don't know about is
> a loosing proposition.
>
Changing it to a void is just going to lead to LSMs that handle this them
selves having to deny every access of the object, because that is the only
sane thing they can do if the data they need isn't present.
It far better to have the one failure upfront than having an LSM rejecting
every access to the object after the fact. And looking down the road to
namespacing for containers I don't see away to handle some of the things
that will be needed without an LSM doing allocations and managing stuff
internally, but thats an argument for a different patch series.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
More information about the Linux-security-module-archive
mailing list