Page MenuHomeFreeBSD

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

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

Details

Summary

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

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

Event Timeline

lohithbsd_gmail.com retitled this revision from to Rework: Fix to avoid potential mbuf leak in network stack, if userland sends control messages to bad file descriptors..Oct 5 2016, 5:01 AM
lohithbsd_gmail.com updated this object.
lohithbsd_gmail.com edited the test plan for this revision. (Show Details)
lohithbsd_gmail.com updated this revision to Diff 21052.
glebius edited edge metadata.Oct 7 2016, 5:21 PM

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

sys/kern/uipc_syscalls.c
766 ↗(On Diff #21052)

Doesn't the leak happen here?

lohithbsd_gmail.com updated this revision to Diff 21187.

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

lohithbsd_gmail.com marked an inline comment as done.Oct 8 2016, 5:26 PM

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.Oct 13 2016, 9:08 PM
glebius accepted this revision.
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.