Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1630539 > unrolled thread

[PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 16:10 +0200
    [PATCH v2 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 16:20 +0200
      [PATCH v3 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 16:30 +0200
    [PATCH 2/5] ipsec: check return value of skb_to_sgvec always "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 16:20 +0200
    [PATCH 4/5] macsec: check return value of skb_to_sgvec always "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 16:20 +0200
    Re: [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to  prevent overflow David Miller <davem@davemloft.net> - 2017-04-25 16:50 +0200
      [PATCH v4 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 17:10 +0200
        Re: [PATCH v4 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to  prevent overflow David Miller <davem@davemloft.net> - 2017-04-25 17:20 +0200
          [PATCH v5 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 18:00 +0200
            [PATCH v5 3/5] rxrpc: check return value of skb_to_sgvec always "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 18:00 +0200
            [PATCH v5 4/5] macsec: check return value of skb_to_sgvec always "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 18:00 +0200
            [PATCH v5 2/5] ipsec: check return value of skb_to_sgvec always "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 18:00 +0200
            [PATCH v5 5/5] virtio_net: check return value of skb_to_sgvec always "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 18:00 +0200
            [PATCH v6 2/5] ipsec: check return value of skb_to_sgvec always "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 20:50 +0200
            [PATCH v6 4/5] macsec: check return value of skb_to_sgvec always "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 20:50 +0200
            [PATCH v6 5/5] virtio_net: check return value of skb_to_sgvec always "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 20:50 +0200
            [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 20:50 +0200
              [PATCH v6 3/5] rxrpc: check return value of skb_to_sgvec always "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-25 20:50 +0200
                Re: [PATCH v6 3/5] rxrpc: check return value of skb_to_sgvec always Sabrina Dubroca <sd@queasysnail.net> - 2017-04-28 13:50 +0200
                  Re: [PATCH v6 3/5] rxrpc: check return value of skb_to_sgvec always "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-28 15:40 +0200
              Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to  prevent overflow "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-27 11:30 +0200
                Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to  prevent overflow Sabrina Dubroca <sd@queasysnail.net> - 2017-04-27 13:40 +0200
                  Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to  prevent overflow "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-27 14:10 +0200
                    RE: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to  prevent overflow David Laight <David.Laight@ACULAB.COM> - 2017-04-27 17:00 +0200
                Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to  prevent overflow David Miller <davem@davemloft.net> - 2017-04-27 18:00 +0200
              Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to  prevent overflow Sabrina Dubroca <sd@queasysnail.net> - 2017-04-28 18:20 +0200
                Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to  prevent overflow "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-04-29 00:50 +0200
    Re: [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent  overflow Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2017-04-25 17:50 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1631988 — Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

Hey Dave,

David Laight and I have been discussing offlist. It occurred to both
of us that this could just be turned into a loop because perhaps this
is actually just tail-recursive. Upon further inspection, however, the
way the current algorithm works, it's possible that each of the
fraglist skbs has its own fraglist, which would make this into tree
recursion, which is why in the first place I wanted to place that
limit on it. If that's the case, then the patch I proposed above is
the best way forward. However, perhaps there's the chance that
fraglist skbs having separate fraglists are actually forbidden? Is
this the case? Are there other parts of the API that enforce this
contract? Is it something we could safely rely on here? If you say
yes, I'll send a v7 that makes this into a non-recursive loop.

Regards,
Jason

[toc] | [prev] | [next] | [standalone]

#1632045 — Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

2017-04-27, 11:21:51 +0200, Jason A. Donenfeld wrote:
> However, perhaps there's the chance that fraglist skbs having
> separate fraglists are actually forbidden? Is this the case?

Hmm, I think this can actually happen:

    /*  net/ipv4/ip_fragment.c  */
    static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
    			 struct net_device *dev)
    {
    
    ...
    
    	/* If the first fragment is fragmented itself, we split
    	 * it to two chunks: the first with data and paged part
    	 * and the second, holding only fragments. */
    	if (skb_has_frag_list(head)) {
    		struct sk_buff *clone;
    		int i, plen = 0;
    
    		clone = alloc_skb(0, GFP_ATOMIC);
    		if (!clone)
    			goto out_nomem;
    		clone->next = head->next;
    		head->next = clone;
    		skb_shinfo(clone)->frag_list = skb_shinfo(head)->frag_list;
    		skb_frag_list_init(head);
    		for (i = 0; i < skb_shinfo(head)->nr_frags; i++)
    			plen += skb_frag_size(&skb_shinfo(head)->frags[i]);
    		clone->len = clone->data_len = head->data_len - plen;
    		head->data_len -= clone->len;
    		head->len -= clone->len;
    		clone->csum = 0;
    		clone->ip_summed = head->ip_summed;
    		add_frag_mem_limit(qp->q.net, clone->truesize);
    	}
    
    ...
    }
    

You can test that with a vxlan tunnel on top of a vxlan tunnel ("real"
MTU is 1500, first tunnel MTU set to 10000, second tunnel MTU set to
40000 -- or anything, as long as they both get fragmented).

-- 
Sabrina

[toc] | [prev] | [next] | [standalone]

#1632059 — Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

On Thu, Apr 27, 2017 at 1:30 PM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> Hmm, I think this can actually happen:

Alright, perhaps better to err on the side of caution, then.

Jason

[toc] | [prev] | [next] | [standalone]

#1632157 — RE: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

From: Jason A. Donenfeld
> On Thu, Apr 27, 2017 at 1:30 PM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> > Hmm, I think this can actually happen:
> 
> Alright, perhaps better to err on the side of caution, then.

You only need to recurse if both pointers are set.

	David

[toc] | [prev] | [next] | [standalone]

#1632217 — Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Thu, 27 Apr 2017 11:21:51 +0200

> Hey Dave,
> 
> David Laight and I have been discussing offlist. It occurred to both
> of us that this could just be turned into a loop because perhaps this
> is actually just tail-recursive. Upon further inspection, however, the
> way the current algorithm works, it's possible that each of the
> fraglist skbs has its own fraglist, which would make this into tree
> recursion, which is why in the first place I wanted to place that
> limit on it. If that's the case, then the patch I proposed above is
> the best way forward. However, perhaps there's the chance that
> fraglist skbs having separate fraglists are actually forbidden? Is
> this the case? Are there other parts of the API that enforce this
> contract? Is it something we could safely rely on here? If you say
> yes, I'll send a v7 that makes this into a non-recursive loop.

As Sabrina showed, it can happen.  There are no such restrictions on
the geometry of an SKB.

[toc] | [prev] | [next] | [standalone]

#1633003 — Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

2017-04-25, 20:47:30 +0200, Jason A. Donenfeld wrote:
> This is a defense-in-depth measure in response to bugs like
> 4d6fa57b4dab ("macsec: avoid heap overflow in skb_to_sgvec"). While
> we're at it, we also limit the amount of recursion this function is
> allowed to do. Not actually providing a bounded base case is a future
> diaster that we can easily avoid here.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Changes v5->v6:
>   * Use unlikely() for the rare overflow conditions.
>   * Also bound recursion, since this is a potential disaster we can avert.
> 
>  net/core/skbuff.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index f86bf69cfb8d..24fb53f8534e 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3489,16 +3489,22 @@ void __init skb_init(void)
>   *	@len: Length of buffer space to be mapped
>   *
>   *	Fill the specified scatter-gather list with mappings/pointers into a
> - *	region of the buffer space attached to a socket buffer.
> + *	region of the buffer space attached to a socket buffer. Returns either
> + *	the number of scatterlist items used, or -EMSGSIZE if the contents
> + *	could not fit.
>   */

One small thing here: since you're touching this comment, could you
move it next to skb_to_sgvec, since that's the function it's supposed
to document?

Thanks!

>  static int
> -__skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
> +__skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len,
> +	       unsigned int recursion_level)

-- 
Sabrina

[toc] | [prev] | [next] | [standalone]

#1633196 — Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

Hi Sabrina,

On Fri, Apr 28, 2017 at 6:18 PM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> One small thing here: since you're touching this comment, could you
> move it next to skb_to_sgvec, since that's the function it's supposed
> to document?

Done. I'll wait until next week to resubmit, to give some more time
for comments, but my current living copy of this series is here:
https://git.zx2c4.com/linux-dev/log/?h=jd/safe-skb-vec

One thing I'm considering, after discussing with David Laight, is the
potential of just using an explicit stack array for pushing and
popping skbs, rather than using the call stack. While this increases
complexity, which I'm opposed to, David makes the point that on some
architectures, the stack frame is rather large, and 32 function calls
of recursion might not be a good idea. Any opinons on this? Overkill
and simplicity is preferred? Or in fact best practice? (Either way,
I'll do a trial implementation of it to get an idea of how the end
result feels.)

[toc] | [prev] | [next] | [standalone]

#1630703 — Re: [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

Hello!

On 04/25/2017 05:08 PM, Jason A. Donenfeld wrote:

> This is a defense-in-depth measure in response to bugs like
> 4d6fa57b4dab0d77f4d8e9d9c73d1e63f6fe8fee.

    You need to also specify the summary line enclosed in (""). And it's 
enough to specify 12 digits of SHA1 ID...

> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
[...]

MBR, Sergei

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web