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.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 8, 1:56 AM
Unknown Object (File)
Fri, Mar 8, 1:26 AM
Unknown Object (File)
Jan 31 2024, 5:09 AM
Unknown Object (File)
Jan 31 2024, 5:09 AM
Unknown Object (File)
Jan 31 2024, 5:09 AM
Unknown Object (File)
Jan 31 2024, 5:09 AM
Unknown Object (File)
Jan 17 2024, 1:11 PM
Unknown Object (File)
Jan 14 2024, 6:21 PM
Subscribers

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 - subversion
Lint
Lint Not Applicable
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..
lohithbsd_gmail.com updated this object.
lohithbsd_gmail.com 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.

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

Doesn't the leak happen here?

lohithbsd_gmail.com 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.