On Thu, May 4, 2023 at 10:14 AM Juraj Marcin <juraj@xxxxxxxxxxxxxxx> wrote:
>
> The file security.c contains two functions (security_read_policy() and
> security_read_state_kernel()) that are almost identical, but their
> cleanup conventions differ.
>
> This patch unifies the behavior by adding cleanup to
> security_read_policy() in case some call inside it fails instead of
> relying on the caller to properly free the memory. On top of that, this
> patch future-proofs both functions by adding local variables and
> modifying the pointers only in case of a success.
>
> Signed-off-by: Juraj Marcin <juraj@xxxxxxxxxxxxxxx>
> ---
> security/selinux/include/security.h | 4 +--
> security/selinux/selinuxfs.c | 2 --
> security/selinux/ss/services.c | 50 +++++++++++++++++++----------
> 3 files changed, 35 insertions(+), 21 deletions(-)
>
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index 8746fafeb7789..2990b3d08236d 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -213,8 +213,8 @@ int security_load_policy(void *data, size_t len,
> struct selinux_load_state *load_state);
> void selinux_policy_commit(struct selinux_load_state *load_state);
> void selinux_policy_cancel(struct selinux_load_state *load_state);
> -int security_read_policy(void **data, size_t *len);
> -int security_read_state_kernel(void **data, size_t *len);
> +int security_read_policy(void **pdata, size_t *plen);
> +int security_read_state_kernel(void **pdata, size_t *plen);
> int security_policycap_supported(unsigned int req_cap);
>
> #define SEL_VEC_MAX 32
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 69a583b91fc57..6d4cd66360739 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -406,8 +406,6 @@ static int sel_open_policy(struct inode *inode, struct file *filp)
> err:
> mutex_unlock(&selinux_state.policy_mutex);
>
> - if (plm)
> - vfree(plm->data);
> kfree(plm);
> return rc;
> }
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index f14d1ffe54c5d..f2fd2b6510560 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -3941,12 +3941,16 @@ static int __security_read_policy(struct selinux_policy *policy,
>
> /**
> * security_read_policy - read the policy.
> - * @data: binary policy data
> - * @len: length of data in bytes
> + * @pdata: binary policy data
> + * @plen: length of data in bytes
> *
> + * In case of a failure, the pointers are not modified.
> */
> -int security_read_policy(void **data, size_t *len)
> +int security_read_policy(void **pdata, size_t *plen)
> {
> + int err;
> + void *data;
> + size_t len;
> struct selinux_state *state = &selinux_state;
> struct selinux_policy *policy;
>
> @@ -3955,28 +3959,39 @@ int security_read_policy(void **data, size_t *len)
> if (!policy)
> return -EINVAL;
>
> - *len = policy->policydb.len;
> - *data = vmalloc_user(*len);
> - if (!*data)
> + len = policy->policydb.len;
> + data = vmalloc_user(len);
> + if (!data)
> return -ENOMEM;
>
> - return __security_read_policy(policy, *data, len);
> + err = __security_read_policy(policy, data, &len);
> + if (err) {
> + vfree(data);
> + return err;
> + }
> + *pdata = data;
> + *plen = len;
> + return err;
> }
Hi Juraj,
Thank you for your patch, but I prefer the code as it is currently
written. I feel it is more maintainable to free the @data buffer at
the same time as we free the policy_load_memory struct which contains
the pointer to the @data buffer.
If there was a possibility that we could unify security_read_policy()
and security_read_state_kernel() then it might be worthwhile to
shuffle some of the allocations, but given the need to allocate a user
pointer in security_read_policy() and a desire preserve a level of
abstraction* between the security server and the rest of the SELinux
code I don't think this change is something I want to merge. I'm
sorry.
* As time goes on, I'm less and less interested in preserving the
separation between the SELinux security server and the SELinux
Linux/hooks code, however there are some that feel strongly about this
so we'll stick with it for now.
--
paul-moore.com
