On Tue, Aug 25, 2020 at 8:51 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > 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; ... > 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 ... In this particular case I think the use of selinux_poicy_free() is okay, we should just make selinux_policy_free() a bit more robust, e.g. checking that ->sidtab is non-NULL before calling sidtab_destroy(). While we are at it, it probably wouldn't hurt to also check ->policydb. -- paul moore www.paul-moore.com
