* Re: [PATCH v1 net] smc: Fix various oops due to inet_sock type confusion.
2025-07-11 6:07 [PATCH v1 net] smc: Fix various oops due to inet_sock type confusion Kuniyuki Iwashima
@ 2025-07-14 2:04 ` D. Wythe
2025-07-14 2:33 ` Wang Liang
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: D. Wythe @ 2025-07-14 2:04 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: D. Wythe, Dust Li, Sidraya Jayagond, Wenjia Zhang,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Mahanta Jambigi, Tony Lu, Wen Gu, Simon Horman, Kuniyuki Iwashima,
netdev, linux-rdma, linux-s390, syzbot+40bf00346c3fe40f90f2,
syzbot+f22031fad6cbe52c70e7, syzbot+271fed3ed6f24600c364
On Fri, Jul 11, 2025 at 06:07:52AM +0000, Kuniyuki Iwashima wrote:
> syzbot reported weird splats [0][1] in cipso_v4_sock_setattr() while
> freeing inet_sk(sk)->inet_opt.
>
> The address was freed multiple times even though it was read-only memory.
>
> cipso_v4_sock_setattr() did nothing wrong, and the root cause was type
> confusion.
>
> The cited commit made it possible to create smc_sock as an INET socket.
>
> The issue is that struct smc_sock does not have struct inet_sock as the
> first member but hijacks AF_INET and AF_INET6 sk_family, which confuses
> various places.
>
> In this case, inet_sock.inet_opt was actually smc_sock.clcsk_data_ready(),
> which is an address of a function in the text segment.
>
> $ pahole -C inet_sock vmlinux
> struct inet_sock {
> ...
> struct ip_options_rcu * inet_opt; /* 784 8 */
>
> $ pahole -C smc_sock vmlinux
> struct smc_sock {
> ...
> void (*clcsk_data_ready)(struct sock *); /* 784 8 */
>
>
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 3760131f14845..1882bab8e00e7 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -30,6 +30,10 @@
> #include <linux/splice.h>
>
> #include <net/sock.h>
> +#include <net/inet_common.h>
> +#if IS_ENABLED(CONFIG_IPV6)
> +#include <net/ipv6.h>
> +#endif
> #include <net/tcp.h>
> #include <net/smc.h>
> #include <asm/ioctls.h>
> @@ -360,6 +364,16 @@ static void smc_destruct(struct sock *sk)
> return;
> if (!sock_flag(sk, SOCK_DEAD))
> return;
> + switch (sk->sk_family) {
> + case AF_INET:
> + inet_sock_destruct(sk);
> + break;
> +#if IS_ENABLED(CONFIG_IPV6)
> + case AF_INET6:
> + inet6_sock_destruct(sk);
> + break;
> +#endif
> + }
> }
>
> static struct lock_class_key smc_key;
> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index 78ae10d06ed2e..2c90849637398 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -283,10 +283,10 @@ struct smc_connection {
> };
>
> struct smc_sock { /* smc sock container */
> - struct sock sk;
> -#if IS_ENABLED(CONFIG_IPV6)
> - struct ipv6_pinfo *pinet6;
> -#endif
> + union {
> + struct sock sk;
> + struct inet_sock icsk_inet;
> + };
> struct socket *clcsock; /* internal tcp socket */
> void (*clcsk_state_change)(struct sock *sk);
> /* original stat_change fct. */
> --
LGTM. This is what we should have resolved a long time ago. Thank
you for your report and repair!
Reviewed-by: D. Wythe <alibuda@linux.alibaba.com>
> 2.50.0.727.gbf7dc18ff4-goog
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1 net] smc: Fix various oops due to inet_sock type confusion.
2025-07-11 6:07 [PATCH v1 net] smc: Fix various oops due to inet_sock type confusion Kuniyuki Iwashima
2025-07-14 2:04 ` D. Wythe
@ 2025-07-14 2:33 ` Wang Liang
2025-07-14 7:42 ` Alexandra Winter
2025-07-15 0:20 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 8+ messages in thread
From: Wang Liang @ 2025-07-14 2:33 UTC (permalink / raw)
To: Kuniyuki Iwashima, D. Wythe, Dust Li, Sidraya Jayagond,
Wenjia Zhang, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Mahanta Jambigi, Tony Lu, Wen Gu, Simon Horman, Kuniyuki Iwashima,
netdev, linux-rdma, linux-s390, syzbot+40bf00346c3fe40f90f2,
syzbot+f22031fad6cbe52c70e7, syzbot+271fed3ed6f24600c364
在 2025/7/11 14:07, Kuniyuki Iwashima 写道:
> syzbot reported weird splats [0][1] in cipso_v4_sock_setattr() while
> freeing inet_sk(sk)->inet_opt.
>
> The address was freed multiple times even though it was read-only memory.
>
> cipso_v4_sock_setattr() did nothing wrong, and the root cause was type
> confusion.
>
> The cited commit made it possible to create smc_sock as an INET socket.
>
> The issue is that struct smc_sock does not have struct inet_sock as the
> first member but hijacks AF_INET and AF_INET6 sk_family, which confuses
> various places.
>
> In this case, inet_sock.inet_opt was actually smc_sock.clcsk_data_ready(),
> which is an address of a function in the text segment.
>
> $ pahole -C inet_sock vmlinux
> struct inet_sock {
> ...
> struct ip_options_rcu * inet_opt; /* 784 8 */
>
> $ pahole -C smc_sock vmlinux
> struct smc_sock {
> ...
> void (*clcsk_data_ready)(struct sock *); /* 784 8 */
>
> The same issue for another field was reported before. [2][3]
>
> At that time, an ugly hack was suggested [4], but it makes both INET
> and SMC code error-prone and hard to change.
>
> Also, yet another variant was fixed by a hacky commit 98d4435efcbf3
> ("net/smc: prevent NULL pointer dereference in txopt_get").
>
> Instead of papering over the root cause by such hacks, we should not
> allow non-INET socket to reuse the INET infra.
>
> Let's add inet_sock as the first member of smc_sock.
>
> [0]:
> kvfree_call_rcu(): Double-freed call. rcu_head 000000006921da73
> WARNING: CPU: 0 PID: 6718 at mm/slab_common.c:1956 kvfree_call_rcu+0x94/0x3f0 mm/slab_common.c:1955
> Modules linked in:
> CPU: 0 UID: 0 PID: 6718 Comm: syz.0.17 Tainted: G W 6.16.0-rc4-syzkaller-g7482bb149b9f #0 PREEMPT
> Tainted: [W]=WARN
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/07/2025
> pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : kvfree_call_rcu+0x94/0x3f0 mm/slab_common.c:1955
> lr : kvfree_call_rcu+0x94/0x3f0 mm/slab_common.c:1955
> sp : ffff8000a03a7730
> x29: ffff8000a03a7730 x28: 00000000fffffff5 x27: 1fffe000184823d3
> x26: dfff800000000000 x25: ffff0000c2411e9e x24: ffff0000dd88da00
> x23: ffff8000891ac9a0 x22: 00000000ffffffea x21: ffff8000891ac9a0
> x20: ffff8000891ac9a0 x19: ffff80008afc2480 x18: 00000000ffffffff
> x17: 0000000000000000 x16: ffff80008ae642c8 x15: ffff700011ede14c
> x14: 1ffff00011ede14c x13: 0000000000000004 x12: ffffffffffffffff
> x11: ffff700011ede14c x10: 0000000000ff0100 x9 : 5fa3c1ffaf0ff000
> x8 : 5fa3c1ffaf0ff000 x7 : 0000000000000001 x6 : 0000000000000001
> x5 : ffff8000a03a7078 x4 : ffff80008f766c20 x3 : ffff80008054d360
> x2 : 0000000000000000 x1 : 0000000000000201 x0 : 0000000000000000
> Call trace:
> kvfree_call_rcu+0x94/0x3f0 mm/slab_common.c:1955 (P)
> cipso_v4_sock_setattr+0x2f0/0x3f4 net/ipv4/cipso_ipv4.c:1914
> netlbl_sock_setattr+0x240/0x334 net/netlabel/netlabel_kapi.c:1000
> smack_netlbl_add+0xa8/0x158 security/smack/smack_lsm.c:2581
> smack_inode_setsecurity+0x378/0x430 security/smack/smack_lsm.c:2912
> security_inode_setsecurity+0x118/0x3c0 security/security.c:2706
> __vfs_setxattr_noperm+0x174/0x5c4 fs/xattr.c:251
> __vfs_setxattr_locked+0x1ec/0x218 fs/xattr.c:295
> vfs_setxattr+0x158/0x2ac fs/xattr.c:321
> do_setxattr fs/xattr.c:636 [inline]
> file_setxattr+0x1b8/0x294 fs/xattr.c:646
> path_setxattrat+0x2ac/0x320 fs/xattr.c:711
> __do_sys_fsetxattr fs/xattr.c:761 [inline]
> __se_sys_fsetxattr fs/xattr.c:758 [inline]
> __arm64_sys_fsetxattr+0xc0/0xdc fs/xattr.c:758
> __invoke_syscall arch/arm64/kernel/syscall.c:35 [inline]
> invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:49
> el0_svc_common+0x130/0x23c arch/arm64/kernel/syscall.c:132
> do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151
> el0_svc+0x58/0x180 arch/arm64/kernel/entry-common.c:879
> el0t_64_sync_handler+0x84/0x12c arch/arm64/kernel/entry-common.c:898
> el0t_64_sync+0x198/0x19c arch/arm64/kernel/entry.S:600
>
> [1]:
> Unable to handle kernel write to read-only memory at virtual address ffff8000891ac9a8
> KASAN: probably user-memory-access in range [0x0000000448d64d40-0x0000000448d64d47]
> Mem abort info:
> ESR = 0x000000009600004e
> EC = 0x25: DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> FSC = 0x0e: level 2 permission fault
> Data abort info:
> ISV = 0, ISS = 0x0000004e, ISS2 = 0x00000000
> CM = 0, WnR = 1, TnD = 0, TagAccess = 0
> GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000207144000
> [ffff8000891ac9a8] pgd=0000000000000000, p4d=100000020f950003, pud=100000020f951003, pmd=0040000201000781
> Internal error: Oops: 000000009600004e [#1] SMP
> Modules linked in:
> CPU: 0 UID: 0 PID: 6946 Comm: syz.0.69 Not tainted 6.16.0-rc4-syzkaller-g7482bb149b9f #0 PREEMPT
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/07/2025
> pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : kvfree_call_rcu+0x31c/0x3f0 mm/slab_common.c:1971
> lr : add_ptr_to_bulk_krc_lock mm/slab_common.c:1838 [inline]
> lr : kvfree_call_rcu+0xfc/0x3f0 mm/slab_common.c:1963
> sp : ffff8000a28a7730
> x29: ffff8000a28a7730 x28: 00000000fffffff5 x27: 1fffe00018b09bb3
> x26: 0000000000000001 x25: ffff80008f66e000 x24: ffff00019beaf498
> x23: ffff00019beaf4c0 x22: 0000000000000000 x21: ffff8000891ac9a0
> x20: ffff8000891ac9a0 x19: 0000000000000000 x18: 00000000ffffffff
> x17: ffff800093363000 x16: ffff80008052c6e4 x15: ffff700014514ecc
> x14: 1ffff00014514ecc x13: 0000000000000004 x12: ffffffffffffffff
> x11: ffff700014514ecc x10: 0000000000000001 x9 : 0000000000000001
> x8 : ffff00019beaf7b4 x7 : ffff800080a94154 x6 : 0000000000000000
> x5 : ffff8000935efa60 x4 : 0000000000000008 x3 : ffff80008052c7fc
> x2 : 0000000000000001 x1 : ffff8000891ac9a0 x0 : 0000000000000001
> Call trace:
> kvfree_call_rcu+0x31c/0x3f0 mm/slab_common.c:1967 (P)
> cipso_v4_sock_setattr+0x2f0/0x3f4 net/ipv4/cipso_ipv4.c:1914
> netlbl_sock_setattr+0x240/0x334 net/netlabel/netlabel_kapi.c:1000
> smack_netlbl_add+0xa8/0x158 security/smack/smack_lsm.c:2581
> smack_inode_setsecurity+0x378/0x430 security/smack/smack_lsm.c:2912
> security_inode_setsecurity+0x118/0x3c0 security/security.c:2706
> __vfs_setxattr_noperm+0x174/0x5c4 fs/xattr.c:251
> __vfs_setxattr_locked+0x1ec/0x218 fs/xattr.c:295
> vfs_setxattr+0x158/0x2ac fs/xattr.c:321
> do_setxattr fs/xattr.c:636 [inline]
> file_setxattr+0x1b8/0x294 fs/xattr.c:646
> path_setxattrat+0x2ac/0x320 fs/xattr.c:711
> __do_sys_fsetxattr fs/xattr.c:761 [inline]
> __se_sys_fsetxattr fs/xattr.c:758 [inline]
> __arm64_sys_fsetxattr+0xc0/0xdc fs/xattr.c:758
> __invoke_syscall arch/arm64/kernel/syscall.c:35 [inline]
> invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:49
> el0_svc_common+0x130/0x23c arch/arm64/kernel/syscall.c:132
> do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151
> el0_svc+0x58/0x180 arch/arm64/kernel/entry-common.c:879
> el0t_64_sync_handler+0x84/0x12c arch/arm64/kernel/entry-common.c:898
> el0t_64_sync+0x198/0x19c arch/arm64/kernel/entry.S:600
> Code: aa1f03e2 52800023 97ee1e8d b4000195 (f90006b4)
>
> Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
> Reported-by: syzbot+40bf00346c3fe40f90f2@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/686d9b50.050a0220.1ffab7.0020.GAE@google.com/
> Tested-by: syzbot+40bf00346c3fe40f90f2@syzkaller.appspotmail.com
> Reported-by: syzbot+f22031fad6cbe52c70e7@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/686da0f3.050a0220.1ffab7.0022.GAE@google.com/
> Reported-by: syzbot+271fed3ed6f24600c364@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364 # [2]
> Link: https://lore.kernel.org/netdev/99f284be-bf1d-4bc4-a629-77b268522fff@huawei.com/ # [3]
> Link: https://lore.kernel.org/netdev/20250331081003.1503211-1-wangliang74@huawei.com/ # [4]
> Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
> ---
> net/smc/af_smc.c | 14 ++++++++++++++
> net/smc/smc.h | 8 ++++----
> 2 files changed, 18 insertions(+), 4 deletions(-)
Reviewed-by: Wang Liang <wangliang74@huawei.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1 net] smc: Fix various oops due to inet_sock type confusion.
2025-07-11 6:07 [PATCH v1 net] smc: Fix various oops due to inet_sock type confusion Kuniyuki Iwashima
2025-07-14 2:04 ` D. Wythe
2025-07-14 2:33 ` Wang Liang
@ 2025-07-14 7:42 ` Alexandra Winter
2025-07-14 16:23 ` Kuniyuki Iwashima
2025-07-15 11:58 ` D. Wythe
2025-07-15 0:20 ` patchwork-bot+netdevbpf
3 siblings, 2 replies; 8+ messages in thread
From: Alexandra Winter @ 2025-07-14 7:42 UTC (permalink / raw)
To: Kuniyuki Iwashima, D. Wythe, Dust Li, Sidraya Jayagond,
Wenjia Zhang, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Mahanta Jambigi, Tony Lu, Wen Gu, Simon Horman, Kuniyuki Iwashima,
netdev, linux-rdma, linux-s390, syzbot+40bf00346c3fe40f90f2,
syzbot+f22031fad6cbe52c70e7, syzbot+271fed3ed6f24600c364
On 11.07.25 08:07, Kuniyuki Iwashima wrote:
> syzbot reported weird splats [0][1] in cipso_v4_sock_setattr() while
> freeing inet_sk(sk)->inet_opt.
>
> The address was freed multiple times even though it was read-only memory.
>
> cipso_v4_sock_setattr() did nothing wrong, and the root cause was type
> confusion.
>
> The cited commit made it possible to create smc_sock as an INET socket.
>
> The issue is that struct smc_sock does not have struct inet_sock as the
> first member but hijacks AF_INET and AF_INET6 sk_family, which confuses
> various places.
>
> In this case, inet_sock.inet_opt was actually smc_sock.clcsk_data_ready(),
> which is an address of a function in the text segment.
>
> $ pahole -C inet_sock vmlinux
> struct inet_sock {
> ...
> struct ip_options_rcu * inet_opt; /* 784 8 */
>
> $ pahole -C smc_sock vmlinux
> struct smc_sock {
> ...
> void (*clcsk_data_ready)(struct sock *); /* 784 8 */
>
> The same issue for another field was reported before. [2][3]
>
> At that time, an ugly hack was suggested [4], but it makes both INET
> and SMC code error-prone and hard to change.
>
> Also, yet another variant was fixed by a hacky commit 98d4435efcbf3
> ("net/smc: prevent NULL pointer dereference in txopt_get").
>
> Instead of papering over the root cause by such hacks, we should not
> allow non-INET socket to reuse the INET infra.
>
> Let's add inet_sock as the first member of smc_sock.
>
[...]
>
> static struct lock_class_key smc_key;
> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index 78ae10d06ed2e..2c90849637398 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -283,10 +283,10 @@ struct smc_connection {
> };
>
> struct smc_sock { /* smc sock container */
> - struct sock sk;
> -#if IS_ENABLED(CONFIG_IPV6)
> - struct ipv6_pinfo *pinet6;
> -#endif
> + union {
> + struct sock sk;
> + struct inet_sock icsk_inet;
> + };
> struct socket *clcsock; /* internal tcp socket */
> void (*clcsk_state_change)(struct sock *sk);
> /* original stat_change fct. */
I would like to remind us of the discussions August 2024 around a patchset
called "net/smc: prevent NULL pointer dereference in txopt_get".
That discussion eventually ended up in the reduced (?)
commit 98d4435efcbf ("net/smc: prevent NULL pointer dereference in txopt_get")
without a union.
I still think this union looks dangerous, but don't understand the code well enough to
propose an alternative.
Maybe incorporate inet_sock in smc_sock? Like Paoplo suggested in
https://lore.kernel.org/lkml/20240815043714.38772-1-aha310510@gmail.com/T/#maf6ee926f782736cb6accd2ba162dea0a34e02f9
He also asked for at least some explanatory comments in the union. Which would help me as well.
Kind regards
Alexandra
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1 net] smc: Fix various oops due to inet_sock type confusion.
2025-07-14 7:42 ` Alexandra Winter
@ 2025-07-14 16:23 ` Kuniyuki Iwashima
2025-07-15 11:58 ` D. Wythe
1 sibling, 0 replies; 8+ messages in thread
From: Kuniyuki Iwashima @ 2025-07-14 16:23 UTC (permalink / raw)
To: Alexandra Winter
Cc: D. Wythe, Dust Li, Sidraya Jayagond, Wenjia Zhang,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Mahanta Jambigi, Tony Lu, Wen Gu, Simon Horman, Kuniyuki Iwashima,
netdev, linux-rdma, linux-s390, syzbot+40bf00346c3fe40f90f2,
syzbot+f22031fad6cbe52c70e7, syzbot+271fed3ed6f24600c364
On Mon, Jul 14, 2025 at 12:42 AM Alexandra Winter <wintera@linux.ibm.com> wrote:
> On 11.07.25 08:07, Kuniyuki Iwashima wrote:
> > syzbot reported weird splats [0][1] in cipso_v4_sock_setattr() while
> > freeing inet_sk(sk)->inet_opt.
> >
> > The address was freed multiple times even though it was read-only memory.
> >
> > cipso_v4_sock_setattr() did nothing wrong, and the root cause was type
> > confusion.
> >
> > The cited commit made it possible to create smc_sock as an INET socket.
> >
> > The issue is that struct smc_sock does not have struct inet_sock as the
> > first member but hijacks AF_INET and AF_INET6 sk_family, which confuses
> > various places.
> >
> > In this case, inet_sock.inet_opt was actually smc_sock.clcsk_data_ready(),
> > which is an address of a function in the text segment.
> >
> > $ pahole -C inet_sock vmlinux
> > struct inet_sock {
> > ...
> > struct ip_options_rcu * inet_opt; /* 784 8 */
> >
> > $ pahole -C smc_sock vmlinux
> > struct smc_sock {
> > ...
> > void (*clcsk_data_ready)(struct sock *); /* 784 8 */
> >
> > The same issue for another field was reported before. [2][3]
> >
> > At that time, an ugly hack was suggested [4], but it makes both INET
> > and SMC code error-prone and hard to change.
> >
> > Also, yet another variant was fixed by a hacky commit 98d4435efcbf3
> > ("net/smc: prevent NULL pointer dereference in txopt_get").
> >
> > Instead of papering over the root cause by such hacks, we should not
> > allow non-INET socket to reuse the INET infra.
> >
> > Let's add inet_sock as the first member of smc_sock.
> >
> [...]
> >
> > static struct lock_class_key smc_key;
> > diff --git a/net/smc/smc.h b/net/smc/smc.h
> > index 78ae10d06ed2e..2c90849637398 100644
> > --- a/net/smc/smc.h
> > +++ b/net/smc/smc.h
> > @@ -283,10 +283,10 @@ struct smc_connection {
> > };
> >
> > struct smc_sock { /* smc sock container */
> > - struct sock sk;
> > -#if IS_ENABLED(CONFIG_IPV6)
> > - struct ipv6_pinfo *pinet6;
> > -#endif
> > + union {
> > + struct sock sk;
> > + struct inet_sock icsk_inet;
> > + };
> > struct socket *clcsock; /* internal tcp socket */
> > void (*clcsk_state_change)(struct sock *sk);
> > /* original stat_change fct. */
>
> I would like to remind us of the discussions August 2024 around a patchset
> called "net/smc: prevent NULL pointer dereference in txopt_get".
> That discussion eventually ended up in the reduced (?)
> commit 98d4435efcbf ("net/smc: prevent NULL pointer dereference in txopt_get")
> without a union.
>
> I still think this union looks dangerous, but don't understand the code well enough to
> propose an alternative.
>
> Maybe incorporate inet_sock in smc_sock? Like Paoplo suggested in
> https://lore.kernel.org/lkml/20240815043714.38772-1-aha310510@gmail.com/T/#maf6ee926f782736cb6accd2ba162dea0a34e02f9
>
> He also asked for at least some explanatory comments in the union. Which would help me as well.
I agree with Paolo that smc_sock should "eventually" have only
inet_sock as the first member, but I think this should/can be done
in net-next as follow-up.
The thread above shows such code churn was distracting enough
and the improper fix was introduced.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1 net] smc: Fix various oops due to inet_sock type confusion.
2025-07-14 7:42 ` Alexandra Winter
2025-07-14 16:23 ` Kuniyuki Iwashima
@ 2025-07-15 11:58 ` D. Wythe
2025-07-15 17:07 ` Kuniyuki Iwashima
1 sibling, 1 reply; 8+ messages in thread
From: D. Wythe @ 2025-07-15 11:58 UTC (permalink / raw)
To: Alexandra Winter
Cc: Kuniyuki Iwashima, D. Wythe, Dust Li, Sidraya Jayagond,
Wenjia Zhang, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Mahanta Jambigi, Tony Lu, Wen Gu, Simon Horman,
Kuniyuki Iwashima, netdev, linux-rdma, linux-s390,
syzbot+40bf00346c3fe40f90f2, syzbot+f22031fad6cbe52c70e7,
syzbot+271fed3ed6f24600c364
On Mon, Jul 14, 2025 at 09:42:22AM +0200, Alexandra Winter wrote:
>
>
> On 11.07.25 08:07, Kuniyuki Iwashima wrote:
> > syzbot reported weird splats [0][1] in cipso_v4_sock_setattr() while
> > freeing inet_sk(sk)->inet_opt.
> >
> > The address was freed multiple times even though it was read-only memory.
> >
> > cipso_v4_sock_setattr() did nothing wrong, and the root cause was type
> > confusion.
> >
> > The cited commit made it possible to create smc_sock as an INET socket.
> >
> > The issue is that struct smc_sock does not have struct inet_sock as the
> > first member but hijacks AF_INET and AF_INET6 sk_family, which confuses
> > various places.
> >
> > In this case, inet_sock.inet_opt was actually smc_sock.clcsk_data_ready(),
>
> I would like to remind us of the discussions August 2024 around a patchset
> called "net/smc: prevent NULL pointer dereference in txopt_get".
> That discussion eventually ended up in the reduced (?)
> commit 98d4435efcbf ("net/smc: prevent NULL pointer dereference in txopt_get")
> without a union.
>
> I still think this union looks dangerous, but don't understand the code well enough to
> propose an alternative.
>
> Maybe incorporate inet_sock in smc_sock? Like Paoplo suggested in
> https://lore.kernel.org/lkml/20240815043714.38772-1-aha310510@gmail.com/T/#maf6ee926f782736cb6accd2ba162dea0a34e02f9
>
> He also asked for at least some explanatory comments in the union. Which would help me as well.
>
Just caught this suggestion... The primary risk with using a union is the
potential for the sk member's offset within the inet_sock structure to
change in the future, although this is highly improbable. But in any
case, directly using inet_sock is certainly a safer approach.
Uncertain if @Kuniyuki will still get to revise a version, If there's no further
follow-up, I'll make the changes when I get a change.
Best wishes,
D. Wythe
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1 net] smc: Fix various oops due to inet_sock type confusion.
2025-07-15 11:58 ` D. Wythe
@ 2025-07-15 17:07 ` Kuniyuki Iwashima
0 siblings, 0 replies; 8+ messages in thread
From: Kuniyuki Iwashima @ 2025-07-15 17:07 UTC (permalink / raw)
To: D. Wythe
Cc: Alexandra Winter, Dust Li, Sidraya Jayagond, Wenjia Zhang,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Mahanta Jambigi, Tony Lu, Wen Gu, Simon Horman, Kuniyuki Iwashima,
netdev, linux-rdma, linux-s390, syzbot+40bf00346c3fe40f90f2,
syzbot+f22031fad6cbe52c70e7, syzbot+271fed3ed6f24600c364
On Tue, Jul 15, 2025 at 4:59 AM D. Wythe <alibuda@linux.alibaba.com> wrote:
>
> On Mon, Jul 14, 2025 at 09:42:22AM +0200, Alexandra Winter wrote:
> >
> >
> > On 11.07.25 08:07, Kuniyuki Iwashima wrote:
> > > syzbot reported weird splats [0][1] in cipso_v4_sock_setattr() while
> > > freeing inet_sk(sk)->inet_opt.
> > >
> > > The address was freed multiple times even though it was read-only memory.
> > >
> > > cipso_v4_sock_setattr() did nothing wrong, and the root cause was type
> > > confusion.
> > >
> > > The cited commit made it possible to create smc_sock as an INET socket.
> > >
> > > The issue is that struct smc_sock does not have struct inet_sock as the
> > > first member but hijacks AF_INET and AF_INET6 sk_family, which confuses
> > > various places.
> > >
> > > In this case, inet_sock.inet_opt was actually smc_sock.clcsk_data_ready(),
> >
> > I would like to remind us of the discussions August 2024 around a patchset
> > called "net/smc: prevent NULL pointer dereference in txopt_get".
> > That discussion eventually ended up in the reduced (?)
> > commit 98d4435efcbf ("net/smc: prevent NULL pointer dereference in txopt_get")
> > without a union.
> >
> > I still think this union looks dangerous, but don't understand the code well enough to
> > propose an alternative.
> >
> > Maybe incorporate inet_sock in smc_sock? Like Paoplo suggested in
> > https://lore.kernel.org/lkml/20240815043714.38772-1-aha310510@gmail.com/T/#maf6ee926f782736cb6accd2ba162dea0a34e02f9
> >
> > He also asked for at least some explanatory comments in the union. Which would help me as well.
> >
>
> Just caught this suggestion... The primary risk with using a union is the
> potential for the sk member's offset within the inet_sock structure to
> change in the future, although this is highly improbable.
Right, and this only happens when we start using inet_sock in the union.
> But in any
> case, directly using inet_sock is certainly a safer approach.
>
> Uncertain if @Kuniyuki will still get to revise a version, If there's no further
> follow-up, I'll make the changes when I get a change.
I'll post a follow-up once net.git is merged to net-next.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 net] smc: Fix various oops due to inet_sock type confusion.
2025-07-11 6:07 [PATCH v1 net] smc: Fix various oops due to inet_sock type confusion Kuniyuki Iwashima
` (2 preceding siblings ...)
2025-07-14 7:42 ` Alexandra Winter
@ 2025-07-15 0:20 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 8+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-07-15 0:20 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: alibuda, dust.li, sidraya, wenjia, davem, edumazet, kuba, pabeni,
mjambigi, tonylu, guwen, horms, kuni1840, netdev, linux-rdma,
linux-s390, syzbot+40bf00346c3fe40f90f2,
syzbot+f22031fad6cbe52c70e7, syzbot+271fed3ed6f24600c364
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Fri, 11 Jul 2025 06:07:52 +0000 you wrote:
> syzbot reported weird splats [0][1] in cipso_v4_sock_setattr() while
> freeing inet_sk(sk)->inet_opt.
>
> The address was freed multiple times even though it was read-only memory.
>
> cipso_v4_sock_setattr() did nothing wrong, and the root cause was type
> confusion.
>
> [...]
Here is the summary with links:
- [v1,net] smc: Fix various oops due to inet_sock type confusion.
https://git.kernel.org/netdev/net/c/60ada4fe644e
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 8+ messages in thread