From owner-svn-src-head@freebsd.org Mon Sep 26 11:56:02 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id C0DBFBEBB28; Mon, 26 Sep 2016 11:56:02 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 416473DD; Mon, 26 Sep 2016 11:56:01 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-149-109.carlnfd1.nsw.optusnet.com.au (c122-106-149-109.carlnfd1.nsw.optusnet.com.au [122.106.149.109]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 0F6A1104541E; Mon, 26 Sep 2016 21:55:52 +1000 (AEST) Date: Mon, 26 Sep 2016 21:55:51 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Hiren Panchasara cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r306337 - head/sys/kern In-Reply-To: <201609261013.u8QADwrV002892@repo.freebsd.org> Message-ID: <20160926203343.S1542@besplex.bde.org> References: <201609261013.u8QADwrV002892@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=EfU1O6SC c=1 sm=1 tr=0 a=R/f3m204ZbWUO/0rwPSMPw==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=HHGDD-5mAAAA:8 a=m-UwTWmGBv1XtxhJIZQA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 a=B8TCfWyN7RlcJ1j5uPC-:22 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 26 Sep 2016 11:56:02 -0000 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 > 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