linux-kernel - Re: [PATCH 3.2 089/110] net: Clone skb before setting peeked flag

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55C88CE1.2030605@yandex-team.ru>
Date:	Mon, 10 Aug 2015 14:37:05 +0300
From:	Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
To:	Ben Hutchings <ben@...adent.org.uk>, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
CC:	akpm@...ux-foundation.org, "David S. Miller" <davem@...emloft.net>,
	Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [PATCH 3.2 089/110] net: Clone skb before setting peeked flag

On 10.08.2015 13:12, Ben Hutchings wrote:
> 3.2.71-rc1 review patch.  If anyone has any objections, please let me know.

Here is important fix: https://patchwork.ozlabs.org/patch/503374/
"net: Fix skb_set_peeked use-after-free". not in upstream yet.

>
> ------------------
>
> From: Herbert Xu <herbert@...dor.apana.org.au>
>
> commit 738ac1ebb96d02e0d23bc320302a6ea94c612dec upstream.
>
> Shared skbs must not be modified and this is crucial for broadcast
> and/or multicast paths where we use it as an optimisation to avoid
> unnecessary cloning.
>
> The function skb_recv_datagram breaks this rule by setting peeked
> without cloning the skb first.  This causes funky races which leads
> to double-free.
>
> This patch fixes this by cloning the skb and replacing the skb
> in the list when setting skb->peeked.
>
> Fixes: a59322be07c9 ("[UDP]: Only increment counter on first peek/recv")
> Reported-by: Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
> Signed-off-by: Herbert Xu <herbert@...dor.apana.org.au>
> Signed-off-by: David S. Miller <davem@...emloft.net>
> [bwh: Backported to 3.2: adjust context]
> Signed-off-by: Ben Hutchings <ben@...adent.org.uk>
> ---
>   net/core/datagram.c | 41 ++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 38 insertions(+), 3 deletions(-)
>
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -128,6 +128,35 @@ out_noerr:
>   	goto out;
>   }
>
> +static int skb_set_peeked(struct sk_buff *skb)
> +{
> +	struct sk_buff *nskb;
> +
> +	if (skb->peeked)
> +		return 0;
> +
> +	/* We have to unshare an skb before modifying it. */
> +	if (!skb_shared(skb))
> +		goto done;
> +
> +	nskb = skb_clone(skb, GFP_ATOMIC);
> +	if (!nskb)
> +		return -ENOMEM;
> +
> +	skb->prev->next = nskb;
> +	skb->next->prev = nskb;
> +	nskb->prev = skb->prev;
> +	nskb->next = skb->next;
> +
> +	consume_skb(skb);
> +	skb = nskb;
> +
> +done:
> +	skb->peeked = 1;
> +
> +	return 0;
> +}
> +
>   /**
>    *	__skb_recv_datagram - Receive a datagram skbuff
>    *	@sk: socket
> @@ -160,7 +189,9 @@ out_noerr:
>   struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned flags,
>   				    int *peeked, int *err)
>   {
> +	struct sk_buff_head *queue = &sk->sk_receive_queue;
>   	struct sk_buff *skb;
> +	unsigned long cpu_flags;
>   	long timeo;
>   	/*
>   	 * Caller is allowed not to check sk->sk_err before skb_recv_datagram()
> @@ -179,15 +210,16 @@ struct sk_buff *__skb_recv_datagram(stru
>   		 * Look at current nfs client by the way...
>   		 * However, this function was correct in any case. 8)
>   		 */
> -		unsigned long cpu_flags;
> -		struct sk_buff_head *queue = &sk->sk_receive_queue;
> -
>   		spin_lock_irqsave(&queue->lock, cpu_flags);
>   		skb = skb_peek(queue);
>   		if (skb) {
>   			*peeked = skb->peeked;
>   			if (flags & MSG_PEEK) {
> -				skb->peeked = 1;
> +
> +				error = skb_set_peeked(skb);
> +				if (error)
> +					goto unlock_err;
> +
>   				atomic_inc(&skb->users);
>   			} else
>   				__skb_unlink(skb, queue);
> @@ -206,6 +238,8 @@ struct sk_buff *__skb_recv_datagram(stru
>
>   	return NULL;
>
> +unlock_err:
> +	spin_unlock_irqrestore(&queue->lock, cpu_flags);
>   no_packet:
>   	*err = error;
>   	return NULL;
>


-- 
Konstantin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/