On Tue, Aug 25, 2020 at 10:12 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> 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.
Patches are welcome :)
--
paul moore
www.paul-moore.com
