Re: [PATCH -next v2] unix stream: Fix use-after-free crashes

["Yan, Zheng" <zheng.z.yan@intel.com>]

On 09/08/2011 01:59 PM, Eric Dumazet wrote:
> Le mercredi 07 septembre 2011 à 14:06 -0700, Tim Chen a écrit :
>> On Thu, 2011-09-08 at 08:27 +0800, Yan, Zheng wrote:
>>
>>>>  	err = -EPIPE;
>>>>  out_err:
>>>> -	if (skb == NULL)
>>>> +	if (!steal_refs)
>>>>  		scm_destroy(siocb->scm);
>>>
>>> I think we should call scm_release() here in the case of
>>> steal_refs == true. Otherwise siocb->scm->fp may leak.
>>
>> Yan Zheng,
>>
>> I've updated the patch.  If it looks good to you now, can you add your
>> Signed-off-by to the patch.  Pending Sedat's testing on this patch,
>> I think it is good to go.
>>
>> Tim
>>
>> ---
>> Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
>> in Unix socket's send and receive path) introduced a use-after-free bug.
>> The sent skbs from unix_stream_sendmsg could be consumed and destructed 
>> by the receive side, removing all references to the credentials, 
>> before the send side has finished sending out all 
>> packets. However, send side could continue to consturct new packets in the 
>> stream, using credentials that have lost its last reference and been
>> freed.  
>>
>> In this fix, we don't steal the reference to credentials we have obtained 
>> in scm_send at beginning of unix_stream_sendmsg, till we've reached
>> the last packet.  This fixes the problem in commit 0856a30409.
>>
>> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
>> Reported-by: Jiri Slaby <jirislaby@gmail.com>
>> Tested-by: Sedat Dilek <sedat.dilek@googlemail.com>
>> Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
>> ---
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index 136298c..47780dc 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -1383,10 +1383,11 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
>>  }
>>  
>>  static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb,
>> -			   bool send_fds, bool ref)
>> +			   bool send_fds, bool steal_refs)
>>  {
>>  	int err = 0;
>> -	if (ref) {
>> +
>> +	if (!steal_refs) {
>>  		UNIXCB(skb).pid  = get_pid(scm->pid);
>>  		UNIXCB(skb).cred = get_cred(scm->cred);
>>  	} else {
>> @@ -1458,7 +1459,7 @@ static int unix_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock,
>>  	if (skb == NULL)
>>  		goto out;
>>  
>> -	err = unix_scm_to_skb(siocb->scm, skb, true, false);
>> +	err = unix_scm_to_skb(siocb->scm, skb, true, true);
>>  	if (err < 0)
>>  		goto out_free;
>>  	max_level = err + 1;
>> @@ -1581,6 +1582,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
>>  	int sent = 0;
>>  	struct scm_cookie tmp_scm;
>>  	bool fds_sent = false;
>> +	bool steal_refs = false;
>>  	int max_level;
>>  
>>  	if (NULL == siocb->scm)
>> @@ -1642,11 +1644,14 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
>>  		size = min_t(int, size, skb_tailroom(skb));
>>  
>>
>> -		/* Only send the fds and no ref to pid in the first buffer */
>> -		err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
>> +		/* Only send the fds in first buffer
>> +		 * Last buffer can steal our references to pid/cred
>> +		 */
>> +		steal_refs = (sent + size >= len);
>> +		err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, steal_refs);
>>  		if (err < 0) {
>>  			kfree_skb(skb);
>> -			goto out;
>> +			goto out_err;
>>  		}
>>  		max_level = err + 1;
>>  		fds_sent = true;
>> @@ -1654,7 +1659,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
>>  		err = memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size);
>>  		if (err) {
>>  			kfree_skb(skb);
>> -			goto out;
>> +			goto out_err;
>>  		}
>>  
>>  		unix_state_lock(other);
>> @@ -1671,7 +1676,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
>>  		sent += size;
>>  	}
>>  
>> -	if (skb)
>> +	if (steal_refs)
>>  		scm_release(siocb->scm);
>>  	else
>>  		scm_destroy(siocb->scm);
>> @@ -1687,9 +1692,10 @@ pipe_err:
>>  		send_sig(SIGPIPE, current, 0);
>>  	err = -EPIPE;
>>  out_err:
>> -	if (skb == NULL)
>> +	if (steal_refs)
>> +		scm_release(siocb->scm);
>> +	else
>>  		scm_destroy(siocb->scm);
>> -out:
>>  	siocb->scm = NULL;
>>  	return sent ? : err;
>>  }
>>
>>
>>
> 
> I dont think this patch is good.
> 
> Sedat traces have nothing to do with af_unix.
> 
> Once unix_scm_to_skb() was called and successful, and steal_refs is true
> our refs are attached to this skb. They will be released by
> skb_free(skb). Same for fp : They either were sent in a previous skb or
> this one.
> 
> This is why the "goto out;" was OK.
> 

I don't think so. unix_scm_to_skb() calls unix_attach_fds(), it
always duplicates scm->fp. 

Regards