netdev - Re: [PATCH] net/unix: fix logic about sk_peek

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: <CANaxB-zvEL6WRNk2Ho7uJUVO8L=t_Nutp9Zxz=Do+Xp1JibHKw@mail.gmail.com>
Date:	Fri, 2 Oct 2015 12:04:39 +0300
From:	Andrey Vagin <avagin@...nvz.org>
To:	netdev@...r.kernel.org
Cc:	Andrey Vagin <avagin@...nvz.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Aaron Conole <aconole@...heb.org>
Subject: Re: [PATCH] net/unix: fix logic about sk_peek_offset

2015-10-02 0:05 GMT+03:00 Andrey Vagin <avagin@...il.com>:
> From: Andrey Vagin <avagin@...nvz.org>
>
> Now send with MSG_PEEK can return data from multiple SKBs.
>
> Unfortunately we take into account the peek offset for each skb,
> that is wrong. We need to apply the peek offset only once.
>
> In addition, the peek offset should be used only if MSG_PEEK is set.

The attached program can be used to reproduce the bug. It cycles in an
infinite loop without this patch, because recv() always returns data.

658   socketpair(PF_LOCAL, SOCK_STREAM, 0, [3, 4]) = 0
658   sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658   sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658   sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658   sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658   sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658   sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658   sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658   sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658   sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658   sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658   setsockopt(4, SOL_SOCKET, 0x2a /* SO_??? */, [0], 4) = 0
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   fstat(1, {st_mode=S_IFCHR|0666, st_rdev=makedev(1, 3), ...}) = 0
658   ioctl(1, TCGETS, 0x7fff54e57b30)  = -1 ENOTTY (Inappropriate
ioctl for device)
658   mmap(NULL, 4096, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f3a86827000
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658   recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
...

>
> Cc: "David S. Miller" <davem@...emloft.net> (maintainer:NETWORKING
> Cc: Eric Dumazet <edumazet@...gle.com> (commit_signer:1/14=7%)
> Cc: Aaron Conole <aconole@...heb.org>
> Fixes: 9f389e35674f ("af_unix: return data from multiple SKBs on recv() with MSG_PEEK flag")
> Signed-off-by: Andrey Vagin <avagin@...nvz.org>
> ---
>  net/unix/af_unix.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index ef31b40..94f6582 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2064,6 +2064,11 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
>                 goto out;
>         }
>
> +       if (flags & MSG_PEEK)
> +               skip = sk_peek_offset(sk, flags);
> +       else
> +               skip = 0;
> +
>         do {
>                 int chunk;
>                 struct sk_buff *skb, *last;
> @@ -2112,7 +2117,6 @@ unlock:
>                         break;
>                 }
>
> -               skip = sk_peek_offset(sk, flags);
>                 while (skip >= unix_skb_len(skb)) {
>                         skip -= unix_skb_len(skb);
>                         last = skb;
> @@ -2179,14 +2183,12 @@ unlock:
>                         if (UNIXCB(skb).fp)
>                                 scm.fp = scm_fp_dup(UNIXCB(skb).fp);
>
> -                       if (skip) {
> -                               sk_peek_offset_fwd(sk, chunk);
> -                               skip -= chunk;
> -                       }
> +                       sk_peek_offset_fwd(sk, chunk);
>
>                         if (UNIXCB(skb).fp)
>                                 break;
>
> +                       skip = 0;
>                         last = skb;
>                         last_len = skb->len;
>                         unix_state_lock(sk);
> --
> 2.4.3
>

View attachment "test.c" of type "text/x-csrc" (743 bytes)