netdev - RE: [PATCH net-next] net: adjust skb->truesize in pskb_expand

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 27 Jan 2017 10:49:45 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Eric Dumazet' <eric.dumazet@...il.com>,
        David Miller <davem@...emloft.net>
CC:     netdev <netdev@...r.kernel.org>,
        Slava Shwartsman <slavash@...lanox.com>
Subject: RE: [PATCH net-next] net: adjust skb->truesize in pskb_expand_head()

From: Eric Dumazet
> Sent: 27 January 2017 00:21
> Slava Shwartsman reported a warning in skb_try_coalesce(), when we
> detect skb->truesize is completely wrong.
> 
> In his case, issue came from IPv6 reassembly coping with malicious
> datagrams, that forced various pskb_may_pull() to reallocate a bigger
> skb->head than the one allocated by NIC driver before entering GRO
> layer.
> 
> Current code does not change skb->truesize, leaving this burden to
> callers if they care enough.
> 
> Blindly changing skb->truesize in pskb_expand_head() is not
> easy, as some producers might track skb->truesize, for example
> in xmit path for back pressure feedback (sk->sk_wmem_alloc)
> 
> We can detect the cases where it should be safe to change
> skb->truesize :
> 
> 1) skb is not attached to a socket.
> 2) If it is attached to a socket, destructor is sock_edemux()
...
>  int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
>  		     gfp_t gfp_mask)
>  {
> +	int i, osize = skb_end_offset(skb);
> +	int size = osize + nhead + ntail;
>  	long off;
> +	u8 *data;
> 
>  	BUG_ON(nhead < 0);
> 
> @@ -1257,6 +1257,14 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
>  	skb->hdr_len  = 0;
>  	skb->nohdr    = 0;
>  	atomic_set(&skb_shinfo(skb)->dataref, 1);
> +
> +	/* It is not generally safe to change skb->truesize.
> +	 * For the moment, we really care of rx path, or
> +	 * when skb is orphaned (not attached to a socket)
> +	 */
> +	if (!skb->sk || skb->destructor == sock_edemux)
> +		skb->truesize += size - osize;

That calculation doesn't look right to me at all.
Isn't 'truesize' supposed to reflect the amount of memory allocated to the skb.
So you need the difference between the size of the new and old memory blocks.

I'm also guessing that extra headroom can be generated by stealing unused tailroom.

	David