[syzbot] [net?] WARNING in __ip6_append_data

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 65d6e954e378 Merge tag 'gfs2-v6.5-rc5-fixes' of git://git...
git tree: upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=142177f4680000
kernel config: https://syzkaller.appspot.com/x/.config?x=b273cdfbc13e9a4b
dashboard link: https://syzkaller.appspot.com/bug?extid=62cbf263225ae13ff153
compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11f37a0c680000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=10034f3fa80000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/74df7181e630/disk-65d6e954.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/8455d5988dfe/vmlinux-65d6e954.xz
kernel image: https://storage.googleapis.com/syzbot-assets/8ee7b79f0dfd/bzImage-65d6e954.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+62cbf2...@syzkaller.appspotmail.com

------------[ cut here ]------------
WARNING: CPU: 1 PID: 5042 at net/ipv6/ip6_output.c:1800 __ip6_append_data.isra.0+0x1be8/0x47f0 net/ipv6/ip6_output.c:1800
Modules linked in:
CPU: 1 PID: 5042 Comm: syz-executor133 Not tainted 6.5.0-syzkaller-11938-g65d6e954e378 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/26/2023
RIP: 0010:__ip6_append_data.isra.0+0x1be8/0x47f0 net/ipv6/ip6_output.c:1800
Code: db f6 ff ff e8 09 d5 97 f8 49 8d 44 24 ff 48 89 44 24 60 49 8d 6c 24 07 e9 c2 f6 ff ff 4c 8b b4 24 90 01 00 00 e8 e8 d4 97 f8 <0f> 0b 48 8b 44 24 10 45 89 f4 48 8d 98 74 02 00 00 e8 d2 d4 97 f8
RSP: 0018:ffffc90003a1f3b8 EFLAGS: 00010293
RAX: 0000000000000000 RBX: 0000000000001004 RCX: 0000000000000000
RDX: ffff88801fe70000 RSI: ffffffff88efcf18 RDI: 0000000000000006
RBP: 0000000000001000 R08: 0000000000000006 R09: 0000000000001004
R10: 0000000000001000 R11: 0000000000000000 R12: 0000000000000001
R13: dffffc0000000000 R14: 0000000000001004 R15: ffff888019f31000
FS: 0000555557280380(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000045ad50 CR3: 0000000072666000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
ip6_append_data+0x1e6/0x510 net/ipv6/ip6_output.c:1895
l2tp_ip6_sendmsg+0xdf9/0x1cc0 net/l2tp/l2tp_ip6.c:631
inet_sendmsg+0x9d/0xe0 net/ipv4/af_inet.c:840
sock_sendmsg_nosec net/socket.c:730 [inline]
sock_sendmsg+0xd9/0x180 net/socket.c:753
splice_to_socket+0xade/0x1010 fs/splice.c:881
do_splice_from fs/splice.c:933 [inline]
direct_splice_actor+0x118/0x180 fs/splice.c:1142
splice_direct_to_actor+0x347/0xa30 fs/splice.c:1088
do_splice_direct+0x1af/0x280 fs/splice.c:1194
do_sendfile+0xb88/0x1390 fs/read_write.c:1254
__do_sys_sendfile64 fs/read_write.c:1322 [inline]
__se_sys_sendfile64 fs/read_write.c:1308 [inline]
__x64_sys_sendfile64+0x1d6/0x220 fs/read_write.c:1308
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f6b11150469
Code: 48 83 c4 28 c3 e8 37 17 00 00 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fffd14e71a8 EFLAGS: 00000246 ORIG_RAX: 0000000000000028
RAX: ffffffffffffffda RBX: 00007fffd14e7378 RCX: 00007f6b11150469
RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000005
RBP: 00007f6b111c3610 R08: 00007fffd14e7378 R09: 00007fffd14e7378
R10: 000000010000a006 R11: 0000000000000246 R12: 0000000000000001
R13: 00007fffd14e7368 R14: 0000000000000001 R15: 0000000000000001
</TASK>


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Eric Dumazet]

On Wed, Sep 13, 2023 at 8:19 AM syzbot
<syzbot+62cbf2...@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: 65d6e954e378 Merge tag 'gfs2-v6.5-rc5-fixes' of git://git...
> git tree: upstream
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=142177f4680000
> kernel config: https://syzkaller.appspot.com/x/.config?x=b273cdfbc13e9a4b
> dashboard link: https://syzkaller.appspot.com/bug?extid=62cbf263225ae13ff153
> compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11f37a0c680000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=10034f3fa80000
>

CC David

Warning added in

commit ce650a1663354a6cad7145e7f5131008458b39d4
Author: David Howells <dhow...@redhat.com>
Date: Wed Aug 2 08:36:50 2023 +0100

udp6: Fix __ip6_append_data()'s handling of MSG_SPLICE_PAGES

[David Howells]

Hi Eric,

> > WARNING: CPU: 1 PID: 5042 at net/ipv6/ip6_output.c:1800 __ip6_append_data.isra.0+0x1be8/0x47f0 net/ipv6/ip6_output.c:1800

That would appear to be this:

if (WARN_ON_ONCE(copy > msg->msg_iter.count))
goto error;

However, I have a problem that the repro program errors out at this point
before it gets that far:

if (cork->length + length > maxnonfragsize - headersize) {
emsgsize:
pmtu = max_t(int, mtu - headersize + sizeof(struct ipv6hdr), 0);
ipv6_local_error(sk, EMSGSIZE, fl6, pmtu);
return -EMSGSIZE;
}

Are you able to reproduce the issue?

The values in and around that point are:

cork->length 0
length 65540
maxnonfragsize 65575
headersize 40
transhdrlen 4
mtu 65536
ip6_sk_ignore_df(sk) true

with maxnonfragsize coming from 'sizeof(struct ipv6hdr) + IPV6_MAXPLEN'. Is
that even viable for the size of a packet?

David

[David Howells]

I think the attached is probably an equivalent cleaned up reproducer. Note
that if the length given to sendfile() is less than 65536, it fails with
EINVAL before it gets into __ip6_append_data().

David
---
#define _GNU_SOURCE
#include <arpa/inet.h>
#include <fcntl.h>
#include <stdarg.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/socket.h>
#include <sys/sendfile.h>
#include <sys/uio.h>
#include <netinet/ip6.h>
#include <netinet/in.h>

#define IPPROTO_L2TP 115

#define OSERROR(R, S) \
do { if ((long)(R) == -1L) { perror((S)); exit(1); } } while(0)

static char buffer[16776960];

int main()
{
struct sockaddr_in6 sin6;
int ffd, dfd, sfd;

ffd = openat(AT_FDCWD, "cgroup.controllers",
O_RDWR | O_CREAT | O_TRUNC, 0);
OSERROR(ffd, "cgroup.controllers/f");

OSERROR(write(ffd, buffer, sizeof(buffer)), "write");

dfd = openat(AT_FDCWD, "cgroup.controllers",
O_RDONLY | O_NONBLOCK | O_DIRECT);
OSERROR(dfd, "cgroup.controllers/d");

sfd = socket(AF_INET6, SOCK_DGRAM, IPPROTO_L2TP);
OSERROR(dfd, "socket");

memset(&sin6, 0, sizeof(sin6));
sin6.sin6_family = AF_INET6;
sin6.sin6_port = htons(0);
sin6.sin6_addr.s6_addr32[0] = htonl(0);
sin6.sin6_addr.s6_addr32[1] = htonl(0);
sin6.sin6_addr.s6_addr32[2] = htonl(0);
sin6.sin6_addr.s6_addr32[3] = htonl(1);
OSERROR(bind(sfd, (struct sockaddr *)&sin6, 32),
"bind");

memset(&sin6, 0, sizeof(sin6));
sin6.sin6_family = AF_INET6;
sin6.sin6_port = htons(7);
sin6.sin6_addr.s6_addr32[0] = htonl(0);
sin6.sin6_addr.s6_addr32[1] = htonl(0);
sin6.sin6_addr.s6_addr32[2] = htonl(0);
sin6.sin6_addr.s6_addr32[3] = htonl(1);
OSERROR(connect(sfd, (struct sockaddr *)&sin6, 32),
"connect");

OSERROR(sendfile(sfd, dfd, NULL, 65536), "sendfile");
return 0;
}

[David Howells]

David Howells <dhow...@redhat.com> wrote:

> I think the attached is probably an equivalent cleaned up reproducer. Note
> that if the length given to sendfile() is less than 65536, it fails with
> EINVAL before it gets into __ip6_append_data().

Actually, it only fails with EINVAL if the size is not a multiple of the block
size of the source file because it's open O_DIRECT so, say, 65536-512 is fine
(and works).

But thinking more on this further, is this even a bug in my code, I wonder?
The length passed is 65536 - but a UDP packet can't carry that, so it
shouldn't it have errored out before getting that far? (which is what it
seems to do when I try it).

I don't see how we get past the length check in ip6_append_data() with the
reproducer we're given unless the MTU is somewhat bigger than 65536 (is that
even possible?)

David

[David Howells]

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 65d6e954e378

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 0665e8b09968..7978335c1fc4 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1517,8 +1517,10 @@ static int __ip6_append_data(struct sock *sk,
rt->rt6i_nfheader_len;

if (mtu <= fragheaderlen ||
- ((mtu - fragheaderlen) & ~7) + fragheaderlen <= sizeof(struct frag_hdr))
+ ((mtu - fragheaderlen) & ~7) + fragheaderlen <= sizeof(struct frag_hdr)) {
+ printk("__%u__: EMSGSIZE\n", __LINE__);
goto emsgsize;
+ }

maxfraglen = ((mtu - fragheaderlen) & ~7) + fragheaderlen -
sizeof(struct frag_hdr);
@@ -1526,8 +1528,10 @@ static int __ip6_append_data(struct sock *sk,
/* as per RFC 7112 section 5, the entire IPv6 Header Chain must fit
* the first fragment
*/
- if (headersize + transhdrlen > mtu)
+ if (headersize + transhdrlen > mtu) {
+ printk("__%u__: EMSGSIZE\n", __LINE__);
goto emsgsize;
+ }

if (cork->length + length > mtu - headersize && ipc6->dontfrag &&
(sk->sk_protocol == IPPROTO_UDP ||
@@ -1535,15 +1539,23 @@ static int __ip6_append_data(struct sock *sk,
sk->sk_protocol == IPPROTO_RAW)) {
ipv6_local_rxpmtu(sk, fl6, mtu - headersize +
sizeof(struct ipv6hdr));
+ printk("__%u__: EMSGSIZE\n", __LINE__);
goto emsgsize;
}

- if (ip6_sk_ignore_df(sk))
+ if (ip6_sk_ignore_df(sk)) {
maxnonfragsize = sizeof(struct ipv6hdr) + IPV6_MAXPLEN;
- else
+ printk("MAXPLEN\n");
+ } else {
maxnonfragsize = mtu;
+ printk("mtu\n");
+ }

+ printk("check %d %zd %d %d, %d %d\n",
+ cork->length, length, maxnonfragsize, headersize,
+ transhdrlen, mtu);

if (cork->length + length > maxnonfragsize - headersize) {

+ printk("__%u__: EMSGSIZE\n", __LINE__);

emsgsize:
pmtu = max_t(int, mtu - headersize + sizeof(struct ipv6hdr), 0);
ipv6_local_error(sk, EMSGSIZE, fl6, pmtu);

@@ -1817,8 +1829,10 @@ static int __ip6_append_data(struct sock *sk,
if (!skb_can_coalesce(skb, i, pfrag->page,
pfrag->offset)) {
err = -EMSGSIZE;
- if (i == MAX_SKB_FRAGS)
+ if (i == MAX_SKB_FRAGS) {
+ printk("__%u__: EMSGSIZE\n", __LINE__);
goto error;
+ }

__skb_fill_page_desc(skb, i, pfrag->page,
pfrag->offset, 0);
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index ed8ebb6f5909..daaaf60dce01 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -502,6 +502,8 @@ static int l2tp_ip6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
int ulen;
int err;

+ printk("%s()\n", __func__);
+
/* Rough check on arithmetic overflow,
* better check is made in ip6_append_data().
*/

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
WARNING in __ip6_append_data

l2tp_ip6_sendmsg()
MAXPLEN
check 0 4100 65575 40, 4 65536
l2tp_ip6_sendmsg()
MAXPLEN
check 4100 4100 65575 40, 0 65536
------------[ cut here ]------------
WARNING: CPU: 0 PID: 5455 at net/ipv6/ip6_output.c:1812 __ip6_append_data.isra.0+0x1c6d/0x4900 net/ipv6/ip6_output.c:1812
Modules linked in:
CPU: 0 PID: 5455 Comm: syz-executor.0 Not tainted 6.5.0-syzkaller-11938-g65d6e954e378-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/04/2023
RIP: 0010:__ip6_append_data.isra.0+0x1c6d/0x4900 net/ipv6/ip6_output.c:1812
Code: c4 f6 ff ff e8 84 d4 97 f8 49 8d 44 24 ff 48 89 44 24 68 49 8d 6c 24 07 e9 ab f6 ff ff 4c 8b b4 24 90 01 00 00 e8 63 d4 97 f8 <0f> 0b 48 8b 44 24 10 45 89 f4 48 8d 98 74 02 00 00 e8 4d d4 97 f8
RSP: 0018:ffffc90004f373b8 EFLAGS: 00010293

RAX: 0000000000000000 RBX: 0000000000001004 RCX: 0000000000000000

RDX: ffff888019e8bb80 RSI: ffffffff88efcf9d RDI: 0000000000000006

RBP: 0000000000001000 R08: 0000000000000006 R09: 0000000000001004

R10: 0000000000001000 R11: 0000000000000001 R12: 0000000000000001
R13: dffffc0000000000 R14: 0000000000001004 R15: ffff888027b1d640
FS: 00007feae40ff6c0(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000

CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033

CR2: 00007f0f01e4e378 CR3: 000000007d467000 CR4: 00000000003506f0

DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>

ip6_append_data+0x1e6/0x510 net/ipv6/ip6_output.c:1909
l2tp_ip6_sendmsg+0xe0c/0x1ce0 net/l2tp/l2tp_ip6.c:633

inet_sendmsg+0x9d/0xe0 net/ipv4/af_inet.c:840
sock_sendmsg_nosec net/socket.c:730 [inline]
sock_sendmsg+0xd9/0x180 net/socket.c:753
splice_to_socket+0xade/0x1010 fs/splice.c:881
do_splice_from fs/splice.c:933 [inline]
direct_splice_actor+0x118/0x180 fs/splice.c:1142
splice_direct_to_actor+0x347/0xa30 fs/splice.c:1088
do_splice_direct+0x1af/0x280 fs/splice.c:1194
do_sendfile+0xb88/0x1390 fs/read_write.c:1254
__do_sys_sendfile64 fs/read_write.c:1322 [inline]
__se_sys_sendfile64 fs/read_write.c:1308 [inline]
__x64_sys_sendfile64+0x1d6/0x220 fs/read_write.c:1308
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

RIP: 0033:0x7feae347cae9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 e1 20 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007feae40ff0c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000028
RAX: ffffffffffffffda RBX: 00007feae359bf80 RCX: 00007feae347cae9

RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000005

RBP: 00007feae34c847a R08: 0000000000000000 R09: 0000000000000000
R10: 000000010000a006 R11: 0000000000000246 R12: 0000000000000000
R13: 000000000000000b R14: 00007feae359bf80 R15: 00007ffc444d03c8
</TASK>


Tested on:

commit: 65d6e954 Merge tag 'gfs2-v6.5-rc5-fixes' of git://git...
git tree: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=12133ac4680000

kernel config: https://syzkaller.appspot.com/x/.config?x=b273cdfbc13e9a4b
dashboard link: https://syzkaller.appspot.com/bug?extid=62cbf263225ae13ff153
compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=139cae54680000

[Willem de Bruijn]

An ipv6 packet can carry 64KB of payload, so maxnonfragsize of 65535 + 40
sounds correct. But payload length passed of 65536 is not (ignoring ipv6
jumbograms). So that should probably trigger an EINVAL -- if that is indeed
what the repro does.

[Eric Dumazet]

l2tp_ip6_sendmsg() claims ip6_append_data() can make better checks,
but what about simply replacing INT_MAX by 65535 ?

diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 44cfb72bbd18a34e83e50bebca09729c55df524f..ab57a134923bfc8040dba0d8fb702551ff265184

100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c

@@ -502,10 +502,7 @@ static int l2tp_ip6_sendmsg(struct sock *sk,

struct msghdr *msg, size_t len)
int ulen;
int err;

- /* Rough check on arithmetic overflow,
- * better check is made in ip6_append_data().
- */
- if (len > INT_MAX - transhdrlen)
+ if (len > 65535 - transhdrlen)
return -EMSGSIZE;
ulen = len + transhdrlen;

[David Howells]

Willem de Bruijn <willemdebr...@gmail.com> wrote:

>

> An ipv6 packet can carry 64KB of payload, so maxnonfragsize of 65535 + 40
> sounds correct. But payload length passed of 65536 is not (ignoring ipv6
> jumbograms). So that should probably trigger an EINVAL -- if that is indeed
> what the repro does.

The problem is that on entry to __ip6_append_data(), the length includes
transhdrlen. However, this is a problem if we already have something in the
packet. At that point, this fails:

if (WARN_ON_ONCE(copy > msg->msg_iter.count))
goto error;

because copy includes transhdrlen.

David

[Eric Dumazet]

On Tue, Sep 19, 2023 at 10:27 AM Tom Parkin <tpa...@katalix.com> wrote:

>
> On Mon, Sep 18, 2023 at 16:04:49 +0200, Eric Dumazet wrote:
> > On Mon, Sep 18, 2023 at 3:58 PM Willem de Bruijn
> > <willemdebr...@gmail.com> wrote:
> > >
> > > David Howells wrote:
> > > > David Howells <dhow...@redhat.com> wrote:
> > > >
> > > > > I think the attached is probably an equivalent cleaned up reproducer. Note
> > > > > that if the length given to sendfile() is less than 65536, it fails with
> > > > > EINVAL before it gets into __ip6_append_data().
> > > >
> > > > Actually, it only fails with EINVAL if the size is not a multiple of the block
> > > > size of the source file because it's open O_DIRECT so, say, 65536-512 is fine
> > > > (and works).
> > > >
> > > > But thinking more on this further, is this even a bug in my code, I wonder?
> > > > The length passed is 65536 - but a UDP packet can't carry that, so it
> > > > shouldn't it have errored out before getting that far? (which is what it
> > > > seems to do when I try it).
> > > >
> > > > I don't see how we get past the length check in ip6_append_data() with the
> > > > reproducer we're given unless the MTU is somewhat bigger than 65536 (is that
> > > > even possible?)
> > >
> > > An ipv6 packet can carry 64KB of payload, so maxnonfragsize of 65535 + 40
> > > sounds correct. But payload length passed of 65536 is not (ignoring ipv6
> > > jumbograms). So that should probably trigger an EINVAL -- if that is indeed
> > > what the repro does.
> >
> > l2tp_ip6_sendmsg() claims ip6_append_data() can make better checks,
> > but what about simply replacing INT_MAX by 65535 ?
>

> Slightly OT but I think the l2tp_ip6.c approach was probably cribbed
> from net/ipv6/udp.c's udpv6_sendmsg originally:

>
>
> /* Rough check on arithmetic overflow,

> better check is made in ip6_append_data().

> */
> if (len > INT_MAX - sizeof(struct udphdr))
> return -EMSGSIZE;
>
>
> Should the udp code be modified similarly?
>

Unfortunately both l2tp and udp support CORK (MSG_MORE),
so a modified check like that will not be enough to prevent syzbot reports.

Better than nothing, of course.

I also note that ipv4 size of l2tp does not have any check,
an overflow seems possible with carefully chosen size.

[Tom Parkin]

On Mon, Sep 18, 2023 at 16:04:49 +0200, Eric Dumazet wrote:

> On Mon, Sep 18, 2023 at 3:58 PM Willem de Bruijn
> <willemdebr...@gmail.com> wrote:
> >
> > David Howells wrote:
> > > David Howells <dhow...@redhat.com> wrote:
> > >
> > > > I think the attached is probably an equivalent cleaned up reproducer. Note
> > > > that if the length given to sendfile() is less than 65536, it fails with
> > > > EINVAL before it gets into __ip6_append_data().
> > >
> > > Actually, it only fails with EINVAL if the size is not a multiple of the block
> > > size of the source file because it's open O_DIRECT so, say, 65536-512 is fine
> > > (and works).
> > >
> > > But thinking more on this further, is this even a bug in my code, I wonder?
> > > The length passed is 65536 - but a UDP packet can't carry that, so it
> > > shouldn't it have errored out before getting that far? (which is what it
> > > seems to do when I try it).
> > >
> > > I don't see how we get past the length check in ip6_append_data() with the
> > > reproducer we're given unless the MTU is somewhat bigger than 65536 (is that
> > > even possible?)
> >
> > An ipv6 packet can carry 64KB of payload, so maxnonfragsize of 65535 + 40
> > sounds correct. But payload length passed of 65536 is not (ignoring ipv6
> > jumbograms). So that should probably trigger an EINVAL -- if that is indeed
> > what the repro does.
>
> l2tp_ip6_sendmsg() claims ip6_append_data() can make better checks,
> but what about simply replacing INT_MAX by 65535 ?

Slightly OT but I think the l2tp_ip6.c approach was probably cribbed
from net/ipv6/udp.c's udpv6_sendmsg originally:

/* Rough check on arithmetic overflow,

better check is made in ip6_append_data().

*/
if (len > INT_MAX - sizeof(struct udphdr))
return -EMSGSIZE;


Should the udp code be modified similarly?

--
Tom Parkin
Katalix Systems Ltd
https://katalix.com
Catalysts for your Embedded Linux software development