NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
kern/41914: ipf/ippool rule maintenace bugs: memory leak, ref-counter bug, request processing bug
>Number: 41914
>Category: kern
>Synopsis: ipf/ippool rule maintenace bugs: memory leak, ref-counter bug,
>request processing bug
>Confidential: no
>Severity: serious
>Priority: high
>Responsible: kern-bug-people
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Fri Aug 21 07:35:00 +0000 2009
>Originator: Wolfgang Stukenbrock
>Release: NetBSD 4.0
>Organization:
Dr. Nagler & Company GmbH
>Environment:
System: NetBSD s012 4.0 NetBSD 4.0 (NSW-S012) #9: Fri Mar 13 12:31:52 CET 2009
wgstuken@s012:/usr/src/sys/arch/amd64/compile/NSW-S012 amd64
Architecture: x86_64
Machine: amd64
>Description:
We are planning to use the ippool stuff in our setup here. During our
stability tests, we got problems with pools that
cannot be removed anymore from the system configuration.
The analyse of the problem leads to several problems (bugs) in
/usr/src/sys/dist/ipf/netinet/fil.c.
We located 3 serious problems there: fr_type is not used corretly at
several places, a memory leak and missing pool-counter maintenace
in case of error processing.
NetBSD 4.0 has an older version as the current kernel, but the problems
reported and fixed here are still present in the
current-version (CVS-id fil.c 1.45).
First problem:
The fr_type field may be or'd with FR_T_BUILTIN in order to distinguish
between rules from user level and kernel calls.
But this flag is not "removed" prior decision at all places.
At one location fr_flags is checked where fr_type should be used.
Second problen:
When additional data is copied into the kernel in the IOCTL-calls
(fr_data/fr_dsize), it is not freed on error in all cases.
Third problem:
When accessing pools in an IPF-rule, the reference counter of the pools
will be incremented, but not decremented on error or after
successfull processing of a remove request. This keeps the counter away
from ever reaching 0 again - and the pool cannot be removed
from the kernel anymore ....
>How-To-Repeat:
setup some pools and try to insert a duplicate rule - you will fail to
remove teh pool after that.
>Fix:
The following patch will fix all three problems in
/usr/src/sys/dist/ipf/netinet/fil.c.
It is based on the version in 4.0. The same problems is still in there
in the current kernel, even if ipf is upgraded several times
since 4.0. I've tried to patch the current version with this patch too
and it looks good.
The failing hunk 17 is only a comment-correction (missing closing
braket), that has been already corrected in the current version.
I haven't tried to patch the version in 5.x - sorry - but I think it
will succedd there too, because the current version can
be patched.
--- fil.c 2009/06/29 15:11:26 1.3
+++ fil.c 2009/08/20 15:03:42
@@ -3755,7 +3755,7 @@
fr->fr_ifas[i] = fr_resolvenic(fr->fr_ifnames[i], v);
}
- if (fr->fr_type == FR_T_IPF) {
+ if ((fr->fr_type & ~FR_T_BUILTIN) == FR_T_IPF) {
if (fr->fr_satype != FRI_NORMAL &&
fr->fr_satype != FRI_LOOKUP) {
(void)fr_ifpaddr(v, fr->fr_satype,
@@ -3789,14 +3789,14 @@
}
#ifdef IPFILTER_LOOKUP
- if (fr->fr_type == FR_T_IPF && fr->fr_satype == FRI_LOOKUP &&
+ if ((fr->fr_type & ~FR_T_BUILTIN) == FR_T_IPF && fr->fr_satype
== FRI_LOOKUP &&
fr->fr_srcptr == NULL) {
fr->fr_srcptr = fr_resolvelookup(fr->fr_srctype,
fr->fr_srcsubtype,
&fr->fr_slookup,
&fr->fr_srcfunc);
}
- if (fr->fr_type == FR_T_IPF && fr->fr_datype == FRI_LOOKUP &&
+ if ((fr->fr_type & ~FR_T_BUILTIN) == FR_T_IPF && fr->fr_datype
== FRI_LOOKUP &&
fr->fr_dstptr == NULL) {
fr->fr_dstptr = fr_resolvelookup(fr->fr_dsttype,
fr->fr_dstsubtype,
@@ -4187,7 +4187,7 @@
void *data;
{
frentry_t frd, *fp, *f, **fprev, **ftail;
- int error = 0, in, v;
+ int error = 0, in, v, need_free = 0;
void *ptr, *uptr;
u_int *p, *pp;
frgroup_t *fg;
@@ -4199,7 +4199,7 @@
error = fr_inobj(data, fp, IPFOBJ_FRENTRY);
if (error)
return EFAULT;
- if ((fp->fr_flags & FR_T_BUILTIN) != 0)
+ if ((fp->fr_type & FR_T_BUILTIN) != 0)
return EINVAL;
fp->fr_ref = 0;
fp->fr_flags |= FR_COPIED;
@@ -4309,7 +4309,6 @@
error = EFAULT;
} else {
ptr = uptr;
- error = 0;
}
if (error != 0) {
KFREES(ptr, fp->fr_dsize);
@@ -4319,6 +4318,8 @@
} else
fp->fr_data = NULL;
+/* remark: after this point we have allocated some additional data that need a
free before exit ... */
+
/*
* Perform per-rule type sanity checks of their members.
*/
@@ -4327,18 +4328,14 @@
#if defined(IPFILTER_BPF)
case FR_T_BPFOPC :
if (fp->fr_dsize == 0)
- return EINVAL;
- if (!bpf_validate(ptr, fp->fr_dsize/sizeof(struct bpf_insn))) {
- if (makecopy && fp->fr_data != NULL) {
- KFREES(fp->fr_data, fp->fr_dsize);
- }
- return EINVAL;
- }
+ return EINVAL; /* special case - no data allocated - no
need to check to free it ... */
+ if (!bpf_validate(ptr, fp->fr_dsize/sizeof(struct bpf_insn)))
+ goto exit_INVAL_free;
break;
#endif
case FR_T_IPF :
if (fp->fr_dsize != sizeof(fripf_t))
- return EINVAL;
+ goto exit_INVAL_free;
/*
* Allowing a rule with both "keep state" and "with oow" is
@@ -4346,7 +4343,7 @@
* fail with the out of window (oow) flag set.
*/
if ((fp->fr_flags & FR_KEEPSTATE) && (fp->fr_flx & FI_OOW))
- return EINVAL;
+ goto exit_INVAL_free;
switch (fp->fr_satype)
{
@@ -4355,12 +4352,8 @@
case FRI_NETWORK :
case FRI_NETMASKED :
case FRI_PEERADDR :
- if (fp->fr_sifpidx < 0 || fp->fr_sifpidx > 3) {
- if (makecopy && fp->fr_data != NULL) {
- KFREES(fp->fr_data, fp->fr_dsize);
- }
- return EINVAL;
- }
+ if (fp->fr_sifpidx < 0 || fp->fr_sifpidx > 3)
+ goto exit_INVAL_free;
break;
#ifdef IPFILTER_LOOKUP
case FRI_LOOKUP :
@@ -4368,8 +4361,10 @@
fp->fr_srcsubtype,
&fp->fr_slookup,
&fp->fr_srcfunc);
- if (fp->fr_srcptr == NULL)
- return ESRCH;
+ if (fp->fr_srcptr == NULL) {
+ error = ESRCH;
+ goto exit_free;
+ }
break;
#endif
default :
@@ -4383,12 +4378,8 @@
case FRI_NETWORK :
case FRI_NETMASKED :
case FRI_PEERADDR :
- if (fp->fr_difpidx < 0 || fp->fr_difpidx > 3) {
- if (makecopy && fp->fr_data != NULL) {
- KFREES(fp->fr_data, fp->fr_dsize);
- }
- return EINVAL;
- }
+ if (fp->fr_difpidx < 0 || fp->fr_difpidx > 3)
+ goto exit_INVAL_free;
break;
#ifdef IPFILTER_LOOKUP
case FRI_LOOKUP :
@@ -4396,8 +4387,12 @@
fp->fr_dstsubtype,
&fp->fr_dlookup,
&fp->fr_dstfunc);
- if (fp->fr_dstptr == NULL)
- return ESRCH;
+ if (fp->fr_dstptr == NULL) {
+ if (fp->fr_satype == FRI_LOOKUP)
+ ip_lookup_deref(fp->fr_srctype,
fp->fr_srcptr);
+ error = ESRCH;
+ goto exit_free;
+ }
break;
#endif
default :
@@ -4405,18 +4400,23 @@
}
break;
case FR_T_NONE :
- break;
case FR_T_CALLFUNC :
- break;
case FR_T_COMPIPF :
break;
default :
+ exit_INVAL_free:
+ error = EINVAL;
+#ifdef IPFILTER_LOOKUP
+ exit_free:
+#endif
if (makecopy && fp->fr_data != NULL) {
KFREES(fp->fr_data, fp->fr_dsize);
}
- return EINVAL;
+ return error;
}
+/* remark: at this point pool reference counter may have been incremented ->
leave via done: label required ... */
+
/*
* Lookup all the interface names that are part of the rule.
*/
@@ -4504,11 +4504,8 @@
}
}
- if ((ptr != NULL) && (makecopy != 0)) {
- KFREES(ptr, fp->fr_dsize);
- }
- RWLOCK_EXIT(&ipf_mutex);
- return error;
+ need_free = 1;
+ goto done;
}
if (!f) {
@@ -4529,7 +4526,6 @@
}
f = NULL;
ptr = NULL;
- error = 0;
} else if (req == (ioctlcmd_t)SIOCINAFR ||
req == (ioctlcmd_t)SIOCINIFR) {
while ((f = *fprev) != NULL) {
@@ -4549,7 +4545,6 @@
}
f = NULL;
ptr = NULL;
- error = 0;
}
}
@@ -4571,7 +4566,7 @@
/*
* Return EBUSY if the rule is being reference by
- * something else (eg state information.
+ * something else (eg state information).
*/
if (f->fr_ref > 1) {
error = EBUSY;
@@ -4582,7 +4577,9 @@
(f->fr_isc != (struct ipscan *)-1))
ipsc_detachfr(f);
#endif
+ need_free = 1;
if (unit == IPL_LOGAUTH) {
+ ASSERT(req == (ioctlcmd_t)SIOCRMAFR); /*
SIOCRMIFR will insert in fr_preauthcmd() ... */
error = fr_preauthcmd(req, f, ftail);
goto done;
}
@@ -4641,8 +4638,22 @@
}
done:
RWLOCK_EXIT(&ipf_mutex);
- if ((ptr != NULL) && (error != 0) && (makecopy != 0)) {
- KFREES(ptr, fp->fr_dsize);
+ if (error != 0 || need_free != 0) {
+ if ((ptr != NULL) && (makecopy != 0)) {
+ KFREES(ptr, fp->fr_dsize);
+ }
+#ifdef IPFILTER_LOOKUP
+ if ((fp->fr_type & ~FR_T_BUILTIN) == FR_T_IPF) {
+ if (fp->fr_satype == FRI_LOOKUP) {
+ ASSERT(fp->fr_srcptr != NULL);
+ ip_lookup_deref(fp->fr_srctype, fp->fr_srcptr);
+ }
+ if (fp->fr_datype == FRI_LOOKUP) {
+ ASSERT(fp->fr_dstptr != NULL);
+ ip_lookup_deref(fp->fr_dsttype, fp->fr_dstptr);
+ }
+ }
+#endif
}
return (error);
}
@@ -4812,9 +4823,9 @@
MUTEX_DESTROY(&fr->fr_lock);
#ifdef IPFILTER_LOOKUP
- if (fr->fr_type == FR_T_IPF && fr->fr_satype == FRI_LOOKUP)
+ if ((fr->fr_type & ~FR_T_BUILTIN) == FR_T_IPF && fr->fr_satype
== FRI_LOOKUP)
ip_lookup_deref(fr->fr_srctype, fr->fr_srcptr);
- if (fr->fr_type == FR_T_IPF && fr->fr_datype == FRI_LOOKUP)
+ if ((fr->fr_type & ~FR_T_BUILTIN) == FR_T_IPF && fr->fr_datype
== FRI_LOOKUP)
ip_lookup_deref(fr->fr_dsttype, fr->fr_dstptr);
#endif
>Unformatted:
Home |
Main Index |
Thread Index |
Old Index