Re: [PATCH v8 02/28] LSM: Infrastructure management of the sock security

From: John Johansen <john.johansen@canonical.com>
To: Stephen Smalley <sds@tycho.nsa.gov>,
	Casey Schaufler <casey@schaufler-ca.com>,
	casey.schaufler@intel.com, jmorris@namei.org,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org
Cc: keescook@chromium.org, penguin-kernel@i-love.sakura.ne.jp,
	paul@paul-moore.com
Subject: Re: [PATCH v8 02/28] LSM: Infrastructure management of the sock security
Date: Wed, 18 Sep 2019 00:19:02 -0700	[thread overview]
Message-ID: <e82206ce-c37d-b8e4-1ba4-c7c49afb5221@canonical.com> (raw)
In-Reply-To: <5fde58fe-3925-c9d6-39bf-9adb318f7186@tycho.nsa.gov>

On 9/16/19 11:42 AM, Stephen Smalley wrote:
> On 8/29/19 7:29 PM, Casey Schaufler wrote:
>> Move management of the sock->sk_security blob out
>> of the individual security modules and into the security
>> infrastructure. Instead of allocating the blobs from within
>> the modules the modules tell the infrastructure how much
>> space is required, and the space is allocated there.
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Reviewed-by: John Johansen <john.johansen@canonical.com>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> 
> One oddity noted below, but it isn't introduced by this patch so you can add my:
> 
> Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> 
>> ---
>>   include/linux/lsm_hooks.h         |  1 +
>>   security/apparmor/include/net.h   |  6 ++-
>>   security/apparmor/lsm.c           | 38 ++++-----------
>>   security/security.c               | 36 +++++++++++++-
>>   security/selinux/hooks.c          | 78 +++++++++++++++----------------
>>   security/selinux/include/objsec.h |  5 ++
>>   security/selinux/netlabel.c       | 23 ++++-----
>>   security/smack/smack.h            |  5 ++
>>   security/smack/smack_lsm.c        | 64 ++++++++++++-------------
>>   security/smack/smack_netfilter.c  |  8 ++--
>>   10 files changed, 144 insertions(+), 120 deletions(-)
>>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index f9222a04968d..b353482ea348 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -2047,6 +2047,7 @@ struct lsm_blob_sizes {
>>       int    lbs_cred;
>>       int    lbs_file;
>>       int    lbs_inode;
>> +    int    lbs_sock;
>>       int    lbs_superblock;
>>       int    lbs_ipc;
>>       int    lbs_msg_msg;
>> diff --git a/security/apparmor/include/net.h b/security/apparmor/include/net.h
>> index 7334ac966d01..adac04e3b3cc 100644
>> --- a/security/apparmor/include/net.h
>> +++ b/security/apparmor/include/net.h
>> @@ -55,7 +55,11 @@ struct aa_sk_ctx {
>>       struct aa_label *peer;
>>   };
>>   -#define SK_CTX(X) ((X)->sk_security)
>> +static inline struct aa_sk_ctx *aa_sock(const struct sock *sk)
>> +{
>> +    return sk->sk_security + apparmor_blob_sizes.lbs_sock;
>> +}
>> +
>>   #define SOCK_ctx(X) SOCK_INODE(X)->i_security
> 
> This use of i_security looks suspicious, but SOCK_ctx doesn't appear to be used presently.  Probably should be removed in a separate patch.
> 

yes this leaked is from some in dev patches that leaked into the base socket mediation patch. I can put together a patch to remove it