[PATCH v1 net] af_packet: Fix data-races of pkt_sk(sk)->num.

[PATCH v1 net] af_packet: Fix data-races of pkt_sk(sk)->num.

[Kuniyuki Iwashima]

syzkaller found a data race of pkt_sk(sk)->num.

The value is changed under lock_sock() and po->bind_lock, so we
need READ_ONCE() to access pkt_sk(sk)->num without these locks in
packet_bind_spkt(), packet_bind(), and sk_diag_fill().

Note that WRITE_ONCE() is already added by commit c7d2ef5dd4b0
("net/packet: annotate accesses to po->bind").

BUG: KCSAN: data-race in packet_bind / packet_do_bind

write (marked) to 0xffff88802ffd1cee of 2 bytes by task 7322 on cpu 0:
 packet_do_bind+0x446/0x640 net/packet/af_packet.c:3236
 packet_bind+0x99/0xe0 net/packet/af_packet.c:3321
 __sys_bind+0x19b/0x1e0 net/socket.c:1803
 __do_sys_bind net/socket.c:1814 [inline]
 __se_sys_bind net/socket.c:1812 [inline]
 __x64_sys_bind+0x40/0x50 net/socket.c:1812
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3b/0x90 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x72/0xdc

read to 0xffff88802ffd1cee of 2 bytes by task 7318 on cpu 1:
 packet_bind+0xbf/0xe0 net/packet/af_packet.c:3322
 __sys_bind+0x19b/0x1e0 net/socket.c:1803
 __do_sys_bind net/socket.c:1814 [inline]
 __se_sys_bind net/socket.c:1812 [inline]
 __x64_sys_bind+0x40/0x50 net/socket.c:1812
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3b/0x90 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x72/0xdc

value changed: 0x0300 -> 0x0000

Reported by Kernel Concurrency Sanitizer on:
CPU: 1 PID: 7318 Comm: syz-executor.4 Not tainted 6.3.0-13380-g7fddb5b5300c #4
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014

Fixes: 96ec6327144e ("packet: Diag core and basic socket info dumping")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzkaller <syzkaller@googlegroups.com>
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/packet/af_packet.c | 4 ++--
 net/packet/diag.c      | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 94c6a1ffa459..a1f9a0e9f3c8 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -3299,7 +3299,7 @@ static int packet_bind_spkt(struct socket *sock, struct sockaddr *uaddr,
 	memcpy(name, uaddr->sa_data, sizeof(uaddr->sa_data_min));
 	name[sizeof(uaddr->sa_data_min)] = 0;
 
-	return packet_do_bind(sk, name, 0, pkt_sk(sk)->num);
+	return packet_do_bind(sk, name, 0, READ_ONCE(pkt_sk(sk)->num));
 }
 
 static int packet_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
@@ -3317,7 +3317,7 @@ static int packet_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len
 		return -EINVAL;
 
 	return packet_do_bind(sk, NULL, sll->sll_ifindex,
-			      sll->sll_protocol ? : pkt_sk(sk)->num);
+			      sll->sll_protocol ? : READ_ONCE(pkt_sk(sk)->num));
 }
 
 static struct proto packet_proto = {
diff --git a/net/packet/diag.c b/net/packet/diag.c
index d0c4eda4cdc6..f6b200cb3c06 100644
--- a/net/packet/diag.c
+++ b/net/packet/diag.c
@@ -143,7 +143,7 @@ static int sk_diag_fill(struct sock *sk, struct sk_buff *skb,
 	rp = nlmsg_data(nlh);
 	rp->pdiag_family = AF_PACKET;
 	rp->pdiag_type = sk->sk_type;
-	rp->pdiag_num = ntohs(po->num);
+	rp->pdiag_num = ntohs(READ_ONCE(po->num));
 	rp->pdiag_ino = sk_ino;
 	sock_diag_save_cookie(sk, rp->pdiag_cookie);
 
-- 
2.30.2

Re: [PATCH v1 net] af_packet: Fix data-races of pkt_sk(sk)->num.

[Willem de Bruijn]

On Wed, May 24, 2023 at 7:30 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> syzkaller found a data race of pkt_sk(sk)->num.
>
> The value is changed under lock_sock() and po->bind_lock, so we
> need READ_ONCE() to access pkt_sk(sk)->num without these locks in
> packet_bind_spkt(), packet_bind(), and sk_diag_fill().
>
> Note that WRITE_ONCE() is already added by commit c7d2ef5dd4b0
> ("net/packet: annotate accesses to po->bind").
>
> BUG: KCSAN: data-race in packet_bind / packet_do_bind
>
> write (marked) to 0xffff88802ffd1cee of 2 bytes by task 7322 on cpu 0:
>  packet_do_bind+0x446/0x640 net/packet/af_packet.c:3236
>  packet_bind+0x99/0xe0 net/packet/af_packet.c:3321
>  __sys_bind+0x19b/0x1e0 net/socket.c:1803
>  __do_sys_bind net/socket.c:1814 [inline]
>  __se_sys_bind net/socket.c:1812 [inline]
>  __x64_sys_bind+0x40/0x50 net/socket.c:1812
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x3b/0x90 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>
> read to 0xffff88802ffd1cee of 2 bytes by task 7318 on cpu 1:
>  packet_bind+0xbf/0xe0 net/packet/af_packet.c:3322
>  __sys_bind+0x19b/0x1e0 net/socket.c:1803
>  __do_sys_bind net/socket.c:1814 [inline]
>  __se_sys_bind net/socket.c:1812 [inline]
>  __x64_sys_bind+0x40/0x50 net/socket.c:1812
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x3b/0x90 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>
> value changed: 0x0300 -> 0x0000
>
> Reported by Kernel Concurrency Sanitizer on:
> CPU: 1 PID: 7318 Comm: syz-executor.4 Not tainted 6.3.0-13380-g7fddb5b5300c #4
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
>
> Fixes: 96ec6327144e ("packet: Diag core and basic socket info dumping")
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzkaller <syzkaller@googlegroups.com>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH v1 net] af_packet: Fix data-races of pkt_sk(sk)->num.

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 24 May 2023 16:29:34 -0700 you wrote:
> syzkaller found a data race of pkt_sk(sk)->num.
> 
> The value is changed under lock_sock() and po->bind_lock, so we
> need READ_ONCE() to access pkt_sk(sk)->num without these locks in
> packet_bind_spkt(), packet_bind(), and sk_diag_fill().
> 
> Note that WRITE_ONCE() is already added by commit c7d2ef5dd4b0
> ("net/packet: annotate accesses to po->bind").
> 
> [...]

Here is the summary with links:
  - [v1,net] af_packet: Fix data-races of pkt_sk(sk)->num.
    https://git.kernel.org/netdev/net/c/822b5a1c17df

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v1 net] af_packet: Fix data-races of pkt_sk(sk)->num.

[Eric Dumazet]

On Thu, May 25, 2023 at 1:30 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> syzkaller found a data race of pkt_sk(sk)->num.
>
> The value is changed under lock_sock() and po->bind_lock, so we
> need READ_ONCE() to access pkt_sk(sk)->num without these locks in
> packet_bind_spkt(), packet_bind(), and sk_diag_fill().
>
> Note that WRITE_ONCE() is already added by commit c7d2ef5dd4b0
> ("net/packet: annotate accesses to po->bind").
>
> BUG: KCSAN: data-race in packet_bind / packet_do_bind
>
> write (marked) to 0xffff88802ffd1cee of 2 bytes by task 7322 on cpu 0:
>  packet_do_bind+0x446/0x640 net/packet/af_packet.c:3236
>  packet_bind+0x99/0xe0 net/packet/af_packet.c:3321
>  __sys_bind+0x19b/0x1e0 net/socket.c:1803
>  __do_sys_bind net/socket.c:1814 [inline]
>  __se_sys_bind net/socket.c:1812 [inline]
>  __x64_sys_bind+0x40/0x50 net/socket.c:1812
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x3b/0x90 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>
> read to 0xffff88802ffd1cee of 2 bytes by task 7318 on cpu 1:
>  packet_bind+0xbf/0xe0 net/packet/af_packet.c:3322
>  __sys_bind+0x19b/0x1e0 net/socket.c:1803
>  __do_sys_bind net/socket.c:1814 [inline]
>  __se_sys_bind net/socket.c:1812 [inline]
>  __x64_sys_bind+0x40/0x50 net/socket.c:1812
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x3b/0x90 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>
> value changed: 0x0300 -> 0x0000
>
> Reported by Kernel Concurrency Sanitizer on:
> CPU: 1 PID: 7318 Comm: syz-executor.4 Not tainted 6.3.0-13380-g7fddb5b5300c #4
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
>
> Fixes: 96ec6327144e ("packet: Diag core and basic socket info dumping")
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzkaller <syzkaller@googlegroups.com>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/packet/af_packet.c | 4 ++--
>  net/packet/diag.c      | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 94c6a1ffa459..a1f9a0e9f3c8 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -3299,7 +3299,7 @@ static int packet_bind_spkt(struct socket *sock, struct sockaddr *uaddr,
>         memcpy(name, uaddr->sa_data, sizeof(uaddr->sa_data_min));
>         name[sizeof(uaddr->sa_data_min)] = 0;
>
> -       return packet_do_bind(sk, name, 0, pkt_sk(sk)->num);
> +       return packet_do_bind(sk, name, 0, READ_ONCE(pkt_sk(sk)->num));
>  }
>
>  static int packet_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> @@ -3317,7 +3317,7 @@ static int packet_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len
>                 return -EINVAL;
>
>         return packet_do_bind(sk, NULL, sll->sll_ifindex,
> -                             sll->sll_protocol ? : pkt_sk(sk)->num);
> +                             sll->sll_protocol ? : READ_ONCE(pkt_sk(sk)->num));
>  }
>

Sorry for being late to the party.

Your change is still racy, and your two READ_ONCE()  hide the races.

READ_ONCE() makes sense for readers, not so much in bind() really,
this is quite confusing. (Like using rcu_read_lock() in writers is often
the sign of a misunderstanding of RCU principles)

I will send the following patch.

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index a1f9a0e9f3c8a72e5a95f96473b7b6c63f893935..a2dbeb264f260e5b8923ece9aac99fe19ddfeb62
100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -3201,6 +3201,9 @@ static int packet_do_bind(struct sock *sk, const
char *name, int ifindex,

        lock_sock(sk);
        spin_lock(&po->bind_lock);
+       if (!proto)
+               proto = po->num;
+
        rcu_read_lock();

        if (po->fanout) {
@@ -3299,7 +3302,7 @@ static int packet_bind_spkt(struct socket *sock,
struct sockaddr *uaddr,
        memcpy(name, uaddr->sa_data, sizeof(uaddr->sa_data_min));
        name[sizeof(uaddr->sa_data_min)] = 0;

-       return packet_do_bind(sk, name, 0, READ_ONCE(pkt_sk(sk)->num));
+       return packet_do_bind(sk, name, 0, 0);
 }

 static int packet_bind(struct socket *sock, struct sockaddr *uaddr,
int addr_len)
@@ -3316,8 +3319,7 @@ static int packet_bind(struct socket *sock,
struct sockaddr *uaddr, int addr_len
        if (sll->sll_family != AF_PACKET)
                return -EINVAL;

-       return packet_do_bind(sk, NULL, sll->sll_ifindex,
-                             sll->sll_protocol ? : READ_ONCE(pkt_sk(sk)->num));
+       return packet_do_bind(sk, NULL, sll->sll_ifindex, sll->sll_protocol);
 }

 static struct proto packet_proto = {