misc/144874: if_bridge frees mbuf after pfil hooks returns non-zero

[Jake Montogmery]

>Number:         144874
>Category:       misc
>Synopsis:       if_bridge frees mbuf after pfil hooks returns non-zero
>Confidential:   no
>Severity:       critical
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Fri Mar 19 15:30:02 UTC 2010
>Closed-Date:
>Last-Modified:
>Originator:     Jake Montogmery
>Release:        8.0 RELEASE
>Organization:
>Environment:
>Description:
he man pages for ipfil state:

"The filter returns an error (errno) if the packet processing is to stop, or 0 if the processing is to continue.  If the packet processing is to stop, it is the responsibility of the filter to free the packet."

When ip_input() is being used to process the hooks, then this is the case [see ip_input.c rev 199625 line 517.] However, when the ip_bridge diver is being used, then bridge_pfil() [if_bridge.c rev 199625 line 2932] will call m_freem() [at line 3214] on the buffer that was passed to the pfil hook even if the hook returns a non-zero value. This is due to "faulty" logic at lines 3189-3192. As stated in the documentation a non-zero return from the pfil hook should indicate that the filter has freed the mbuf. This results in "double freeing" of the mbuf, which causes various panics down the line.

If the pfil hook sets the mbuf to NULL before returning, then the if_bridge filter behaves properly. The problem is that if a filter is written based on the documentation, and tested on a system that is not running as a bridge, it will behave fine. Once a bridge is introduced the system will panic at apparently "random" times.

The problem exists in at least in 7.1, 7.2 and 8.0, though I suspect it is older than that.


>How-To-Repeat:
Create a simple pfil hook and install it with pfil_add_hook(PFIL_IN). The hook should drop (some) packets by returning a non-zero value. The hook should free the mbuf on dropped packets by calling m_freem(*mp). The filter should _not_ modify the mbuf pointer (mp). Install a if_bridge on the system, and pass traffic through the bridge, such that at least one packet gets dropped by the pfil hook. At some point shortly after that the system will panic. The panic is usually occurs in sbflush_internal(), though there are other ways that the corruption can manifest. 
>Fix:
It would be nice to see the bridge_pfil() code fixed. However, it could be high risk, since there may be some incorrectly written filters out there that rely on this behavior. If the if_bridge code is not fixed, then it would be very helpful to developers to document that the mbuf pointer should be set to NULL if a non-zero value is returned from a pfil hook. This could be added as part of the pfil specification, or at least in the BUGS section of the pfil manpage documentation. 

The code could be fixed by changing [if_bridge.c rev 199625 line 3189]
-  if (*mp == NULL)
-    return (error);
-  if (error != 0)
-    goto bad;

+  if (*mp == NULL || error !=0)
+    return (error);

>Release-Note:
>Audit-Trail:
>Unformatted: