svn commit: r336282 - head/sys/netinet

[John Baldwin]

On 7/14/18 9:19 AM, Sean Bruno wrote:
> Author: sbruno
> Date: Sat Jul 14 16:19:46 2018
> New Revision: 336282
> URL: https://svnweb.freebsd.org/changeset/base/336282
> 
> Log:
>   Fixup memory management for fetching options in ip_ctloutput()
>   
>   Submitted by:	Jason Eggleston <jason at eggnet.com>
>   Sponsored by:	Limelight Networks
>   Differential Revision:	https://reviews.freebsd.org/D14621
> 
> Modified:
>   head/sys/netinet/ip_output.c
> 
> Modified: head/sys/netinet/ip_output.c
> ==============================================================================
> --- head/sys/netinet/ip_output.c	Sat Jul 14 16:06:53 2018	(r336281)
> +++ head/sys/netinet/ip_output.c	Sat Jul 14 16:19:46 2018	(r336282)
> @@ -1256,12 +1256,18 @@ ip_ctloutput(struct socket *so, struct sockopt *sopt)
>  		switch (sopt->sopt_name) {
>  		case IP_OPTIONS:
>  		case IP_RETOPTS:
> -			if (inp->inp_options)
> +			if (inp->inp_options) {
> +				unsigned long len = ulmin(inp->inp_options->m_len, sopt->sopt_valsize);
> +				struct mbuf *options = malloc(len, M_TEMP, M_WAITOK);
> +				INP_RLOCK(inp);
> +				bcopy(inp->inp_options, options, len);
> +				INP_RUNLOCK(inp);
>  				error = sooptcopyout(sopt,
> -						     mtod(inp->inp_options,
> +						     mtod(options,
>  							  char *),
> -						     inp->inp_options->m_len);
> -			else
> +						     len);
> +				free(options, M_TEMP);
> +			} else
>  				sopt->sopt_valsize = 0;
>  			break;

You can't malloc an mbuf, and you don't really want an mbuf here anyway.
Also, style(9) doesn't normally assign values when a variable is created.
I would perhaps have done something like this:

	if (inp->inp_options) {
		void *options;
		u_long len;

		INP_RLOCK(inp);
		len = ulmin(inp->inp_options->m_len, sopt->sopt_valsize);
		INP_RUNLOCK(inp);
		options = malloc(len, M_TEMP, M_WAITOK);
		INP_RLOCK(inp);
		len = ulmin(inp->inp_options->m_len, len);
		m_copydata(inp->inp_options, 0, len, options);
		INP_RUNLOCK(inp);
		error = sooptcopyout(sopt, options, len);
		free(options, M_TEMP);
	}

The current code isn't doing what you think it is doing since the bcopy()
copies the m_data pointer from 'inp_options' into 'options' so that
'options->m_data' points at the m_data buffer in the 'inp_options' mbuf.
Thus, the mtod() returns a pointer into 'inp_options' just like the old
code, so this commit doesn't change anything in terms of the previous
race (it is still just as broken).

Also, while INP_RLOCK isn't helpful in my version above for reading the
'm_len' member (it's racy to read and then malloc no matter what), you
still need the lock to ensure the inp_options pointer itself is valid
when you dereference it.  Without the lock another thread might have
freed inp_options out from under you and the unlocked dereference could
panic (though it probably just reads garbage on most of our architectures
rather than panicking since we don't tend to invalidate KVA if you lose
that race).

-- 
John Baldwin