lkml.org 
[lkml]   [2020]   [Jul]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 4/5] LSM: Define SELinux function to measure security state
From
Date
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


\
 
 \ /
  Last update: 2020-07-15 20:34    [W:0.039 / U:0.972 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site