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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <fec80278-1306-787d-a8ed-615a3709ae77@linux.microsoft.com>
Date:   Wed, 15 Jul 2020 11:34:07 -0700
From:   Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>
To:     Stephen Smalley <stephen.smalley.work@...il.com>
Cc:     Mimi Zohar <zohar@...ux.ibm.com>,
        Casey Schaufler <casey@...aufler-ca.com>,
        James Morris <jmorris@...ei.org>,
        linux-integrity@...r.kernel.org,
        SElinux list <selinux@...r.kernel.org>,
        LSM List <linux-security-module@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 4/5] LSM: Define SELinux function to measure security
 state

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