Skip site navigation (1)Skip section navigation (2) 
Date:      Mon, 26 Sep 2016 21:55:51 +1000 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Hiren Panchasara <hiren@freebsd.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org,  svn-src-head@freebsd.org
Subject:   Re: svn commit: r306337 - head/sys/kern
Message-ID:  <20160926203343.S1542@besplex.bde.org>
In-Reply-To: <201609261013.u8QADwrV002892@repo.freebsd.org>
References:  <201609261013.u8QADwrV002892@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help 
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.bellad@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

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160926203343.S1542>