svn commit: r336282 - head/sys/netinet

[Sean Bruno]

On 07/14/18 10:37, John Baldwin wrote:
> 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).
> 


Shall I revert and rethink?

sean

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 618 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20180714/0d409328/attachment.sig>