netdev - Re: [PATCH net-next 2/2] mac80211: Resolve sk_refcnt/sk_wmem

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]

Date:	Thu, 11 Sep 2014 09:06:38 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	Alexander Duyck <alexander.h.duyck@...el.com>
Cc:	netdev@...r.kernel.org, linux-wireless@...r.kernel.org,
	davem@...emloft.net, eric.dumazet@...il.com, linville@...driver.com
Subject: Re: [PATCH net-next 2/2] mac80211: Resolve sk_refcnt/sk_wmem_alloc
 issue in wifi ack path

On Wed, 2014-09-10 at 18:05 -0400, Alexander Duyck wrote:
> There is a possible issue with the use, or lack thereof of sk_refcnt and
> sk_wmem_alloc in the wifi ack status functionality.
> 
> Specifically if a socket were to request acknowledgements, and the socket
> were to have sk_refcnt drop to 0 resulting in it waiting on sk_wmem_alloc
> to reach 0 it would be possible to have sock_queue_err_skb orphan the last
> buffer, resulting in __sk_free being called on the socket.  After this the
> buffer is enqueued on sk_error_queue, however the queue has already been
> flushed resulting in at least a memory leak, if not a data corruption.

Oh. Thanks :-)

> +	/* take a reference to prevent skb_orphan() from freeing the socket */
> +	sock_hold(sk);
> +
>  	err = sock_queue_err_skb(sk, skb);
>  	if (err)
>  		kfree_skb(skb);
> +
> +	sock_put(sk);
>  }
>  EXPORT_SYMBOL_GPL(skb_complete_wifi_ack);

Here I'm not sure it matters *for this function*? Wouldn't it be freed
then in sock_put(), which has the same net effect on this function
overall? It doesn't use it after sock_queue_err_skb().

Seems like maybe this should be in sock_queue_err_skb() itself, since it
does the orphaning first and then looks at the socket. Or the
documentation for that function should state that it has to be held, but
there are plenty of callers?

>  			spin_lock_irqsave(&local->ack_status_lock, flags);
> -			id = idr_alloc(&local->ack_status_frames, orig_skb,
> +			id = idr_alloc(&local->ack_status_frames, ack_skb,
>  				       1, 0x10000, GFP_ATOMIC);
>  			spin_unlock_irqrestore(&local->ack_status_lock, flags);
>  
>  			if (id >= 0) {
>  				info_id = id;
>  				info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
> -			} else if (skb_shared(skb)) {
> -				kfree_skb(orig_skb);
>  			} else {
> -				kfree_skb(skb);
> -				skb = orig_skb;
> +				kfree_skb(ack_skb);
>  			}

So you're removing this part, but can't we really not reuse the clone_sk
copy? The difference is that it's charged, but that's fine for the
purposes here, no? Or am I misunderstanding that?

johannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html