netdev - Re: [PATCH net-next] net: remove sk_is_ipmr() and sk_is

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]

Date: Tue, 20 Jun 2023 08:58:13 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, eric.dumazet@...il.com,
	syzbot <syzkaller@...glegroups.com>,
	Breno Leitao <leitao@...ian.org>,
	Willem de Bruijn <willemb@...gle.com>,
	David Ahern <dsahern@...nel.org>,
	Kuniyuki Iwashima <kuniyu@...zon.com>
Subject: Re: [PATCH net-next] net: remove sk_is_ipmr() and sk_is_icmpv6()
 helpers

Mon, Jun 19, 2023 at 07:31:35PM CEST, edumazet@...gle.com wrote:
>On Mon, Jun 19, 2023 at 5:18 PM Jiri Pirko <jiri@...nulli.us> wrote:
>>
>> Mon, Jun 19, 2023 at 02:43:35PM CEST, edumazet@...gle.com wrote:
>> >Blamed commit added these helpers for sake of detecting RAW
>> >sockets specific ioctl.
>> >
>> >syzbot complained about it [1].
>> >
>> >Issue here is that RAW sockets could pretend there was no need
>> >to call ipmr_sk_ioctl()
>> >
>> >Regardless of inet_sk(sk)->inet_num, we must be prepared
>> >for ipmr_ioctl() being called later. This must happen
>> >from ipmr_sk_ioctl() context only.
>> >
>> >We could add a safety check in ipmr_ioctl() at the risk of breaking
>> >applications.
>> >
>> >Instead, remove sk_is_ipmr() and sk_is_icmpv6() because their
>> >name would be misleading, once we change their implementation.
>>
>> Hurts my fingers to write this, but it would be easier to follow the
>> patch description and actually undestand what you do with imperative
>> mood commanding the codebase, without the "we"nesses. But again,
>> nevermind :)
>
>It is not the first time you comment on the style of my changelogs.

It is the first time. I did my best to not to do that in your case :)


>
>I would prefer we spend time elsewhere, we obviously have different
>ways of writing in English.

This is not about "ways of writing in English". This is about making
obvious to the reader what you do in the patch. So eventually it saves
time of the reader/reviewer and also submitter who does not need to
explain possible uncertainties.
Citing from Documentation/process/submitting-patches.rst:

"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
 instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
 to do frotz", as if you are giving orders to the codebase to change
 its behaviour."


>
>Just my two cents.
>
>
>
>>
>>
>> >
>> >[1]
>> >BUG: KASAN: stack-out-of-bounds in ipmr_ioctl+0xb12/0xbd0 net/ipv4/ipmr.c:1654
>> >Read of size 4 at addr ffffc90003aefae4 by task syz-executor105/5004
>> >
>> >CPU: 0 PID: 5004 Comm: syz-executor105 Not tainted 6.4.0-rc6-syzkaller-01304-gc08afcdcf952 #0
>> >Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/27/2023
>> >Call Trace:
>> ><TASK>
>> >__dump_stack lib/dump_stack.c:88 [inline]
>> >dump_stack_lvl+0xd9/0x150 lib/dump_stack.c:106
>> >print_address_description.constprop.0+0x2c/0x3c0 mm/kasan/report.c:351
>> >print_report mm/kasan/report.c:462 [inline]
>> >kasan_report+0x11c/0x130 mm/kasan/report.c:572
>> >ipmr_ioctl+0xb12/0xbd0 net/ipv4/ipmr.c:1654
>> >raw_ioctl+0x4e/0x1e0 net/ipv4/raw.c:881
>> >sock_ioctl_out net/core/sock.c:4186 [inline]
>> >sk_ioctl+0x151/0x440 net/core/sock.c:4214
>> >inet_ioctl+0x18c/0x380 net/ipv4/af_inet.c:1001
>> >sock_do_ioctl+0xcc/0x230 net/socket.c:1189
>> >sock_ioctl+0x1f8/0x680 net/socket.c:1306
>> >vfs_ioctl fs/ioctl.c:51 [inline]
>> >__do_sys_ioctl fs/ioctl.c:870 [inline]
>> >__se_sys_ioctl fs/ioctl.c:856 [inline]
>> >__x64_sys_ioctl+0x197/0x210 fs/ioctl.c:856
>> >do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>> >do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
>> >entry_SYSCALL_64_after_hwframe+0x63/0xcd
>> >RIP: 0033:0x7f2944bf6ad9
>> >Code: 28 c3 e8 2a 14 00 00 66 2e 0f 1f 84 00 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 c0 ff ff ff f7 d8 64 89 01 48
>> >RSP: 002b:00007ffd8897a028 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
>> >RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f2944bf6ad9
>> >RDX: 0000000000000000 RSI: 00000000000089e1 RDI: 0000000000000003
>> >RBP: 00007f2944bbac80 R08: 0000000000000000 R09: 0000000000000000
>> >R10: 0000000000000000 R11: 0000000000000246 R12: 00007f2944bbad10
>> >R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
>> ></TASK>
>> >
>> >The buggy address belongs to stack of task syz-executor105/5004
>> >and is located at offset 36 in frame:
>> >sk_ioctl+0x0/0x440 net/core/sock.c:4172
>> >
>> >This frame has 2 objects:
>> >[32, 36) 'karg'
>> >[48, 88) 'buffer'
>> >
>> >Fixes: e1d001fa5b47 ("net: ioctl: Use kernel memory on protocol ioctl callbacks")
>> >Reported-by: syzbot <syzkaller@...glegroups.com>
>> >Signed-off-by: Eric Dumazet <edumazet@...gle.com>
>>
>> Reviewed-by: Jiri Pirko <jiri@...dia.com>