[PATCH] selinux: make cleanup on error consistent

[Juraj Marcin <juraj@xxxxxxxxxxxxxxx>]

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;
 }
 
 /**
  * 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;
 }
-- 
2.39.2