[PATCH net] llc: make llc_ui_sendmsg() more robust against bonding changes

[PATCH net] llc: make llc_ui_sendmsg() more robust against bonding changes

[Eric Dumazet]

syzbot was able to trick llc_ui_sendmsg(), allocating an skb with no
headroom, but subsequently trying to push 14 bytes of Ethernet header [1]

Like some others, llc_ui_sendmsg() releases the socket lock before
calling sock_alloc_send_skb().
Then it acquires it again, but does not redo all the sanity checks
that were performed.

This fix:

- Uses LL_RESERVED_SPACE() to reserve space.
- Check all conditions again after socket lock is held again.
- Do not account Ethernet header for mtu limitation.

[1]

skbuff: skb_under_panic: text:ffff800088baa334 len:1514 put:14 head:ffff0000c9c37000 data:ffff0000c9c36ff2 tail:0x5dc end:0x6c0 dev:bond0

 kernel BUG at net/core/skbuff.c:193 !
Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 6875 Comm: syz-executor.0 Not tainted 6.7.0-rc8-syzkaller-00101-g0802e17d9aca-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/17/2023
pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : skb_panic net/core/skbuff.c:189 [inline]
 pc : skb_under_panic+0x13c/0x140 net/core/skbuff.c:203
 lr : skb_panic net/core/skbuff.c:189 [inline]
 lr : skb_under_panic+0x13c/0x140 net/core/skbuff.c:203
sp : ffff800096f97000
x29: ffff800096f97010 x28: ffff80008cc8d668 x27: dfff800000000000
x26: ffff0000cb970c90 x25: 00000000000005dc x24: ffff0000c9c36ff2
x23: ffff0000c9c37000 x22: 00000000000005ea x21: 00000000000006c0
x20: 000000000000000e x19: ffff800088baa334 x18: 1fffe000368261ce
x17: ffff80008e4ed000 x16: ffff80008a8310f8 x15: 0000000000000001
x14: 1ffff00012df2d58 x13: 0000000000000000 x12: 0000000000000000
x11: 0000000000000001 x10: 0000000000ff0100 x9 : e28a51f1087e8400
x8 : e28a51f1087e8400 x7 : ffff80008028f8d0 x6 : 0000000000000000
x5 : 0000000000000001 x4 : 0000000000000001 x3 : ffff800082b78714
x2 : 0000000000000001 x1 : 0000000100000000 x0 : 0000000000000089
Call trace:
  skb_panic net/core/skbuff.c:189 [inline]
  skb_under_panic+0x13c/0x140 net/core/skbuff.c:203
  skb_push+0xf0/0x108 net/core/skbuff.c:2451
  eth_header+0x44/0x1f8 net/ethernet/eth.c:83
  dev_hard_header include/linux/netdevice.h:3188 [inline]
  llc_mac_hdr_init+0x110/0x17c net/llc/llc_output.c:33
  llc_sap_action_send_xid_c+0x170/0x344 net/llc/llc_s_ac.c:85
  llc_exec_sap_trans_actions net/llc/llc_sap.c:153 [inline]
  llc_sap_next_state net/llc/llc_sap.c:182 [inline]
  llc_sap_state_process+0x1ec/0x774 net/llc/llc_sap.c:209
  llc_build_and_send_xid_pkt+0x12c/0x1c0 net/llc/llc_sap.c:270
  llc_ui_sendmsg+0x7bc/0xb1c net/llc/af_llc.c:997
  sock_sendmsg_nosec net/socket.c:730 [inline]
  __sock_sendmsg net/socket.c:745 [inline]
  sock_sendmsg+0x194/0x274 net/socket.c:767
  splice_to_socket+0x7cc/0xd58 fs/splice.c:881
  do_splice_from fs/splice.c:933 [inline]
  direct_splice_actor+0xe4/0x1c0 fs/splice.c:1142
  splice_direct_to_actor+0x2a0/0x7e4 fs/splice.c:1088
  do_splice_direct+0x20c/0x348 fs/splice.c:1194
  do_sendfile+0x4bc/0xc70 fs/read_write.c:1254
  __do_sys_sendfile64 fs/read_write.c:1322 [inline]
  __se_sys_sendfile64 fs/read_write.c:1308 [inline]
  __arm64_sys_sendfile64+0x160/0x3b4 fs/read_write.c:1308
  __invoke_syscall arch/arm64/kernel/syscall.c:37 [inline]
  invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:51
  el0_svc_common+0x130/0x23c arch/arm64/kernel/syscall.c:136
  do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:155
  el0_svc+0x54/0x158 arch/arm64/kernel/entry-common.c:678
  el0t_64_sync_handler+0x84/0xfc arch/arm64/kernel/entry-common.c:696
  el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:595
Code: aa1803e6 aa1903e7 a90023f5 94792f6a (d4210000)

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-and-tested-by: syzbot+2a7024e9502df538e8ef@syzkaller.appspotmail.com
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/llc/af_llc.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
index 9b06c380866b53bcb395bf255587279db025d11d..20551cfb7da6d8dd098c906477895e26c080fe32 100644
--- a/net/llc/af_llc.c
+++ b/net/llc/af_llc.c
@@ -928,14 +928,15 @@ static int llc_ui_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
  */
 static int llc_ui_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 {
+	DECLARE_SOCKADDR(struct sockaddr_llc *, addr, msg->msg_name);
 	struct sock *sk = sock->sk;
 	struct llc_sock *llc = llc_sk(sk);
-	DECLARE_SOCKADDR(struct sockaddr_llc *, addr, msg->msg_name);
 	int flags = msg->msg_flags;
 	int noblock = flags & MSG_DONTWAIT;
+	int rc = -EINVAL, copied = 0, hdrlen, hh_len;
 	struct sk_buff *skb = NULL;
+	struct net_device *dev;
 	size_t size = 0;
-	int rc = -EINVAL, copied = 0, hdrlen;
 
 	dprintk("%s: sending from %02X to %02X\n", __func__,
 		llc->laddr.lsap, llc->daddr.lsap);
@@ -955,22 +956,29 @@ static int llc_ui_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 		if (rc)
 			goto out;
 	}
-	hdrlen = llc->dev->hard_header_len + llc_ui_header_len(sk, addr);
+	dev = llc->dev;
+	hh_len = LL_RESERVED_SPACE(dev);
+	hdrlen = llc_ui_header_len(sk, addr);
 	size = hdrlen + len;
-	if (size > llc->dev->mtu)
-		size = llc->dev->mtu;
+	size = min_t(size_t, size, READ_ONCE(dev->mtu));
 	copied = size - hdrlen;
 	rc = -EINVAL;
 	if (copied < 0)
 		goto out;
 	release_sock(sk);
-	skb = sock_alloc_send_skb(sk, size, noblock, &rc);
+	skb = sock_alloc_send_skb(sk, hh_len + size, noblock, &rc);
 	lock_sock(sk);
 	if (!skb)
 		goto out;
-	skb->dev      = llc->dev;
+	if (sock_flag(sk, SOCK_ZAPPED) ||
+	    llc->dev != dev ||
+	    hdrlen != llc_ui_header_len(sk, addr) ||
+	    hh_len != LL_RESERVED_SPACE(dev) ||
+	    size > READ_ONCE(dev->mtu))
+		goto out;
+	skb->dev      = dev;
 	skb->protocol = llc_proto_type(addr->sllc_arphrd);
-	skb_reserve(skb, hdrlen);
+	skb_reserve(skb, hh_len + hdrlen);
 	rc = memcpy_from_msg(skb_put(skb, copied), msg, copied);
 	if (rc)
 		goto out;
-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH net] llc: make llc_ui_sendmsg() more robust against bonding changes

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@google.com>
Date: Thu, 18 Jan 2024 18:36:25 +0000
> syzbot was able to trick llc_ui_sendmsg(), allocating an skb with no
> headroom, but subsequently trying to push 14 bytes of Ethernet header [1]
> 
> Like some others, llc_ui_sendmsg() releases the socket lock before
> calling sock_alloc_send_skb().
> Then it acquires it again, but does not redo all the sanity checks
> that were performed.
> 
> This fix:
> 
> - Uses LL_RESERVED_SPACE() to reserve space.
> - Check all conditions again after socket lock is held again.
> - Do not account Ethernet header for mtu limitation.
> 
> [1]
> 
> skbuff: skb_under_panic: text:ffff800088baa334 len:1514 put:14 head:ffff0000c9c37000 data:ffff0000c9c36ff2 tail:0x5dc end:0x6c0 dev:bond0
> 
>  kernel BUG at net/core/skbuff.c:193 !
> Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 0 PID: 6875 Comm: syz-executor.0 Not tainted 6.7.0-rc8-syzkaller-00101-g0802e17d9aca-dirty #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/17/2023
> pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>  pc : skb_panic net/core/skbuff.c:189 [inline]
>  pc : skb_under_panic+0x13c/0x140 net/core/skbuff.c:203
>  lr : skb_panic net/core/skbuff.c:189 [inline]
>  lr : skb_under_panic+0x13c/0x140 net/core/skbuff.c:203
> sp : ffff800096f97000
> x29: ffff800096f97010 x28: ffff80008cc8d668 x27: dfff800000000000
> x26: ffff0000cb970c90 x25: 00000000000005dc x24: ffff0000c9c36ff2
> x23: ffff0000c9c37000 x22: 00000000000005ea x21: 00000000000006c0
> x20: 000000000000000e x19: ffff800088baa334 x18: 1fffe000368261ce
> x17: ffff80008e4ed000 x16: ffff80008a8310f8 x15: 0000000000000001
> x14: 1ffff00012df2d58 x13: 0000000000000000 x12: 0000000000000000
> x11: 0000000000000001 x10: 0000000000ff0100 x9 : e28a51f1087e8400
> x8 : e28a51f1087e8400 x7 : ffff80008028f8d0 x6 : 0000000000000000
> x5 : 0000000000000001 x4 : 0000000000000001 x3 : ffff800082b78714
> x2 : 0000000000000001 x1 : 0000000100000000 x0 : 0000000000000089
> Call trace:
>   skb_panic net/core/skbuff.c:189 [inline]
>   skb_under_panic+0x13c/0x140 net/core/skbuff.c:203
>   skb_push+0xf0/0x108 net/core/skbuff.c:2451
>   eth_header+0x44/0x1f8 net/ethernet/eth.c:83
>   dev_hard_header include/linux/netdevice.h:3188 [inline]
>   llc_mac_hdr_init+0x110/0x17c net/llc/llc_output.c:33
>   llc_sap_action_send_xid_c+0x170/0x344 net/llc/llc_s_ac.c:85
>   llc_exec_sap_trans_actions net/llc/llc_sap.c:153 [inline]
>   llc_sap_next_state net/llc/llc_sap.c:182 [inline]
>   llc_sap_state_process+0x1ec/0x774 net/llc/llc_sap.c:209
>   llc_build_and_send_xid_pkt+0x12c/0x1c0 net/llc/llc_sap.c:270
>   llc_ui_sendmsg+0x7bc/0xb1c net/llc/af_llc.c:997
>   sock_sendmsg_nosec net/socket.c:730 [inline]
>   __sock_sendmsg net/socket.c:745 [inline]
>   sock_sendmsg+0x194/0x274 net/socket.c:767
>   splice_to_socket+0x7cc/0xd58 fs/splice.c:881
>   do_splice_from fs/splice.c:933 [inline]
>   direct_splice_actor+0xe4/0x1c0 fs/splice.c:1142
>   splice_direct_to_actor+0x2a0/0x7e4 fs/splice.c:1088
>   do_splice_direct+0x20c/0x348 fs/splice.c:1194
>   do_sendfile+0x4bc/0xc70 fs/read_write.c:1254
>   __do_sys_sendfile64 fs/read_write.c:1322 [inline]
>   __se_sys_sendfile64 fs/read_write.c:1308 [inline]
>   __arm64_sys_sendfile64+0x160/0x3b4 fs/read_write.c:1308
>   __invoke_syscall arch/arm64/kernel/syscall.c:37 [inline]
>   invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:51
>   el0_svc_common+0x130/0x23c arch/arm64/kernel/syscall.c:136
>   do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:155
>   el0_svc+0x54/0x158 arch/arm64/kernel/entry-common.c:678
>   el0t_64_sync_handler+0x84/0xfc arch/arm64/kernel/entry-common.c:696
>   el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:595
> Code: aa1803e6 aa1903e7 a90023f5 94792f6a (d4210000)
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-and-tested-by: syzbot+2a7024e9502df538e8ef@syzkaller.appspotmail.com
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/llc/af_llc.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
> index 9b06c380866b53bcb395bf255587279db025d11d..20551cfb7da6d8dd098c906477895e26c080fe32 100644
> --- a/net/llc/af_llc.c
> +++ b/net/llc/af_llc.c
> @@ -928,14 +928,15 @@ static int llc_ui_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>   */
>  static int llc_ui_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
>  {
> +	DECLARE_SOCKADDR(struct sockaddr_llc *, addr, msg->msg_name);
>  	struct sock *sk = sock->sk;
>  	struct llc_sock *llc = llc_sk(sk);
> -	DECLARE_SOCKADDR(struct sockaddr_llc *, addr, msg->msg_name);
>  	int flags = msg->msg_flags;
>  	int noblock = flags & MSG_DONTWAIT;
> +	int rc = -EINVAL, copied = 0, hdrlen, hh_len;
>  	struct sk_buff *skb = NULL;
> +	struct net_device *dev;
>  	size_t size = 0;
> -	int rc = -EINVAL, copied = 0, hdrlen;
>  
>  	dprintk("%s: sending from %02X to %02X\n", __func__,
>  		llc->laddr.lsap, llc->daddr.lsap);
> @@ -955,22 +956,29 @@ static int llc_ui_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
>  		if (rc)
>  			goto out;
>  	}
> -	hdrlen = llc->dev->hard_header_len + llc_ui_header_len(sk, addr);
> +	dev = llc->dev;
> +	hh_len = LL_RESERVED_SPACE(dev);
> +	hdrlen = llc_ui_header_len(sk, addr);
>  	size = hdrlen + len;
> -	if (size > llc->dev->mtu)
> -		size = llc->dev->mtu;
> +	size = min_t(size_t, size, READ_ONCE(dev->mtu));
>  	copied = size - hdrlen;
>  	rc = -EINVAL;
>  	if (copied < 0)
>  		goto out;
>  	release_sock(sk);
> -	skb = sock_alloc_send_skb(sk, size, noblock, &rc);
> +	skb = sock_alloc_send_skb(sk, hh_len + size, noblock, &rc);
>  	lock_sock(sk);
>  	if (!skb)
>  		goto out;
> -	skb->dev      = llc->dev;
> +	if (sock_flag(sk, SOCK_ZAPPED) ||

Probably we need not check SOCK_ZAPPED again after llc_ui_autobind() ?


> +	    llc->dev != dev ||
> +	    hdrlen != llc_ui_header_len(sk, addr) ||
> +	    hh_len != LL_RESERVED_SPACE(dev) ||
> +	    size > READ_ONCE(dev->mtu))
> +		goto out;
> +	skb->dev      = dev;
>  	skb->protocol = llc_proto_type(addr->sllc_arphrd);
> -	skb_reserve(skb, hdrlen);
> +	skb_reserve(skb, hh_len + hdrlen);
>  	rc = memcpy_from_msg(skb_put(skb, copied), msg, copied);
>  	if (rc)
>  		goto out;
> -- 
> 2.43.0.429.g432eaa2c6b-goog

Re: [PATCH net] llc: make llc_ui_sendmsg() more robust against bonding changes

[Eric Dumazet]

On Thu, Jan 18, 2024 at 10:59 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>

> Probably we need not check SOCK_ZAPPED again after llc_ui_autobind() ?
>

Possibly, I was not sure if the socket could be disconnected or not.

This would be a nop, or a correct check if disconnect is implemented.

Do you see a problem with a strict validation ?

I am tired of syzbot reports about llc, I want to add every possible checks.

Re: [PATCH net] llc: make llc_ui_sendmsg() more robust against bonding changes

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@google.com>
Date: Thu, 18 Jan 2024 23:14:21 +0100
> On Thu, Jan 18, 2024 at 10:59 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> 
> > Probably we need not check SOCK_ZAPPED again after llc_ui_autobind() ?
> >
> 
> Possibly, I was not sure if the socket could be disconnected or not.
> 
> This would be a nop, or a correct check if disconnect is implemented.
> 
> Do you see a problem with a strict validation ?
> 
> I am tired of syzbot reports about llc, I want to add every possible checks.

Agreed, no problem.

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Re: [PATCH net] llc: make llc_ui_sendmsg() more robust against bonding changes

[patchwork-bot+netdevbpf]

Hello:

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

On Thu, 18 Jan 2024 18:36:25 +0000 you wrote:
> syzbot was able to trick llc_ui_sendmsg(), allocating an skb with no
> headroom, but subsequently trying to push 14 bytes of Ethernet header [1]
> 
> Like some others, llc_ui_sendmsg() releases the socket lock before
> calling sock_alloc_send_skb().
> Then it acquires it again, but does not redo all the sanity checks
> that were performed.
> 
> [...]

Here is the summary with links:
  - [net] llc: make llc_ui_sendmsg() more robust against bonding changes
    https://git.kernel.org/netdev/net/c/dad555c816a5

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