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 <ja...@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
signature.asc
Description: OpenPGP digital signature
