Hello Stephen Smalley,
The patch 461698026ffa: "selinux: encapsulate policy state, refactor
policy load" from Aug 7, 2020, leads to the following static checker
warning:
security/selinux/ss/services.c:2288 security_load_policy()
error: we previously assumed 'newpolicy->sidtab' could be null (see line 2227)
security/selinux/ss/services.c
2221
2222 newpolicy = kzalloc(sizeof(*newpolicy), GFP_KERNEL);
2223 if (!newpolicy)
2224 return -ENOMEM;
2225
2226 newpolicy->sidtab = kzalloc(sizeof(*newpolicy->sidtab), GFP_KERNEL);
2227 if (!newpolicy->sidtab)
2228 goto err;
2229
2230 rc = policydb_read(&newpolicy->policydb, fp);
2231 if (rc)
2232 goto err;
2233
2234 newpolicy->policydb.len = len;
2235 rc = selinux_set_mapping(&newpolicy->policydb, secclass_map,
2236 &newpolicy->map);
2237 if (rc)
2238 goto err;
2239
2240 rc = policydb_load_isids(&newpolicy->policydb, newpolicy->sidtab);
2241 if (rc) {
2242 pr_err("SELinux: unable to load the initial SIDs\n");
2243 goto err;
2244 }
2245
2246
2247 if (!selinux_initialized(state)) {
2248 /* First policy load, so no need to preserve state from old policy */
2249 *newpolicyp = newpolicy;
2250 return 0;
2251 }
2252
2253 /* Preserve active boolean values from the old policy */
2254 rc = security_preserve_bools(state, &newpolicy->policydb);
2255 if (rc) {
2256 pr_err("SELinux: unable to preserve booleans\n");
2257 goto err;
2258 }
2259
2260 /*
2261 * Convert the internal representations of contexts
2262 * in the new SID table.
2263 *
2264 * NOTE: We do not need to take the policy read-lock
2265 * around the code below because other policy-modifying
2266 * operations are already excluded by selinuxfs via
2267 * fsi->mutex.
2268 */
2269 args.state = state;
2270 args.oldp = &state->ss->policy->policydb;
2271 args.newp = &newpolicy->policydb;
2272
2273 convert_params.func = convert_context;
2274 convert_params.args = &args;
2275 convert_params.target = newpolicy->sidtab;
2276
2277 rc = sidtab_convert(state->ss->policy->sidtab, &convert_params);
2278 if (rc) {
2279 pr_err("SELinux: unable to convert the internal"
2280 " representation of contexts in the new SID"
2281 " table\n");
2282 goto err;
2283 }
2284
2285 *newpolicyp = newpolicy;
2286 return 0;
2287 err:
2288 selinux_policy_free(newpolicy);
This style of "one function frees everything" error handling is the
most bug prone style of error handling. Typical bugs are:
1) Free uninitalized memory.
2) Decrement reference counts which weren't incremented.
3) Rely on things to be allocated like this example where
"newpolicy->sidtab" is NULL.
4) Double free things because other code is written in the style where
each function cleans up its own allocations. For example, in this
case policydb_read() calls policydb_destroy(p); and
selinux_policy_free() calls it as well so it is a double free.
There are some other double frees on this path as well.
5) Leaks because the code is hard/impossible to audit.
The best way to write error handling is to use "Free the most recent
allocation" style. Use good label names, like "err_free_foo: kfree(foo);"
which say what the goto frees. That way we never free uninitialized
memory. It's easy to audit because we just have to mentally keep
track of the most recent allocation and we check that "goto free_foo;"
matches that foo is the recentest allocation.
2289 return rc;
2290 }
regards,
dan carpenter
