freebsd security - Oct 2006 - [patch] Memory leak from namei_zone in an error path in nfsrv_rename

Description:
------------
Memory leak in nfsrv_rename: In nfsrv_rename, every time a VOP_RENAME 
operation fails FreeBSD leaks 2 items from the namei_zone which is equal 
to 2K of kernel memory.

Filing this as a security issue because a FreeBSD NFS server (versions 
4.6.2 to 6.1) can be compromised by exhausting kernel memory if a user 
touches this error path many times.

I have tried a simple test case against Freebsd nfs server versions 
4.6.2, 5.3 and 6.1.

How to reproduce:
----------------

 From an NFS client running the following cmds against a 4.6.2 FreeBSD 
NFS server mount will cause the memory leak:

$ mkdir a/b
$ while (true) do
 > mv -f a a/b/
 > done

Again running the following cmds against 5.3 or 6.1 FreeBSD NFS server 
will cause the leak:

$ mkdir -p a/b
$ cd a
$ whie (true) do
 > mv . ../a/b/
 > done

There are many other ways to reproduce it, but these are trivial test 
cases I could come up with.

Patch to fix the problem :
________________________


--- nfs_serv.c  2005-11-25 06:32:38.000000000 -0800
+++ /tmp/nfs_serv.c     2006-09-22 14:41:39.000000000 -0700
@@ -2514,26 +2514,26 @@
              /*
               * The VOP_RENAME function releases all vnode references &
               * locks prior to returning so we need to clear the pointers
               * to bypass cleanup code later on.
               */
              error = VOP_RENAME(fromnd.ni_dvp, fromnd.ni_vp, 
&fromnd.ni_cnd,
                                 tond.ni_dvp, tond.ni_vp, &tond.ni_cnd);
              fromnd.ni_dvp = NULL;
              fromnd.ni_vp = NULL;
              tond.ni_dvp = NULL;
              tond.ni_vp = NULL;
              if (error) {
-                       fromnd.ni_cnd.cn_flags &= ~HASBUF;
-                       tond.ni_cnd.cn_flags &= ~HASBUF;
+                       NDFREE(&fromnd, NDF_ONLY_PNBUF);
+                       NDFREE(&tond, NDF_ONLY_PNBUF);
              }
      } else {
              if (error == -1)
                      error = 0;
      }
      /* fall through */


I will be happy to answer any questions wrt this. Please provide me some 
feedback on this fix.

Thanks,
Padma Bhooma

> --- nfs_serv.c  2005-11-25 06:32:38.000000000 -0800
> +++ /tmp/nfs_serv.c     2006-09-22 14:41:39.000000000 -0700
> @@ -2514,26 +2514,26 @@
>             /*
>              * The VOP_RENAME function releases all vnode references &
>              * locks prior to returning so we need to clear the pointers
>              * to bypass cleanup code later on.
>              */
>             error = VOP_RENAME(fromnd.ni_dvp, fromnd.ni_vp,
&fromnd.ni_cnd,
>                                tond.ni_dvp, tond.ni_vp, &tond.ni_cnd);
>             fromnd.ni_dvp = NULL;
>             fromnd.ni_vp = NULL;
>             tond.ni_dvp = NULL;
>             tond.ni_vp = NULL;
>             if (error) {
> -                       fromnd.ni_cnd.cn_flags &= ~HASBUF;
> -                       tond.ni_cnd.cn_flags &= ~HASBUF;
> +                       NDFREE(&fromnd, NDF_ONLY_PNBUF);
> +                       NDFREE(&tond, NDF_ONLY_PNBUF);
>             }
>     } else {
>             if (error == -1)
>                     error = 0;
>     }
>     /* fall through */
>
>
> I will be happy to answer any questions wrt this. Please provide me some 
> feedback on this fix.
Seems about right, but why does it clear HASBUF at all?  Rev.1.79 added
a lot of similar clearings of HASBUF, but rev.1.91 converted all instances
of HASBUF except the above 2 above and 1 in a comment into NDFREE().  I
think associated changes also moved the VOP_ABORTUP() calls into the vfs
layer and out of the HASBUF conditionals.  It looks like the leak was
in rev.1.90 and rev.1.91 tried too hard not to change the logic by leaving
the 2 buggy HASBUF clearings untouched.  The comment about HASBUF is now
bogus -- _we_ now mostly don't use HASBUF to track the clearing of the name
buffer -- now namei iinternals do that and we only (?) use it to implement
the leak :-).

Bruce