On Tue, Aug 25, 2020 at 09:38:22AM -0400, Paul Moore wrote:
> 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.
There are other bugs on this path as well. For example in context_cpy():
150 static inline int context_cpy(struct context *dst, struct context *src)
151 {
152 int rc;
153
154 dst->user = src->user;
155 dst->role = src->role;
156 dst->type = src->type;
157 if (src->str) {
158 dst->str = kstrdup(src->str, GFP_ATOMIC);
159 if (!dst->str)
160 return -ENOMEM;
161 dst->len = src->len;
162 } else {
163 dst->str = NULL;
164 dst->len = 0;
165 }
166 rc = mls_context_cpy(dst, src);
167 if (rc) {
168 kfree(dst->str);
^^^^^^^^
This is the right place to free it. That's what other coders generally
expect and what static analysis tools expect. Otherwise it's not clear
if it's the caller which should free it or the caller's caller. In this
code we free it in all three places.
169 return rc;
170 }
171 return 0;
172 }
regards,
dan carpenter
