On Mon, 26 Sep 2016, Hiren Panchasara wrote:
Author: hiren
Date: Mon Sep 26 10:13:58 2016
New Revision: 306337
URL: https://svnweb.freebsd.org/changeset/base/306337
Log:
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. Fix this possible leak.
Submitted by: Lohith Bellad <lohith.bel...@me.com>
Reviewed by: adrian
MFC after: 3 weeks
Differential Revision: https://reviews.freebsd.org/D7910
Modified:
head/sys/kern/uipc_syscalls.c
I don't like this. It has some style bugs and seems to be very broken.
Modified: head/sys/kern/uipc_syscalls.c
==============================================================================
--- head/sys/kern/uipc_syscalls.c Mon Sep 26 08:21:29 2016
(r306336)
+++ head/sys/kern/uipc_syscalls.c Mon Sep 26 10:13:58 2016
(r306337)
@@ -685,7 +685,7 @@ sys_socketpair(struct thread *td, struct
static int
sendit(struct thread *td, int s, struct msghdr *mp, int flags)
{
- struct mbuf *control;
+ struct mbuf *control = NULL;
struct sockaddr *to;
int error;
Style bug: initialization in declaration. Many collateral style bugs
and obfuscations.
@@ -737,6 +737,8 @@ sendit(struct thread *td, int s, struct
bad:
free(to, M_SONAME);
+ if (control)
+ m_freem(control);
return (error);
}
The "bad" label is an old style bug. This is the general return path,
not an error handling path. This gives many collateral obfuscations
and pessimizations.
Now it seems to give a large bug with the help of the obfuscations:
when mp->msg_control != NULL, in the usual subcase where there is no
error, kern_sendit() must have already done m_freem(control) else the
usual subcase would have leaked. The code falls through to "bad"
and does m_freem(control) again.
I still use my optimization for sendit() which avoids using malloc()
for 'to'. It has to be more careful not to free things. The design
for freeing 'control' is worse than for freeing 'to', as shown by
this bug and its extension.
X Index: uipc_syscalls.c
X ===================================================================
X --- uipc_syscalls.c (revision 306310)
X +++ uipc_syscalls.c (working copy)
X @@ -114,6 +114,26 @@
X return (0);
X }
X
X +static __inline int
X +lgetsockaddr(struct sockaddr *sa, caddr_t uaddr, size_t len)
X +{
X + int error;
X +
X + if (len > SOCK_MAXADDRLEN)
X + return (ENAMETOOLONG);
X + if (len < offsetof(struct sockaddr, sa_data[0]))
X + return (EINVAL);
X + error = copyin(uaddr, sa, len);
X + if (error == 0) {
X +#if defined(COMPAT_OLDSOCK) && BYTE_ORDER != BIG_ENDIAN
X + if (sa->sa_family == 0 && sa->sa_len < AF_MAX)
X + sa->sa_family = sa->sa_len;
X +#endif
X + sa->sa_len = len;
X + }
X + return (error);
X +}
X +
X /*
X * System call interface to the socket abstraction.
X */
X @@ -687,6 +707,7 @@
X {
X struct mbuf *control;
X struct sockaddr *to;
X + char sa[SOCK_MAXADDRLEN] __aligned(16);
X int error;
X
X #ifdef CAPABILITY_MODE
X @@ -695,7 +716,8 @@
X #endif
X
X if (mp->msg_name != NULL) {
X - error = getsockaddr(&to, mp->msg_name, mp->msg_namelen);
X + to = (struct sockaddr *)&sa[0];
X + error = lgetsockaddr(to, mp->msg_name, mp->msg_namelen);
X if (error != 0) {
X to = NULL;
X goto bad;
The error handing should be cleaned up by removing all the gotos and the
bad label, but this patch is written to minimize diffs.
X @@ -736,7 +758,6 @@
X error = kern_sendit(td, s, mp, flags, control, UIO_USERSPACE);
X
X bad:
X - free(to, M_SONAME);
X return (error);
X }
X
After merging this fix, cleaning up the gotos is more needed. Untested
merge:
Y Index: uipc_syscalls.c
Y ===================================================================
Y --- uipc_syscalls.c (revision 306337)
Y +++ uipc_syscalls.c (working copy)
Y @@ -114,6 +114,26 @@
Y return (0);
Y }
Y
Y +static __inline int
Y +lgetsockaddr(struct sockaddr *sa, caddr_t uaddr, size_t len)
Y +{
Y + int error;
Y +
Y + if (len > SOCK_MAXADDRLEN)
Y + return (ENAMETOOLONG);
Y + if (len < offsetof(struct sockaddr, sa_data[0]))
Y + return (EINVAL);
Y + error = copyin(uaddr, sa, len);
Y + if (error == 0) {
Y +#if defined(COMPAT_OLDSOCK) && BYTE_ORDER != BIG_ENDIAN
Y + if (sa->sa_family == 0 && sa->sa_len < AF_MAX)
Y + sa->sa_family = sa->sa_len;
Y +#endif
Y + sa->sa_len = len;
Y + }
Y + return (error);
Y +}
Y +
Y /*
Y * System call interface to the socket abstraction.
Y */
Y @@ -687,6 +707,7 @@
Y {
Y struct mbuf *control = NULL;
Y struct sockaddr *to;
Y + char sa[SOCK_MAXADDRLEN] __aligned(16);
Y int error;
Y
Y #ifdef CAPABILITY_MODE
Y @@ -695,14 +716,11 @@
Y #endif
Y
Y if (mp->msg_name != NULL) {
Y - error = getsockaddr(&to, mp->msg_name, mp->msg_namelen);
Y - if (error != 0) {
Y - to = NULL;
Y - goto bad;
Y - }
Y + to = (struct sockaddr *)&sa[0];
Y + error = lgetsockaddr(to, mp->msg_name, mp->msg_namelen);
Y + if (error != 0)
Y + return error);
Y mp->msg_name = to;
Y - } else {
Y - to = NULL;
Y }
Y
Y if (mp->msg_control) {
Y @@ -710,14 +728,12 @@
Y #ifdef COMPAT_OLDSOCK
Y && mp->msg_flags != MSG_COMPAT
Y #endif
Y - ) {
Y - error = EINVAL;
Y - goto bad;
Y - }
Y + )
Y + return (EINVAL);
Y error = sockargs(&control, mp->msg_control,
Y mp->msg_controllen, MT_CONTROL);
Y if (error != 0)
Y - goto bad;
Y + return (error);
Here it must be checked that 'control' is not allocated if an error is
returned (only a bad API would leave it allocated).
Y #ifdef COMPAT_OLDSOCK
Y if (mp->msg_flags == MSG_COMPAT) {
Y struct cmsghdr *cm;
Y @@ -729,17 +745,9 @@
Y cm->cmsg_type = SCM_RIGHTS;
Y }
Y #endif
Y - } else {
Y - control = NULL;
Y }
Y
Y - error = kern_sendit(td, s, mp, flags, control, UIO_USERSPACE);
Y -
Y -bad:
Y - free(to, M_SONAME);
Y - if (control)
Y - m_freem(control);
Y - return (error);
Y + return (kern_sendit(td, s, mp, flags, control, UIO_USERSPACE));
Y }
Y
Y int
Bruce
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
