From owner-svn-src-head@freebsd.org Sat Jul 14 16:37:10 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D3B8F1032336; Sat, 14 Jul 2018 16:37:09 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from mail.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 3262E80264; Sat, 14 Jul 2018 16:37:08 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from John-Baldwins-MacBook-Pro-2.local (unknown [73.93.142.101]) by mail.baldwin.cx (Postfix) with ESMTPSA id 3BC1A10AFCD; Sat, 14 Jul 2018 12:37:02 -0400 (EDT) Subject: Re: svn commit: r336282 - head/sys/netinet To: Sean Bruno , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201807141619.w6EGJk2Y016469@repo.freebsd.org> From: John Baldwin Message-ID: <7bbbb553-fca5-9801-49aa-caf6df929c2f@FreeBSD.org> Date: Sat, 14 Jul 2018 09:37:09 -0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <201807141619.w6EGJk2Y016469@repo.freebsd.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (mail.baldwin.cx); Sat, 14 Jul 2018 12:37:02 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.99.2 at mail.baldwin.cx X-Virus-Status: Clean X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.27 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: Sat, 14 Jul 2018 16:37:10 -0000 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 > 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