[PATCH v1 net-next] atm: atmtcp: Free invalid length skb in atmtcp_c_send().

[PATCH v1 net-next] atm: atmtcp: Free invalid length skb in atmtcp_c_send().

[Kuniyuki Iwashima]

From: Kuniyuki Iwashima <kuniyu@google.com>

syzbot reported the splat below. [0]

vcc_sendmsg() copies data passed from userspace to skb and passes
it to vcc->dev->ops->send().

atmtcp_c_send() accesses skb->data as struct atmtcp_hdr after
checking if skb->len is 0, but it's not enough.

Also, when skb->len == 0, skb and sk (vcc) were leaked because
dev_kfree_skb() is not called and atm_return() is missing to
revert atm_account_tx() in vcc_sendmsg().

Let's properly free skb with an invalid length in atmtcp_c_send().

[0]:
BUG: KMSAN: uninit-value in atmtcp_c_send+0x255/0xed0 drivers/atm/atmtcp.c:294
 atmtcp_c_send+0x255/0xed0 drivers/atm/atmtcp.c:294
 vcc_sendmsg+0xd7c/0xff0 net/atm/common.c:644
 sock_sendmsg_nosec net/socket.c:712 [inline]
 __sock_sendmsg+0x330/0x3d0 net/socket.c:727
 ____sys_sendmsg+0x7e0/0xd80 net/socket.c:2566
 ___sys_sendmsg+0x271/0x3b0 net/socket.c:2620
 __sys_sendmsg net/socket.c:2652 [inline]
 __do_sys_sendmsg net/socket.c:2657 [inline]
 __se_sys_sendmsg net/socket.c:2655 [inline]
 __x64_sys_sendmsg+0x211/0x3e0 net/socket.c:2655
 x64_sys_call+0x32fb/0x3db0 arch/x86/include/generated/asm/syscalls_64.h:47
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0xd9/0x210 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Uninit was created at:
 slab_post_alloc_hook mm/slub.c:4154 [inline]
 slab_alloc_node mm/slub.c:4197 [inline]
 kmem_cache_alloc_node_noprof+0x818/0xf00 mm/slub.c:4249
 kmalloc_reserve+0x13c/0x4b0 net/core/skbuff.c:579
 __alloc_skb+0x347/0x7d0 net/core/skbuff.c:670
 alloc_skb include/linux/skbuff.h:1336 [inline]
 vcc_sendmsg+0xb40/0xff0 net/atm/common.c:628
 sock_sendmsg_nosec net/socket.c:712 [inline]
 __sock_sendmsg+0x330/0x3d0 net/socket.c:727
 ____sys_sendmsg+0x7e0/0xd80 net/socket.c:2566
 ___sys_sendmsg+0x271/0x3b0 net/socket.c:2620
 __sys_sendmsg net/socket.c:2652 [inline]
 __do_sys_sendmsg net/socket.c:2657 [inline]
 __se_sys_sendmsg net/socket.c:2655 [inline]
 __x64_sys_sendmsg+0x211/0x3e0 net/socket.c:2655
 x64_sys_call+0x32fb/0x3db0 arch/x86/include/generated/asm/syscalls_64.h:47
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0xd9/0x210 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

CPU: 1 UID: 0 PID: 5798 Comm: syz-executor192 Not tainted 6.16.0-rc1-syzkaller-00010-g2c4a1f3fe03e #0 PREEMPT(undef)
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/07/2025

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+1d3c235276f62963e93a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1d3c235276f62963e93a
Tested-by: syzbot+1d3c235276f62963e93a@syzkaller.appspotmail.com
Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
---
 drivers/atm/atmtcp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/atm/atmtcp.c b/drivers/atm/atmtcp.c
index d4aa0f353b6c..54c6aeac63d0 100644
--- a/drivers/atm/atmtcp.c
+++ b/drivers/atm/atmtcp.c
@@ -288,7 +288,12 @@ static int atmtcp_c_send(struct atm_vcc *vcc,struct sk_buff *skb)
 	struct sk_buff *new_skb;
 	int result = 0;
 
-	if (!skb->len) return 0;
+	if (skb->len < sizeof(struct atmtcp_hdr)) {
+		atm_return(vcc, skb->truesize);
+		dev_kfree_skb(skb);
+		return -EINVAL;
+	}
+
 	dev = vcc->dev_data;
 	hdr = (struct atmtcp_hdr *) skb->data;
 	if (hdr->length == ATMTCP_HDR_MAGIC) {
-- 
2.49.0

Re: [PATCH v1 net-next] atm: atmtcp: Free invalid length skb in atmtcp_c_send().

[Simon Horman]

On Thu, Jun 12, 2025 at 10:56:55PM -0700, Kuniyuki Iwashima wrote:
> From: Kuniyuki Iwashima <kuniyu@google.com>
> 
> syzbot reported the splat below. [0]
> 
> vcc_sendmsg() copies data passed from userspace to skb and passes
> it to vcc->dev->ops->send().
> 
> atmtcp_c_send() accesses skb->data as struct atmtcp_hdr after
> checking if skb->len is 0, but it's not enough.
> 
> Also, when skb->len == 0, skb and sk (vcc) were leaked because
> dev_kfree_skb() is not called and atm_return() is missing to
> revert atm_account_tx() in vcc_sendmsg().

Hi Iwashima-san,

I agree with the above and your patch.
But I am wondering if atm_return() also needs to be called when:

* atmtcp_c_send returns -ENOBUFS because atm_alloc_charge() fails.
* copy_from_iter_full returns false in vcc_sendmsg.

I ask because both occur after the call to atm_account_tx() in vcc_sendmsg().

> 
> Let's properly free skb with an invalid length in atmtcp_c_send().

...

> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+1d3c235276f62963e93a@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=1d3c235276f62963e93a
> Tested-by: syzbot+1d3c235276f62963e93a@syzkaller.appspotmail.com
> Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>

My question above not withstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...

Re: [PATCH v1 net-next] atm: atmtcp: Free invalid length skb in atmtcp_c_send().

[Kuniyuki Iwashima]

From: Simon Horman <horms@kernel.org>
Date: Sat, 14 Jun 2025 17:19:59 +0100
> On Thu, Jun 12, 2025 at 10:56:55PM -0700, Kuniyuki Iwashima wrote:
> > From: Kuniyuki Iwashima <kuniyu@google.com>
> > 
> > syzbot reported the splat below. [0]
> > 
> > vcc_sendmsg() copies data passed from userspace to skb and passes
> > it to vcc->dev->ops->send().
> > 
> > atmtcp_c_send() accesses skb->data as struct atmtcp_hdr after
> > checking if skb->len is 0, but it's not enough.
> > 
> > Also, when skb->len == 0, skb and sk (vcc) were leaked because
> > dev_kfree_skb() is not called and atm_return() is missing to
> > revert atm_account_tx() in vcc_sendmsg().
> 
> Hi Iwashima-san,
> 
> I agree with the above and your patch.
> But I am wondering if atm_return() also needs to be called when:

Oh, I noticed atm_return() was for rmem_alloc, so I had to adjust
wmem_alloc manually here.


> 
> * atmtcp_c_send returns -ENOBUFS because atm_alloc_charge() fails.

In this case, I guess vcc->pop is atm_pop_raw() that handles wmem
accounting.

        if (vcc->pop) vcc->pop(vcc,skb);
        else dev_kfree_skb(skb);

If it's NULL, we need to adjust it here, but this pattern can be
seen other ATM drivers..

Okay, sendmsg() requires SS_CONNECTED, and __vcc_connect() must go
through one of atm_init_aal{0,34,5}, which sets ->pop to atm_pop_raw().

So, it seems !vcc->pop() is defensive unlikely path.

In v2, I'll change the invalid length handling to goto done;


> * copy_from_iter_full returns false in vcc_sendmsg.

I agree.  I can send a followup.

Thank you!

pw-bot: cr