Brainy: Kernel Memory Leak in PF

2015-02-19 Thread Maxime Villard 
Hi,
I put here a bug among others:

-- sys/net/pf_ioctl.c --

1027qs = pool_get(pf_queue_pl, PR_WAITOK|PR_LIMITFAIL|PR_ZERO);
if (qs == NULL) {
error = ENOMEM;
break;
}
bcopy(q-queue, qs, sizeof(*qs));
qs-qid = pf_qname2qid(qs-qname, 1);
if (qs-parent[0]  (qs-parent_qid =
pf_qname2qid(qs-parent, 0)) == 0)
return (ESRCH);



'qs' is leaked.

Found by The Brainy Code Scanner.

Maxime

Re: Brainy: Kernel Memory Leak in PF

2015-02-19 Thread Todd C. Miller 
On Thu, 19 Feb 2015 18:31:01 -0500, Ted Unangst wrote:

 Thanks. I see two cases here where we need to pool_put the qs. Also need to
 change return to break so that we release the rwlock.

Not to mention the splx().  OK millert@

 - todd

Re: Brainy: Kernel Memory Leak in PF

2015-02-19 Thread Alexander Bluhm 
On Thu, Feb 19, 2015 at 06:31:01PM -0500, Ted Unangst wrote:
 Maxime Villard wrote:
  Hi,
  I put here a bug among others:
 
 Thanks. I see two cases here where we need to pool_put the qs. Also need to
 change return to break so that we release the rwlock.
 
 
 Index: pf_ioctl.c
 ===
 RCS file: /cvs/src/sys/net/pf_ioctl.c,v
 retrieving revision 1.282
 diff -u -p -r1.282 pf_ioctl.c
 --- pf_ioctl.c10 Feb 2015 06:45:55 -  1.282
 +++ pf_ioctl.c19 Feb 2015 23:28:33 -
 @@ -1032,10 +1032,14 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
   bcopy(q-queue, qs, sizeof(*qs));
   qs-qid = pf_qname2qid(qs-qname, 1);
   if (qs-parent[0]  (qs-parent_qid =
 - pf_qname2qid(qs-parent, 0)) == 0)
 - return (ESRCH);
 + pf_qname2qid(qs-parent, 0)) == 0) {
 + pool_put(pf_queue_pl, qs);
 + error = ESRCH;
 + break;
 + }
   qs-kif = pfi_kif_get(qs-ifname);
   if (!qs-kif-pfik_ifp) {

pfi_kif_get() may return NULL.  And if it mallocs the kif, we would
leak it here.  But it does not set pfik_ifp.  I think this check
should be if (qs-kif == NULL).

 + pool_put(pf_queue_pl, qs);
   error = ESRCH;
   break;
   }

Otherwise OK bluhm@

Re: Brainy: Kernel Memory Leak in PF

2015-02-19 Thread Ted Unangst 
Maxime Villard wrote:
 Hi,
 I put here a bug among others:

Thanks. I see two cases here where we need to pool_put the qs. Also need to
change return to break so that we release the rwlock.


Index: pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.282
diff -u -p -r1.282 pf_ioctl.c
--- pf_ioctl.c  10 Feb 2015 06:45:55 -  1.282
+++ pf_ioctl.c  19 Feb 2015 23:28:33 -
@@ -1032,10 +1032,14 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
bcopy(q-queue, qs, sizeof(*qs));
qs-qid = pf_qname2qid(qs-qname, 1);
if (qs-parent[0]  (qs-parent_qid =
-   pf_qname2qid(qs-parent, 0)) == 0)
-   return (ESRCH);
+   pf_qname2qid(qs-parent, 0)) == 0) {
+   pool_put(pf_queue_pl, qs);
+   error = ESRCH;
+   break;
+   }
qs-kif = pfi_kif_get(qs-ifname);
if (!qs-kif-pfik_ifp) {
+   pool_put(pf_queue_pl, qs);
error = ESRCH;
break;
}

Re: Brainy: Kernel Memory Leak in PF

2015-02-19 Thread Ted Unangst 
Alexander Bluhm wrote:
 On Thu, Feb 19, 2015 at 06:31:01PM -0500, Ted Unangst wrote:
  Maxime Villard wrote:
   Hi,
   I put here a bug among others:
  
  Thanks. I see two cases here where we need to pool_put the qs. Also need to
  change return to break so that we release the rwlock.
  
  
  Index: pf_ioctl.c
  ===
  RCS file: /cvs/src/sys/net/pf_ioctl.c,v
  retrieving revision 1.282
  diff -u -p -r1.282 pf_ioctl.c
  --- pf_ioctl.c  10 Feb 2015 06:45:55 -  1.282
  +++ pf_ioctl.c  19 Feb 2015 23:28:33 -
  @@ -1032,10 +1032,14 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
  bcopy(q-queue, qs, sizeof(*qs));
  qs-qid = pf_qname2qid(qs-qname, 1);
  if (qs-parent[0]  (qs-parent_qid =
  -   pf_qname2qid(qs-parent, 0)) == 0)
  -   return (ESRCH);
  +   pf_qname2qid(qs-parent, 0)) == 0) {
  +   pool_put(pf_queue_pl, qs);
  +   error = ESRCH;
  +   break;
  +   }
  qs-kif = pfi_kif_get(qs-ifname);
  if (!qs-kif-pfik_ifp) {
 
 pfi_kif_get() may return NULL.  And if it mallocs the kif, we would
 leak it here.  But it does not set pfik_ifp.  I think this check
 should be if (qs-kif == NULL).

Yes. That is consistent with other callers.


Index: pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.282
diff -u -p -r1.282 pf_ioctl.c
--- pf_ioctl.c  10 Feb 2015 06:45:55 -  1.282
+++ pf_ioctl.c  20 Feb 2015 01:00:29 -
@@ -1032,10 +1032,14 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
bcopy(q-queue, qs, sizeof(*qs));
qs-qid = pf_qname2qid(qs-qname, 1);
if (qs-parent[0]  (qs-parent_qid =
-   pf_qname2qid(qs-parent, 0)) == 0)
-   return (ESRCH);
+   pf_qname2qid(qs-parent, 0)) == 0) {
+   pool_put(pf_queue_pl, qs);
+   error = ESRCH;
+   break;
+   }
qs-kif = pfi_kif_get(qs-ifname);
-   if (!qs-kif-pfik_ifp) {
+   if (qs-kif == NULL) {
+   pool_put(pf_queue_pl, qs);
error = ESRCH;
break;
}

Re: Brainy: Kernel Memory Leak in PF

2015-02-19 Thread Alexander Bluhm 
On Thu, Feb 19, 2015 at 08:01:01PM -0500, Ted Unangst wrote:
 Yes. That is consistent with other callers.

OK bluhm@

 Index: pf_ioctl.c
 ===
 RCS file: /cvs/src/sys/net/pf_ioctl.c,v
 retrieving revision 1.282
 diff -u -p -r1.282 pf_ioctl.c
 --- pf_ioctl.c10 Feb 2015 06:45:55 -  1.282
 +++ pf_ioctl.c20 Feb 2015 01:00:29 -
 @@ -1032,10 +1032,14 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
   bcopy(q-queue, qs, sizeof(*qs));
   qs-qid = pf_qname2qid(qs-qname, 1);
   if (qs-parent[0]  (qs-parent_qid =
 - pf_qname2qid(qs-parent, 0)) == 0)
 - return (ESRCH);
 + pf_qname2qid(qs-parent, 0)) == 0) {
 + pool_put(pf_queue_pl, qs);
 + error = ESRCH;
 + break;
 + }
   qs-kif = pfi_kif_get(qs-ifname);
 - if (!qs-kif-pfik_ifp) {
 + if (qs-kif == NULL) {
 + pool_put(pf_queue_pl, qs);
   error = ESRCH;
   break;
   }