[PATCH net-next] net: remove sk_is_ipmr() and sk_is_icmpv6() helpers

[PATCH net-next] net: remove sk_is_ipmr() and sk_is_icmpv6() helpers

[Eric Dumazet]

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.

[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@googlegroups.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Breno Leitao <leitao@debian.org>
Cc: Willem de Bruijn <willemb@google.com>
Cc: David Ahern <dsahern@kernel.org>
Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/linux/icmpv6.h |  6 ------
 include/linux/mroute.h | 11 -----------
 net/core/sock.c        |  4 ++--
 3 files changed, 2 insertions(+), 19 deletions(-)

diff --git a/include/linux/icmpv6.h b/include/linux/icmpv6.h
index 1fe33e6741cca2685082da214afbc94c7974f4ce..db0f4fcfdaf4f138d75dc6c4073cb286364f2923 100644
--- a/include/linux/icmpv6.h
+++ b/include/linux/icmpv6.h
@@ -111,10 +111,4 @@ static inline bool icmpv6_is_err(int type)
 	return false;
 }
 
-static inline int sk_is_icmpv6(struct sock *sk)
-{
-	return sk->sk_family == AF_INET6 &&
-		inet_sk(sk)->inet_num == IPPROTO_ICMPV6;
-}
-
 #endif
diff --git a/include/linux/mroute.h b/include/linux/mroute.h
index 94c6e6f549f0a503f6707d1175f1c47a0f0cd887..4c5003afee6c51962bc13879978845bc7daf08fa 100644
--- a/include/linux/mroute.h
+++ b/include/linux/mroute.h
@@ -16,12 +16,6 @@ static inline int ip_mroute_opt(int opt)
 	return opt >= MRT_BASE && opt <= MRT_MAX;
 }
 
-static inline int sk_is_ipmr(struct sock *sk)
-{
-	return sk->sk_family == AF_INET &&
-		inet_sk(sk)->inet_num == IPPROTO_IGMP;
-}
-
 int ip_mroute_setsockopt(struct sock *, int, sockptr_t, unsigned int);
 int ip_mroute_getsockopt(struct sock *, int, sockptr_t, sockptr_t);
 int ipmr_ioctl(struct sock *sk, int cmd, void *arg);
@@ -57,11 +51,6 @@ static inline int ip_mroute_opt(int opt)
 	return 0;
 }
 
-static inline int sk_is_ipmr(struct sock *sk)
-{
-	return 0;
-}
-
 static inline bool ipmr_rule_default(const struct fib_rule *rule)
 {
 	return true;
diff --git a/net/core/sock.c b/net/core/sock.c
index cff3e82514d17513bc96300dc18014083a0d7ea7..8ec8f4c9911f2a6dd3dd946798d512394d62a861 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -4199,9 +4199,9 @@ int sk_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
 {
 	int rc = 1;
 
-	if (sk_is_ipmr(sk))
+	if (sk->sk_type == SOCK_RAW && sk->sk_family == AF_INET)
 		rc = ipmr_sk_ioctl(sk, cmd, arg);
-	else if (sk_is_icmpv6(sk))
+	else if (sk->sk_type == SOCK_RAW && sk->sk_family == AF_INET6)
 		rc = ip6mr_sk_ioctl(sk, cmd, arg);
 	else if (sk_is_phonet(sk))
 		rc = phonet_sk_ioctl(sk, cmd, arg);
-- 
2.41.0.185.g7c58973941-goog

Re: [PATCH net-next] net: remove sk_is_ipmr() and sk_is_icmpv6() helpers

[Jiri Pirko]

Mon, Jun 19, 2023 at 02:43:35PM CEST, edumazet@google.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 :)


>
>[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@googlegroups.com>
>Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH net-next] net: remove sk_is_ipmr() and sk_is_icmpv6() helpers

[Eric Dumazet]

On Mon, Jun 19, 2023 at 5:18 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Mon, Jun 19, 2023 at 02:43:35PM CEST, edumazet@google.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.

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

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@googlegroups.com>
> >Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH net-next] net: remove sk_is_ipmr() and sk_is_icmpv6() helpers

[David Ahern]

On 6/19/23 5:43 AM, Eric Dumazet 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.
> 
> [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@googlegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Breno Leitao <leitao@debian.org>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: David Ahern <dsahern@kernel.org>
> Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  include/linux/icmpv6.h |  6 ------
>  include/linux/mroute.h | 11 -----------
>  net/core/sock.c        |  4 ++--
>  3 files changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/icmpv6.h b/include/linux/icmpv6.h
> index 1fe33e6741cca2685082da214afbc94c7974f4ce..db0f4fcfdaf4f138d75dc6c4073cb286364f2923 100644
> --- a/include/linux/icmpv6.h
> +++ b/include/linux/icmpv6.h
> @@ -111,10 +111,4 @@ static inline bool icmpv6_is_err(int type)
>  	return false;
>  }
>  
> -static inline int sk_is_icmpv6(struct sock *sk)
> -{
> -	return sk->sk_family == AF_INET6 &&
> -		inet_sk(sk)->inet_num == IPPROTO_ICMPV6;
> -}
> -
>  #endif
> diff --git a/include/linux/mroute.h b/include/linux/mroute.h
> index 94c6e6f549f0a503f6707d1175f1c47a0f0cd887..4c5003afee6c51962bc13879978845bc7daf08fa 100644
> --- a/include/linux/mroute.h
> +++ b/include/linux/mroute.h
> @@ -16,12 +16,6 @@ static inline int ip_mroute_opt(int opt)
>  	return opt >= MRT_BASE && opt <= MRT_MAX;
>  }
>  
> -static inline int sk_is_ipmr(struct sock *sk)
> -{
> -	return sk->sk_family == AF_INET &&
> -		inet_sk(sk)->inet_num == IPPROTO_IGMP;
> -}
> -
>  int ip_mroute_setsockopt(struct sock *, int, sockptr_t, unsigned int);
>  int ip_mroute_getsockopt(struct sock *, int, sockptr_t, sockptr_t);
>  int ipmr_ioctl(struct sock *sk, int cmd, void *arg);
> @@ -57,11 +51,6 @@ static inline int ip_mroute_opt(int opt)
>  	return 0;
>  }
>  
> -static inline int sk_is_ipmr(struct sock *sk)
> -{
> -	return 0;
> -}
> -
>  static inline bool ipmr_rule_default(const struct fib_rule *rule)
>  {
>  	return true;
> diff --git a/net/core/sock.c b/net/core/sock.c
> index cff3e82514d17513bc96300dc18014083a0d7ea7..8ec8f4c9911f2a6dd3dd946798d512394d62a861 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -4199,9 +4199,9 @@ int sk_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>  {
>  	int rc = 1;
>  
> -	if (sk_is_ipmr(sk))
> +	if (sk->sk_type == SOCK_RAW && sk->sk_family == AF_INET)
>  		rc = ipmr_sk_ioctl(sk, cmd, arg);
> -	else if (sk_is_icmpv6(sk))
> +	else if (sk->sk_type == SOCK_RAW && sk->sk_family == AF_INET6)
>  		rc = ip6mr_sk_ioctl(sk, cmd, arg);
>  	else if (sk_is_phonet(sk))
>  		rc = phonet_sk_ioctl(sk, cmd, arg);

I was staring at the type when reviewing the original patch; I was
expecting to see RAW based on where ipmr_ioctl (and v6 version) is
called. That's why I asked about the testing of the patch, so I am not
surprised by this followup.

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next] net: remove sk_is_ipmr() and sk_is_icmpv6() helpers

[Jiri Pirko]

Mon, Jun 19, 2023 at 07:31:35PM CEST, edumazet@google.com wrote:
>On Mon, Jun 19, 2023 at 5:18 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Mon, Jun 19, 2023 at 02:43:35PM CEST, edumazet@google.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@googlegroups.com>
>> >Signed-off-by: Eric Dumazet <edumazet@google.com>
>>
>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH net-next] net: remove sk_is_ipmr() and sk_is_icmpv6() helpers

[Willem de Bruijn]

David Ahern wrote:
> On 6/19/23 5:43 AM, Eric Dumazet 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.
> > 
> > [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@googlegroups.com>
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Breno Leitao <leitao@debian.org>
> > Cc: Willem de Bruijn <willemb@google.com>
> > Cc: David Ahern <dsahern@kernel.org>
> > Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> >  include/linux/icmpv6.h |  6 ------
> >  include/linux/mroute.h | 11 -----------
> >  net/core/sock.c        |  4 ++--
> >  3 files changed, 2 insertions(+), 19 deletions(-)
> > 
> > diff --git a/include/linux/icmpv6.h b/include/linux/icmpv6.h
> > index 1fe33e6741cca2685082da214afbc94c7974f4ce..db0f4fcfdaf4f138d75dc6c4073cb286364f2923 100644
> > --- a/include/linux/icmpv6.h
> > +++ b/include/linux/icmpv6.h
> > @@ -111,10 +111,4 @@ static inline bool icmpv6_is_err(int type)
> >  	return false;
> >  }
> >  
> > -static inline int sk_is_icmpv6(struct sock *sk)
> > -{
> > -	return sk->sk_family == AF_INET6 &&
> > -		inet_sk(sk)->inet_num == IPPROTO_ICMPV6;
> > -}
> > -
> >  #endif
> > diff --git a/include/linux/mroute.h b/include/linux/mroute.h
> > index 94c6e6f549f0a503f6707d1175f1c47a0f0cd887..4c5003afee6c51962bc13879978845bc7daf08fa 100644
> > --- a/include/linux/mroute.h
> > +++ b/include/linux/mroute.h
> > @@ -16,12 +16,6 @@ static inline int ip_mroute_opt(int opt)
> >  	return opt >= MRT_BASE && opt <= MRT_MAX;
> >  }
> >  
> > -static inline int sk_is_ipmr(struct sock *sk)
> > -{
> > -	return sk->sk_family == AF_INET &&
> > -		inet_sk(sk)->inet_num == IPPROTO_IGMP;
> > -}
> > -
> >  int ip_mroute_setsockopt(struct sock *, int, sockptr_t, unsigned int);
> >  int ip_mroute_getsockopt(struct sock *, int, sockptr_t, sockptr_t);
> >  int ipmr_ioctl(struct sock *sk, int cmd, void *arg);
> > @@ -57,11 +51,6 @@ static inline int ip_mroute_opt(int opt)
> >  	return 0;
> >  }
> >  
> > -static inline int sk_is_ipmr(struct sock *sk)
> > -{
> > -	return 0;
> > -}
> > -
> >  static inline bool ipmr_rule_default(const struct fib_rule *rule)
> >  {
> >  	return true;
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index cff3e82514d17513bc96300dc18014083a0d7ea7..8ec8f4c9911f2a6dd3dd946798d512394d62a861 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -4199,9 +4199,9 @@ int sk_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
> >  {
> >  	int rc = 1;
> >  
> > -	if (sk_is_ipmr(sk))
> > +	if (sk->sk_type == SOCK_RAW && sk->sk_family == AF_INET)
> >  		rc = ipmr_sk_ioctl(sk, cmd, arg);
> > -	else if (sk_is_icmpv6(sk))
> > +	else if (sk->sk_type == SOCK_RAW && sk->sk_family == AF_INET6)
> >  		rc = ip6mr_sk_ioctl(sk, cmd, arg);
> >  	else if (sk_is_phonet(sk))
> >  		rc = phonet_sk_ioctl(sk, cmd, arg);
> 
> I was staring at the type when reviewing the original patch; I was
> expecting to see RAW based on where ipmr_ioctl (and v6 version) is
> called. That's why I asked about the testing of the patch, so I am not
> surprised by this followup.
> 
> Reviewed-by: David Ahern <dsahern@kernel.org>

ipmr_ioctl is called from raw_ioctl without any additional checks on
inet_sk(sk)->inet_num too (as are ip6mr_ioctl and ipmr_compat_ioctl),
and casts to raw_sk(sk) without further checks. So this condition
lgtm too.

And conversely, interpreting inet_num(sk) as a protocol is only okay
after testing sk_type == SOCK_RAW.

Thanks for the fix, Eric

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

Re: [PATCH net-next] net: remove sk_is_ipmr() and sk_is_icmpv6() helpers

[patchwork-bot+netdevbpf]

Hello:

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

On Mon, 19 Jun 2023 12:43:35 +0000 you 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()
> 
> [...]

Here is the summary with links:
  - [net-next] net: remove sk_is_ipmr() and sk_is_icmpv6() helpers
    https://git.kernel.org/netdev/net-next/c/634236b34d7a

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