Re: [PATCH v1 4/5] LSM: Define SELinux function to measure security state

From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
To: Stephen Smalley <stephen.smalley.work@gmail.com>
Cc: Mimi Zohar <zohar@linux.ibm.com>,
	Casey Schaufler <casey@schaufler-ca.com>,
	James Morris <jmorris@namei.org>,
	linux-integrity@vger.kernel.org,
	SElinux list <selinux@vger.kernel.org>,
	LSM List <linux-security-module@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 4/5] LSM: Define SELinux function to measure security state
Date: Wed, 15 Jul 2020 11:34:07 -0700	[thread overview]
Message-ID: <fec80278-1306-787d-a8ed-615a3709ae77@linux.microsoft.com> (raw)
In-Reply-To: <CAEjxPJ6UsK9QqFTpMKjgSin2Q6D-5NCNDS0enuRNuixVP9H2wQ@mail.gmail.com>

On 7/15/20 11:04 AM, Stephen Smalley wrote:

>> +static inline bool selinux_checkreqprot(void)
>> +{
>> +       struct selinux_state *state = &selinux_state;
>> +
>> +       return state->checkreqprot;
>> +}
> 
> Probably should use READ_ONCE().
Will do.

> 
>> diff --git a/security/selinux/measure.c b/security/selinux/measure.c
>> new file mode 100644
>> index 000000000000..b909e8e61542
>> --- /dev/null
>> +++ b/security/selinux/measure.c
>> @@ -0,0 +1,122 @@
>> +int selinux_security_state(void)
> 
> Let's call this selinux_measure_state() or similar.  Needs a verb. And
> pass it a struct selinux_state * pointer argument to be measured, even
> though initially it will always be passed &selinux_state.  The
> encapsulation of selinux state within selinux_state was to support
> multiple selinux namespaces in the future, each with their own state.
Will do.

>> +       static char *security_state_string =
>> +                       "enabled=%d;enforcing=%d;checkreqprot=%d;"        \
>> +                       "netpeer=%d;openperm=%d;extsockclass=%d;"         \
>> +                       "alwaysnetwork=%d;cgroupseclabel=%d;"             \
>> +                       "nnpnosuidtransition=%d;genfsseclabelsymlink=%d;";
> 
> Rather than hardcoding policy capability names here, I would recommend
> using the selinux_policycap_names[] array for the names and the
> selinux_state.policycap[] array for the values.  Also recommend
> passing in a struct selinux_state * here to allow for future case
> where there are multiple selinux states, one per selinux namespace.
Will do.

> 
>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
>> index ef0afd878bfc..0c289d13ef6a 100644
>> --- a/security/selinux/ss/services.c
>> +++ b/security/selinux/ss/services.c
>> @@ -3724,10 +3724,11 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
>>    * security_read_policy - read the policy.
>>    * @data: binary policy data
>>    * @len: length of data in bytes
>> - *
>> + * @alloc_kernel_memory: flag to indicate memory allocation
>>    */
>> -int security_read_policy(struct selinux_state *state,
>> -                        void **data, size_t *len)
>> +int security_read_selinux_policy(struct selinux_state *state,
>> +                                void **data, size_t *len,
>> +                                bool alloc_kernel_memory)
> 
> Instead of passing in a boolean to control how the memory is
> allocated, split this into a helper function that takes an
> already-allocated buffer and two
> different front-end wrappers, one for kernel-internal use and one for
> userspace use.
Will do.

> 
>> @@ -3738,7 +3739,10 @@ int security_read_policy(struct selinux_state *state,
>>
>>          *len = security_policydb_len(state);
>>
>> -       *data = vmalloc_user(*len);
>> +       if (alloc_kernel_memory)
>> +               *data = kzalloc(*len, GFP_KERNEL);
> 
> You need vmalloc() since policy image size may exceed kmalloc max (or
> at least that used to be the case).
Will do.

thanks,
  -lakshmi