| Message ID | 20230504141330.1557243-1-juraj@jurajmarcin.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: make cleanup on error consistent | expand |
On Thu, May 4, 2023 at 10:14 AM Juraj Marcin <juraj@jurajmarcin.com> 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@jurajmarcin.com> > --- > 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.
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; } /** * security_read_state_kernel - read the policy. - * @data: binary policy data - * @len: length of data in bytes + * @pdata: binary policy data + * @plen: length of data in bytes * * Allocates kernel memory for reading SELinux policy. * This function is for internal use only and should not * be used for returning data to user space. * + * In case of a failure, the pointers are not modified. + * * This function must be called with policy_mutex held. */ -int security_read_state_kernel(void **data, size_t *len) +int security_read_state_kernel(void **pdata, size_t *plen) { int err; + void *data; + size_t len; struct selinux_state *state = &selinux_state; struct selinux_policy *policy; @@ -3985,16 +4000,17 @@ int security_read_state_kernel(void **data, size_t *len) if (!policy) return -EINVAL; - *len = policy->policydb.len; - *data = vmalloc(*len); - if (!*data) + len = policy->policydb.len; + data = vmalloc(len); + if (!data) return -ENOMEM; - err = __security_read_policy(policy, *data, len); + err = __security_read_policy(policy, data, &len); if (err) { - vfree(*data); - *data = NULL; - *len = 0; + vfree(data); + return err; } + *pdata = data; + *plen = len; return err; }
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@jurajmarcin.com> --- security/selinux/include/security.h | 4 +-- security/selinux/selinuxfs.c | 2 -- security/selinux/ss/services.c | 50 +++++++++++++++++++---------- 3 files changed, 35 insertions(+), 21 deletions(-)