Messages in this thread | | | Subject | Re: [PATCH v1 4/5] LSM: Define SELinux function to measure security state | From | Lakshmi Ramasubramanian <> | Date | Wed, 15 Jul 2020 11:34:07 -0700 |
| |
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
| |