Page MenuHomeFreeBSD
Differential D8152

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

Authored by lohithbsd_gmail.com on Oct 5 2016, 5:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 6:55 AM
Unknown Object (File)
Oct 15 2024, 4:17 PM
Unknown Object (File)
Oct 6 2024, 11:08 PM
Unknown Object (File)
Oct 5 2024, 12:01 PM
Unknown Object (File)
Oct 5 2024, 10:33 AM
Unknown Object (File)
Oct 2 2024, 11:53 AM
Unknown Object (File)
Oct 2 2024, 8:59 AM
Unknown Object (File)
Sep 30 2024, 7:27 PM
View All Files
Subscribers
imp

Details

Reviewers
hiren
glebius
Commits
rS307745: Rework r306337.
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 Passed
Unit
No Test Coverage
Build Status
Buildable 5515
Build 5740: arc lint + arc unit

Event Timeline

lohithbsd_gmail.com updated this revision to Diff 21052.Oct 5 2016, 5:01 AM
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)
lohithbsd_gmail.com added reviewers: glebius, hiren, adrian.
Herald added a subscriber: imp. · View Herald TranscriptOct 5 2016, 5:01 AM
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
768–769

Doesn't the leak happen here?

lohithbsd_gmail.com updated this revision to Diff 21187.Oct 8 2016, 5:23 PM
lohithbsd_gmail.com edited edge metadata.

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!

lohithbsd_gmail.com updated this revision to Diff 21188.Oct 8 2016, 5:41 PM

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

glebius added a comment.Oct 11 2016, 8:56 PM

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

lohithbsd_gmail.com updated this revision to Diff 21289.Oct 12 2016, 5:08 AM

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

glebius accepted this revision.Oct 13 2016, 9:08 PM
glebius edited edge metadata.
This revision is now accepted and ready to land.Oct 13 2016, 9:08 PM
lohithbsd_gmail.com removed a reviewer: adrian.Oct 14 2016, 4:32 AM
Closed by commit rS307745: Rework r306337. (authored by hiren). · Explain WhyOct 21 2016, 6:27 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
kern/
10 lines

Diff 21187

View Options

sys/kern/uipc_syscalls.c

Loading...