| Message ID | 1458601213-5835-1-git-send-email-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
Quoting Colin King (colin.king@canonical.com): > From: Colin Ian King <colin.king@canonical.com> > > node_ptr is not being free'd if the list allocation fails, fix > this by kfree'ing it before exiting on the error path. > > Signed-off-by: Colin Ian King <colin.king@canonical.com> Hi, I'm not very familiar with this code any more, but are you sure this is needed and doesn't cause a new bug? It *looks* like the avtab_insert_nonunique() actually inserts the node_ptr into the policydb, and the policydb is the one that should eventually free it. > --- > security/selinux/ss/conditional.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c > index 456e1a9..5d010ef 100644 > --- a/security/selinux/ss/conditional.c > +++ b/security/selinux/ss/conditional.c > @@ -332,6 +332,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum > list = kzalloc(sizeof(struct cond_av_list), GFP_KERNEL); > if (!list) { > rc = -ENOMEM; > + kfree(node_ptr); > goto err; > } > > -- > 2.7.3
On Tue, Mar 22, 2016 at 4:28 PM, Serge E. Hallyn <serge@hallyn.com> wrote: > Quoting Colin King (colin.king@canonical.com): >> From: Colin Ian King <colin.king@canonical.com> >> >> node_ptr is not being free'd if the list allocation fails, fix >> this by kfree'ing it before exiting on the error path. >> >> Signed-off-by: Colin Ian King <colin.king@canonical.com> > > Hi, > > I'm not very familiar with this code any more, but are you sure > this is needed and doesn't cause a new bug? It *looks* like > the avtab_insert_nonunique() actually inserts the node_ptr > into the policydb, and the policydb is the one that should > eventually free it. Exactly. cond_insertf() calls avtab_insert_nonunique() which calls avtab_insert_node() which adds the node to the avtab. The avtab will get cleaned up later by the error handling code in the cond_insertf() call chain.
On 22/03/16 21:35, Paul Moore wrote: > On Tue, Mar 22, 2016 at 4:28 PM, Serge E. Hallyn <serge@hallyn.com> wrote: >> Quoting Colin King (colin.king@canonical.com): >>> From: Colin Ian King <colin.king@canonical.com> >>> >>> node_ptr is not being free'd if the list allocation fails, fix >>> this by kfree'ing it before exiting on the error path. >>> >>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> >> Hi, >> >> I'm not very familiar with this code any more, but are you sure >> this is needed and doesn't cause a new bug? It *looks* like >> the avtab_insert_nonunique() actually inserts the node_ptr >> into the policydb, and the policydb is the one that should >> eventually free it. > > Exactly. cond_insertf() calls avtab_insert_nonunique() which calls > avtab_insert_node() which adds the node to the avtab. The avtab will > get cleaned up later by the error handling code in the cond_insertf() > call chain. > My bad, apologies.
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index 456e1a9..5d010ef 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -332,6 +332,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum list = kzalloc(sizeof(struct cond_av_list), GFP_KERNEL); if (!list) { rc = -ENOMEM; + kfree(node_ptr); goto err; }