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

[Lakshmi Ramasubramanian]

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