Page MenuHomeFreeBSD

Rework: Fix to avoid potential mbuf leak in network stack, if userland sends control messages to bad file descriptors.

Authored by on Oct 5 2016, 5:01 AM.



In sendit(), if mp->msg_control is present, then in sockargs() we are allocating mbuf to store mp->msg_control. Later in kern_sendit(), call to getsock_cap(), will check validity of file pointer passed, if this fails EBADF is returned but mbuf allocated in sockargs() is not freed. Made code changes to free the same.

Since its not possible to check and free the control mbuf correctly in sendit() routine.
We can clear the control mbuf in kern_sendit() routine after checking correctly.

Since, we know for sure sosend() routine will consume the control mbuf if its present else it will clear the mbuf. So, making control = NULL, after the call to sosend() will prevent double freeing of control mbuf.

If there are any errors before call to sosend() in kern_sendit(), for example EBADF (Bad File Descriptor) then we will fall to "bad:" and if control != NULL, we will clear the mbuf. This way mbuf leak for EBADF is also prevented.

Test Plan

Built custom kernel with this change and ran stress test suite.

Diff Detail

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline retitled this revision from to Rework: Fix to avoid potential mbuf leak in network stack, if userland sends control messages to bad file descriptors.. updated this object. edited the test plan for this revision. (Show Details)

I'd suggest to just put m_freem(control) into all error places, including getsock_cap() failure, instead of editing end of function.

766 ↗(On Diff #21052)

Doesn't the leak happen here? edited edge metadata.

Made suitable modifications to take care of all return paths leading to leak.

yes, taken care. Thanks!

Made code changes to clear control mbuf at all error places instead of at end of function.

m_freem() can accept NULL pointer just like free(). I'd suggest to make code more laconic and remove ifs.

Removed redundant (control != NULL) checks since m_freem() checks for NULL pointers.

glebius edited edge metadata.
This revision is now accepted and ready to land.Oct 13 2016, 9:08 PM
This revision was automatically updated to reflect the committed changes.