netdev - Re: [PATCH net] tcp: do not underestimate skb->truesize in tcp_trim

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: <CAAeHK+xWDnPu05g4Hqt3wkHEsuW=Ry7p7rLnxL3ct1MuSLBXLw@mail.gmail.com>
Date:   Thu, 27 Apr 2017 13:49:57 +0200
From:   Andrey Konovalov <andreyknvl@...gle.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] tcp: do not underestimate skb->truesize in tcp_trim_head()

On Thu, Apr 27, 2017 at 2:15 AM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> From: Eric Dumazet <edumazet@...gle.com>
>
> Andrey found a way to trigger the WARN_ON_ONCE(delta < len) in
> skb_try_coalesce() using syzkaller and a filter attached to a TCP
> socket over loopback interface.
>
> I believe one issue with looped skbs is that tcp_trim_head() can end up
> producing skb with under estimated truesize.
>
> It hardly matters for normal conditions, since packets sent over
> loopback are never truncated.
>
> Bytes trimmed from skb->head should not change skb truesize, since
> skb->head is not reallocated.

Hi Eric,

With all 3 of your patches applied to net-next I don't see the warning any more.

Thanks!

Tested-by: Andrey Konovalov <andreyknvl@...gle.com>

>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Reported-by: Andrey Konovalov <andreyknvl@...gle.com>
> ---
>  net/ipv4/tcp_output.c |   19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index c3c082ed38796f65948e7a1042e37813b71d5abf..a85d863c44196e60fd22e25471cf773e72d2c133 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1267,7 +1267,7 @@ int tcp_fragment(struct sock *sk, struct sk_buff *skb, u32 len,
>   * eventually). The difference is that pulled data not copied, but
>   * immediately discarded.
>   */
> -static void __pskb_trim_head(struct sk_buff *skb, int len)
> +static int __pskb_trim_head(struct sk_buff *skb, int len)
>  {
>         struct skb_shared_info *shinfo;
>         int i, k, eat;
> @@ -1277,7 +1277,7 @@ static void __pskb_trim_head(struct sk_buff *skb, int len)
>                 __skb_pull(skb, eat);
>                 len -= eat;
>                 if (!len)
> -                       return;
> +                       return 0;
>         }
>         eat = len;
>         k = 0;
> @@ -1303,23 +1303,28 @@ static void __pskb_trim_head(struct sk_buff *skb, int len)
>         skb_reset_tail_pointer(skb);
>         skb->data_len -= len;
>         skb->len = skb->data_len;
> +       return len;
>  }
>
>  /* Remove acked data from a packet in the transmit queue. */
>  int tcp_trim_head(struct sock *sk, struct sk_buff *skb, u32 len)
>  {
> +       u32 delta_truesize;
> +
>         if (skb_unclone(skb, GFP_ATOMIC))
>                 return -ENOMEM;
>
> -       __pskb_trim_head(skb, len);
> +       delta_truesize = __pskb_trim_head(skb, len);
>
>         TCP_SKB_CB(skb)->seq += len;
>         skb->ip_summed = CHECKSUM_PARTIAL;
>
> -       skb->truesize        -= len;
> -       sk->sk_wmem_queued   -= len;
> -       sk_mem_uncharge(sk, len);
> -       sock_set_flag(sk, SOCK_QUEUE_SHRUNK);
> +       if (delta_truesize) {
> +               skb->truesize      -= delta_truesize;
> +               sk->sk_wmem_queued -= delta_truesize;
> +               sk_mem_uncharge(sk, delta_truesize);
> +               sock_set_flag(sk, SOCK_QUEUE_SHRUNK);
> +       }
>
>         /* Any change of skb->len requires recalculation of tso factor. */
>         if (tcp_skb_pcount(skb) > 1)
>
>