selinux: fix error handling bugs in security_load_policy() (0256b0aa) · Commits · 戴 / test

There are a few bugs in the error handling for security_load_policy(). 1) If the newpolicy->sidtab allocation fails then it leads to a NULL dereference. Also the error code was not set to -ENOMEM on that path. 2) If policydb_read() failed then we call policydb_destroy() twice which meands we call kvfree(p->sym_val_to_name[i]) twice. 3) If policydb_load_isids() failed then we call sidtab_destroy() twice and that results in a double free in the sidtab_destroy_tree() function because entry.ptr_inner and entry.ptr_leaf are not set to NULL. One thing that makes this code nice to deal with is that none of the functions return partially allocated data. In other words, the policydb_read() either allocates everything successfully or it frees all the data it allocates. It never returns a mix of allocated and not allocated data. I re-wrote this to only free the successfully allocated data which avoids the double frees. I also re-ordered selinux_policy_free() so it's in the reverse order of the allocation function. Fixes: ("selinux: refactor changing booleans") Acked-by:

Stephen Smalley <stephen.smalley.work@gmail.com> Signed-off-by:

Dan Carpenter <dan.carpenter@oracle.com> [PM: partially merged by hand due to merge fuzz] Signed-off-by:

Paul Moore <paul@paul-moore.com>

security/selinux/ss/services.c

+23 −11

Original line number	Diff line number	Diff line
		@@ -2145,10 +2145,10 @@ static void selinux_policy_free(struct selinux_policy *policy)
		if (!policy)
		return;

		policydb_destroy(&policy->policydb);
		sidtab_destroy(policy->sidtab);
		kfree(policy->sidtab);
		kfree(policy->map.mapping);
		policydb_destroy(&policy->policydb);
		kfree(policy->sidtab);
		kfree(policy);
		}

		@@ -2263,23 +2263,25 @@ int security_load_policy(struct selinux_state state, void data, size_t len,
		return -ENOMEM;

		newpolicy->sidtab = kzalloc(sizeof(*newpolicy->sidtab), GFP_KERNEL);
		if (!newpolicy->sidtab)
		goto err;
		if (!newpolicy->sidtab) {
		rc = -ENOMEM;
		goto err_policy;
		}

		rc = policydb_read(&newpolicy->policydb, fp);
		if (rc)
		goto err;
		goto err_sidtab;

		newpolicy->policydb.len = len;
		rc = selinux_set_mapping(&newpolicy->policydb, secclass_map,
		&newpolicy->map);
		if (rc)
		goto err;
		goto err_policydb;

		rc = policydb_load_isids(&newpolicy->policydb, newpolicy->sidtab);
		if (rc) {
		pr_err("SELinux: unable to load the initial SIDs\n");
		goto err;
		goto err_mapping;
		}


		@@ -2301,7 +2303,7 @@ int security_load_policy(struct selinux_state state, void data, size_t len,
		rc = security_preserve_bools(oldpolicy, newpolicy);
		if (rc) {
		pr_err("SELinux: unable to preserve booleans\n");
		goto err;
		goto err_free_isids;
		}

		/*
		@@ -2321,13 +2323,23 @@ int security_load_policy(struct selinux_state state, void data, size_t len,
		pr_err("SELinux: unable to convert the internal"
		" representation of contexts in the new SID"
		" table\n");
		goto err;
		goto err_free_isids;
		}

		*newpolicyp = newpolicy;
		return 0;
		err:
		selinux_policy_free(newpolicy);

		err_free_isids:
		sidtab_destroy(newpolicy->sidtab);
		err_mapping:
		kfree(newpolicy->map.mapping);
		err_policydb:
		policydb_destroy(&newpolicy->policydb);
		err_sidtab:
		kfree(newpolicy->sidtab);
		err_policy:
		kfree(newpolicy);

		return rc;
		}

Admin message