From: Chris Wright <chrisw@osdl.org>
To: David Yu Chen <dychen@stanford.edu>
Cc: linux-kernel@vger.kernel.org, mc@cs.stanford.edu, sds@epoch.ncsc.mil
Subject: Re: [CHECKER] 32 Memory Leaks on Error Paths
Date: Fri, 19 Sep 2003 16:04:59 -0700 [thread overview]
Message-ID: <20030919160459.K27079@osdlab.pdx.osdl.net> (raw)
In-Reply-To: <200309160435.h8G4ZkQM009953@elaine4.Stanford.EDU>; from dychen@stanford.edu on Mon, Sep 15, 2003 at 09:35:46PM -0700
* David Yu Chen (dychen@stanford.edu) wrote:
> [FILE: 2.6.0-test5/security/selinux/ss/policydb.c]
> START -->
> 1334: newgenfs = kmalloc(sizeof(*newgenfs), GFP_KERNEL);
> 1335: if (!newgenfs) {
> 1336: rc = -ENOMEM;
> 1337: goto bad;
> 1338: }
> 1339: memset(newgenfs, 0, sizeof(*newgenfs));
> ... DELETED 5 lines ...
You missed one ;-)
1344 buf = next_entry(fp, len);
1345 if (!buf)
1346 goto bad;
> GOTO -->
> 1350: goto bad;
Yes, this is a bug.
> 1357: printk(KERN_ERR "security: dup genfs "
> 1358: "fstype %s\n", newgenfs->fstype);
> GOTO -->
> 1359: goto bad;
Yes, this is a bug. I'm not sure if CHECKER caught that it leaks two
allocations here. Both newgenfs and newgenfs->fstype.
> [FILE: 2.6.0-test5/security/selinux/ss/policydb.c]
> START -->
> 1374: newc = kmalloc(sizeof(*newc), GFP_KERNEL);
> 1375: if (!newc) {
> 1376: rc = -ENOMEM;
> 1377: goto bad;
> 1378: }
> 1379: memset(newc, 0, sizeof(*newc));
You missed some more. Not sure if this is meant to be comprensive or not...
1380 buf = next_entry(fp, sizeof(u32));
1381 if (!buf)
1382 goto bad;
*HERE*
1383 len = le32_to_cpu(buf[0]);
1384 buf = next_entry(fp, len);
1385 if (!buf)
1386 goto bad;
*HERE*
1387 newc->u.name = kmalloc(len + 1,GFP_KERNEL);
1388 if (!newc->u.name) {
1389 rc = -ENOMEM;
1390 goto bad;
*HERE*
> ... DELETED 14 lines ...
> 1394: buf = next_entry(fp, sizeof(u32));
> 1395: if (!buf)
> 1396: goto bad;
Again, not sure if you meant to capture both leakages here: newc and
newc->u.name.
> 1397: newc->v.sclass = le32_to_cpu(buf[0]);
> 1398: if (context_read_and_validate(&newc->context[0], p, fp))
> GOTO -->
> 1399: goto bad;
And both here.
> 1400: for (l = NULL, c = newgenfs->head; c;
> 1401: l = c, c = c->next) {
> 1402: if (!strcmp(newc->u.name, c->u.name) &&
> 1403: (!c->v.sclass || !newc->v.sclass ||
> 1404: newc->v.sclass == c->v.sclass)) {
> 1405: printk(KERN_ERR "security: dup genfs "
> 1406: "entry (%s,%s)\n",
> 1407: newgenfs->fstype, c->u.name);
> GOTO -->
> 1408: goto bad;
And both here.
Patch below fixes the leaks in a brute force method. Stephen, look ok?
I could see rearranging the code to ensure things are stored in the policydb
earlier so they are all caught by policydb_destroy().
> [FILE: 2.6.0-test5/security/selinux/ss/policydb.c]
> START -->
> 759: c = kmalloc(sizeof(*c), GFP_KERNEL);
> 760: if (!c) {
> 761: rc = -ENOMEM;
> 762: goto bad;
> 763: }
> 764: memset(c, 0, sizeof(*c));
You missed this one:
765 buf = next_entry(fp, sizeof(u32)*2);
766 if (!buf)
767 goto bad;
And this one:
774 if (!e) {
775 rc = -ENOMEM;
776 goto bad;
And this one:
780 if (!buf) {
781 kfree(e);
782 goto bad;
And this one:
790 if (depth < 0) {
791 kfree(e);
792 goto bad;
etc...
> ... DELETED 64 lines ...
> 829: c->expr = e;
> 830: }
> 831: le = e;
> 832: }
> 833: if (depth != 0)
> GOTO -->
Yes, this is a bug. Stephen, the patch below fixes this, and adds a
constraint_destroy() function to help cleanup when the constaint_node
isn't yet attached to the class_datum but there may already be some
constraint_expr's on the constraint_node.
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
===== security/selinux/ss/policydb.c 1.3 vs edited =====
--- 1.3/security/selinux/ss/policydb.c Sun Aug 31 16:14:19 2003
+++ edited/security/selinux/ss/policydb.c Fri Sep 19 15:59:02 2003
@@ -321,17 +321,11 @@
return 0;
}
-static int class_destroy(void *key, void *datum, void *p)
+static void constraint_destroy(struct constraint_node *constraint)
{
- struct class_datum *cladatum;
- struct constraint_node *constraint, *ctemp;
+ struct constraint_node *ctemp;
struct constraint_expr *e, *etmp;
- kfree(key);
- cladatum = datum;
- hashtab_map(cladatum->permissions.table, perm_destroy, 0);
- hashtab_destroy(cladatum->permissions.table);
- constraint = cladatum->constraints;
while (constraint) {
e = constraint->expr;
while (e) {
@@ -344,6 +338,19 @@
constraint = constraint->next;
kfree(ctemp);
}
+}
+
+static int class_destroy(void *key, void *datum, void *p)
+{
+ struct class_datum *cladatum;
+ struct constraint_node *constraint;
+
+ kfree(key);
+ cladatum = datum;
+ hashtab_map(cladatum->permissions.table, perm_destroy, 0);
+ hashtab_destroy(cladatum->permissions.table);
+ constraint = cladatum->constraints;
+ constraint_destroy(constraint);
kfree(cladatum->comkey);
kfree(datum);
return 0;
@@ -763,8 +770,10 @@
}
memset(c, 0, sizeof(*c));
buf = next_entry(fp, sizeof(u32)*2);
- if (!buf)
+ if (!buf) {
+ kfree(c);
goto bad;
+ }
c->permissions = le32_to_cpu(buf[0]);
nexpr = le32_to_cpu(buf[1]);
le = NULL;
@@ -773,11 +782,13 @@
e = kmalloc(sizeof(*e), GFP_KERNEL);
if (!e) {
rc = -ENOMEM;
+ constraint_destroy(c);
goto bad;
}
memset(e, 0, sizeof(*e));
buf = next_entry(fp, sizeof(u32)*3);
if (!buf) {
+ constraint_destroy(c);
kfree(e);
goto bad;
}
@@ -788,6 +799,7 @@
switch (e->expr_type) {
case CEXPR_NOT:
if (depth < 0) {
+ constraint_destroy(c);
kfree(e);
goto bad;
}
@@ -795,6 +807,7 @@
case CEXPR_AND:
case CEXPR_OR:
if (depth < 1) {
+ constraint_destroy(c);
kfree(e);
goto bad;
}
@@ -802,6 +815,7 @@
break;
case CEXPR_ATTR:
if (depth == (CEXPR_MAXDEPTH-1)) {
+ constraint_destroy(c);
kfree(e);
goto bad;
}
@@ -809,16 +823,19 @@
break;
case CEXPR_NAMES:
if (depth == (CEXPR_MAXDEPTH-1)) {
+ constraint_destroy(c);
kfree(e);
goto bad;
}
depth++;
if (ebitmap_read(&e->names, fp)) {
+ constraint_destroy(c);
kfree(e);
goto bad;
}
break;
default:
+ constraint_destroy(c);
kfree(e);
goto bad;
break;
@@ -830,8 +847,10 @@
}
le = e;
}
- if (depth != 0)
+ if (depth != 0) {
+ constraint_destroy(c);
goto bad;
+ }
if (lc) {
lc->next = c;
} else {
@@ -1338,15 +1357,20 @@
}
memset(newgenfs, 0, sizeof(*newgenfs));
buf = next_entry(fp, sizeof(u32));
- if (!buf)
+ if (!buf) {
+ kfree(newgenfs);
goto bad;
+ }
len = le32_to_cpu(buf[0]);
buf = next_entry(fp, len);
- if (!buf)
+ if (!buf) {
+ kfree(newgenfs);
goto bad;
+ }
newgenfs->fstype = kmalloc(len + 1,GFP_KERNEL);
if (!newgenfs->fstype) {
rc = -ENOMEM;
+ kfree(newgenfs);
goto bad;
}
memcpy(newgenfs->fstype, buf, len);
@@ -1356,6 +1380,8 @@
if (strcmp(newgenfs->fstype, genfs->fstype) == 0) {
printk(KERN_ERR "security: dup genfs "
"fstype %s\n", newgenfs->fstype);
+ kfree(newgenfs->fstype);
+ kfree(newgenfs);
goto bad;
}
if (strcmp(newgenfs->fstype, genfs->fstype) < 0)
@@ -1378,25 +1404,36 @@
}
memset(newc, 0, sizeof(*newc));
buf = next_entry(fp, sizeof(u32));
- if (!buf)
+ if (!buf) {
+ kfree(newc);
goto bad;
+ }
len = le32_to_cpu(buf[0]);
buf = next_entry(fp, len);
- if (!buf)
+ if (!buf) {
+ kfree(newc);
goto bad;
+ }
newc->u.name = kmalloc(len + 1,GFP_KERNEL);
if (!newc->u.name) {
rc = -ENOMEM;
+ kfree(newc);
goto bad;
}
memcpy(newc->u.name, buf, len);
newc->u.name[len] = 0;
buf = next_entry(fp, sizeof(u32));
- if (!buf)
+ if (!buf) {
+ kfree(newc->u.name);
+ kfree(newc);
goto bad;
+ }
newc->v.sclass = le32_to_cpu(buf[0]);
- if (context_read_and_validate(&newc->context[0], p, fp))
+ if (context_read_and_validate(&newc->context[0], p, fp)){
+ kfree(newc->u.name);
+ kfree(newc);
goto bad;
+ }
for (l = NULL, c = newgenfs->head; c;
l = c, c = c->next) {
if (!strcmp(newc->u.name, c->u.name) &&
@@ -1405,6 +1442,8 @@
printk(KERN_ERR "security: dup genfs "
"entry (%s,%s)\n",
newgenfs->fstype, c->u.name);
+ kfree(newc->u.name);
+ kfree(newc);
goto bad;
}
len = strlen(newc->u.name);
next prev parent reply other threads:[~2003-09-19 23:06 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2003-09-16 4:35 David Yu Chen
2003-09-16 6:40 ` Neil Brown
2003-09-16 6:55 ` Jörn Engel
2003-09-16 7:21 ` [PATCH] fix memleak in fs/jffs2/scan.c (was: re: [CHECKER] 32 Memory Leaks on Error Paths) Jörn Engel
2003-09-16 7:32 ` [CHECKER] 32 Memory Leaks on Error Paths Jörn Engel
2003-09-16 8:51 ` Jörn Engel
2003-09-16 14:52 ` Timothy Miller
2003-09-16 15:02 ` Wade
2003-09-16 15:04 ` Valdis.Kletnieks
2003-09-16 15:04 ` Nick Piggin
2003-09-16 8:45 ` Wade
2003-09-16 8:56 ` Jörn Engel
2003-09-16 12:10 ` Andries Brouwer
2003-09-16 9:07 ` Jörn Engel
2003-09-20 7:58 ` David S. Miller
2003-09-16 9:48 ` [PATCH] bttv-risc.c (was: Re: [CHECKER] 32 Memory Leaks on Error Paths) Wade
2003-09-16 10:18 ` [PATCH] fix memleak in emu10k1/midi.c " Wade
2003-09-16 12:03 ` [CHECKER] 32 Memory Leaks on Error Paths Andries Brouwer
2003-09-19 23:03 ` Chris Wright
2003-09-19 23:04 ` Chris Wright
2003-09-19 23:04 ` Chris Wright
2003-09-19 23:04 ` Chris Wright [this message]
2003-09-23 13:15 ` Stephen Smalley
2003-09-23 18:02 ` Chris Wright
2003-09-22 22:54 ` Chris Wright
2003-09-22 22:55 ` Chris Wright
2003-09-22 22:55 ` Chris Wright
2003-09-23 20:13 ` Chris Wright
2003-09-23 20:25 ` Greg KH
2003-09-23 21:38 ` Chris Wright
2003-09-23 22:14 ` Chris Wright
2003-09-24 0:17 ` Greg KH
2003-09-23 20:14 ` Chris Wright
2003-09-23 20:14 ` Chris Wright
2003-09-23 20:21 ` Jean Tourrilhes
2003-09-23 20:24 ` Chris Wright
2003-09-23 20:14 ` Chris Wright
2003-09-23 20:15 ` Chris Wright
2003-09-24 7:08 ` David Howells
[not found] <20030923140503.N20572@osdlab.pdx.osdl.net>
2003-09-24 4:13 ` Pete Zaitcev
2003-09-24 12:49 ` Alan Cox
2003-09-24 16:38 ` Pete Zaitcev
2003-09-29 17:48 ` Marcelo Tosatti
2003-09-29 17:54 ` Pete Zaitcev
2003-11-06 0:58 ` Pete Zaitcev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20030919160459.K27079@osdlab.pdx.osdl.net \
--to=chrisw@osdl.org \
--cc=dychen@stanford.edu \
--cc=linux-kernel@vger.kernel.org \
--cc=mc@cs.stanford.edu \
--cc=sds@epoch.ncsc.mil \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®