On Fri, Feb 14, 2020 at 4:12 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Fri, Feb 14, 2020 at 1:35 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Wed, Feb 12, 2020 at 6:23 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
...
> > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > > index 981797bfc547..d8b72718e793 100644
> > > --- a/security/selinux/ss/policydb.c
> > > +++ b/security/selinux/ss/policydb.c
> > > @@ -1882,64 +1884,91 @@ out:
> > >
> > > static int filename_trans_read_one(struct policydb *p, void *fp)
> > > {
> > > - struct filename_trans *ft;
> > > - struct filename_trans_datum *otype = NULL;
> > > + struct filename_trans_key key, *ft = NULL;
> > > + struct filename_trans_datum *datum, *last, *newdatum = NULL;
> > > + uintptr_t stype, otype;
> > > char *name = NULL;
> > > u32 len;
> > > __le32 buf[4];
> > > int rc;
> > > -
> > > - ft = kzalloc(sizeof(*ft), GFP_KERNEL);
> > > - if (!ft)
> > > - return -ENOMEM;
> > > -
> > > - rc = -ENOMEM;
> > > - otype = kmalloc(sizeof(*otype), GFP_KERNEL);
> > > - if (!otype)
> > > - goto out;
> > > + bool already_there;
> > >
> > > /* length of the path component string */
> > > rc = next_entry(buf, fp, sizeof(u32));
> > > if (rc)
> > > - goto out;
> > > + return rc;
> > > len = le32_to_cpu(buf[0]);
> > >
> > > /* path component string */
> > > rc = str_read(&name, GFP_KERNEL, fp, len);
> > > if (rc)
> > > - goto out;
> > > -
> > > - ft->name = name;
> > > + return rc;
> > >
> > > rc = next_entry(buf, fp, sizeof(u32) * 4);
> > > if (rc)
> > > goto out;
> > >
> > > - ft->stype = le32_to_cpu(buf[0]);
> > > - ft->ttype = le32_to_cpu(buf[1]);
> > > - ft->tclass = le32_to_cpu(buf[2]);
> > > + stype = le32_to_cpu(buf[0]);
> > > + key.ttype = le32_to_cpu(buf[1]);
> > > + key.tclass = le32_to_cpu(buf[2]);
> > > + key.name = name;
> >
> > We don't really need the "name" variable anymore do we, we can just
> > use "key.name" instead, right?
>
> It is possible, but there is a slight obstacle in that "key.name" is
> "const char *" and "name" is "char *" (and str_read() expects a
> reference to "char *"). We could change the type in the
> filename_trans_key struct, but is it really worth it?
>
> I like to have a separate variable for the name, since it is easier to
> spot that it is something we allocate and need to take care not to
> leak it. It is easier to forget that there is that one member of key
> that you need to free in the error path.
>
> I'll be foolish enough to hope that I convinced you so I'll wait for
> your reaction for now, but I'm willing to do the change if you still
> want it :)
Heh ;)
Okay, I'm convinced, let's keep "name".
--
paul moore
www.paul-moore.com
