* [PATCH net] net/smc: prevent NULL pointer dereference in txopt_get
@ 2024-08-10 17:22 Jeongjun Park
2024-08-13 4:06 ` Jeongjun Park
2024-08-13 8:05 ` Gerd Bayer
0 siblings, 2 replies; 6+ messages in thread
From: Jeongjun Park @ 2024-08-10 17:22 UTC (permalink / raw)
To: wenjia, jaka
Cc: alibuda, tonylu, guwen, davem, edumazet, kuba, pabeni, dust.li,
linux-s390, netdev, linux-kernel, Jeongjun Park
Since smc_inet6_prot does not initialize ipv6_pinfo_offset, inet6_create()
copies an incorrect address value, sk + 0 (offset), to inet_sk(sk)->pinet6.
In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock practically
point to the same address, when smc_create_clcsk() stores the newly
created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6 is corrupted
into clcsock. This causes NULL pointer dereference and various other
memory corruptions.
To solve this, we need to add a smc6_sock structure for ipv6_pinfo_offset
initialization and modify the smc_sock structure.
[ 278.629552][T28696] ==================================================================
[ 278.631367][T28696] BUG: KASAN: null-ptr-deref in txopt_get+0x102/0x430
[ 278.632724][T28696] Read of size 4 at addr 0000000000000200 by task syz.0.2965/28696
[ 278.634802][T28696]
[ 278.635236][T28696] CPU: 0 UID: 0 PID: 28696 Comm: syz.0.2965 Not tainted 6.11.0-rc2 #3
[ 278.637458][T28696] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
[ 278.639426][T28696] Call Trace:
[ 278.639833][T28696] <TASK>
[ 278.640190][T28696] dump_stack_lvl+0x116/0x1b0
[ 278.640844][T28696] ? txopt_get+0x102/0x430
[ 278.641620][T28696] kasan_report+0xbd/0xf0
[ 278.642440][T28696] ? txopt_get+0x102/0x430
[ 278.643291][T28696] kasan_check_range+0xf4/0x1a0
[ 278.644163][T28696] txopt_get+0x102/0x430
[ 278.644940][T28696] ? __pfx_txopt_get+0x10/0x10
[ 278.645877][T28696] ? selinux_netlbl_socket_setsockopt+0x1d0/0x420
[ 278.646972][T28696] calipso_sock_getattr+0xc6/0x3e0
[ 278.647630][T28696] calipso_sock_getattr+0x4b/0x80
[ 278.648349][T28696] netlbl_sock_getattr+0x63/0xc0
[ 278.649318][T28696] selinux_netlbl_socket_setsockopt+0x1db/0x420
[ 278.650471][T28696] ? __pfx_selinux_netlbl_socket_setsockopt+0x10/0x10
[ 278.652217][T28696] ? find_held_lock+0x2d/0x120
[ 278.652231][T28696] selinux_socket_setsockopt+0x66/0x90
[ 278.652247][T28696] security_socket_setsockopt+0x57/0xb0
[ 278.652278][T28696] do_sock_setsockopt+0xf2/0x480
[ 278.652289][T28696] ? __pfx_do_sock_setsockopt+0x10/0x10
[ 278.652298][T28696] ? __fget_files+0x24b/0x4a0
[ 278.652308][T28696] ? __fget_light+0x177/0x210
[ 278.652316][T28696] __sys_setsockopt+0x1a6/0x270
[ 278.652328][T28696] ? __pfx___sys_setsockopt+0x10/0x10
[ 278.661787][T28696] ? xfd_validate_state+0x5d/0x180
[ 278.662821][T28696] __x64_sys_setsockopt+0xbd/0x160
[ 278.663719][T28696] ? lockdep_hardirqs_on+0x7c/0x110
[ 278.664690][T28696] do_syscall_64+0xcb/0x250
[ 278.665507][T28696] entry_SYSCALL_64_after_hwframe+0x77/0x7f
[ 278.666618][T28696] RIP: 0033:0x7fe87ed9712d
[ 278.667236][T28696] Code: 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa 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 a8 ff ff ff f7 d8 64 89 01 48
[ 278.670801][T28696] RSP: 002b:00007fe87faa4fa8 EFLAGS: 00000246 ORIG_RAX: 0000000000000036
[ 278.671832][T28696] RAX: ffffffffffffffda RBX: 00007fe87ef35f80 RCX: 00007fe87ed9712d
[ 278.672806][T28696] RDX: 0000000000000036 RSI: 0000000000000029 RDI: 0000000000000003
[ 278.674263][T28696] RBP: 00007fe87ee1bd8a R08: 0000000000000018 R09: 0000000000000000
[ 278.675967][T28696] R10: 0000000020000000 R11: 0000000000000246 R12: 0000000000000000
[ 278.677953][T28696] R13: 000000000000000b R14: 00007fe87ef35f80 R15: 00007fe87fa85000
[ 278.679321][T28696] </TASK>
[ 278.679917][T28696] ==================================================================
Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
net/smc/smc.h | 19 ++++++++++---------
net/smc/smc_inet.c | 24 +++++++++++++++---------
2 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/net/smc/smc.h b/net/smc/smc.h
index 34b781e463c4..f4d9338b5ed5 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -284,15 +284,6 @@ struct smc_connection {
struct smc_sock { /* smc sock container */
struct sock sk;
- struct socket *clcsock; /* internal tcp socket */
- void (*clcsk_state_change)(struct sock *sk);
- /* original stat_change fct. */
- void (*clcsk_data_ready)(struct sock *sk);
- /* original data_ready fct. */
- void (*clcsk_write_space)(struct sock *sk);
- /* original write_space fct. */
- void (*clcsk_error_report)(struct sock *sk);
- /* original error_report fct. */
struct smc_connection conn; /* smc connection */
struct smc_sock *listen_smc; /* listen parent */
struct work_struct connect_work; /* handle non-blocking connect*/
@@ -325,6 +316,16 @@ struct smc_sock { /* smc sock container */
/* protects clcsock of a listen
* socket
* */
+ struct socket *clcsock; /* internal tcp socket */
+ void (*clcsk_state_change)(struct sock *sk);
+ /* original stat_change fct. */
+ void (*clcsk_data_ready)(struct sock *sk);
+ /* original data_ready fct. */
+ void (*clcsk_write_space)(struct sock *sk);
+ /* original write_space fct. */
+ void (*clcsk_error_report)(struct sock *sk);
+ /* original error_report fct. */
+
};
#define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
index bece346dd8e9..3c54faef6042 100644
--- a/net/smc/smc_inet.c
+++ b/net/smc/smc_inet.c
@@ -60,16 +60,22 @@ static struct inet_protosw smc_inet_protosw = {
};
#if IS_ENABLED(CONFIG_IPV6)
+struct smc6_sock {
+ struct smc_sock smc;
+ struct ipv6_pinfo np;
+};
+
static struct proto smc_inet6_prot = {
- .name = "INET6_SMC",
- .owner = THIS_MODULE,
- .init = smc_inet_init_sock,
- .hash = smc_hash_sk,
- .unhash = smc_unhash_sk,
- .release_cb = smc_release_cb,
- .obj_size = sizeof(struct smc_sock),
- .h.smc_hash = &smc_v6_hashinfo,
- .slab_flags = SLAB_TYPESAFE_BY_RCU,
+ .name = "INET6_SMC",
+ .owner = THIS_MODULE,
+ .init = smc_inet_init_sock,
+ .hash = smc_hash_sk,
+ .unhash = smc_unhash_sk,
+ .release_cb = smc_release_cb,
+ .obj_size = sizeof(struct smc6_sock),
+ .h.smc_hash = &smc_v6_hashinfo,
+ .slab_flags = SLAB_TYPESAFE_BY_RCU,
+ .ipv6_pinfo_offset = offsetof(struct smc6_sock, np),
};
static const struct proto_ops smc_inet6_stream_ops = {
--
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/smc: prevent NULL pointer dereference in txopt_get
2024-08-10 17:22 [PATCH net] net/smc: prevent NULL pointer dereference in txopt_get Jeongjun Park
@ 2024-08-13 4:06 ` Jeongjun Park
2024-08-13 8:05 ` Gerd Bayer
1 sibling, 0 replies; 6+ messages in thread
From: Jeongjun Park @ 2024-08-13 4:06 UTC (permalink / raw)
To: aha310510
Cc: alibuda, davem, dust.li, edumazet, guwen, jaka, kuba,
linux-kernel, linux-s390, netdev, pabeni, tonylu, wenjia,
syzbot+f69bfae0a4eb29976e44
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 3988 bytes --]
Jeongjun Park wrote:
>
> Since smc_inet6_prot does not initialize ipv6_pinfo_offset, inet6_create()
> copies an incorrect address value, sk + 0 (offset), to inet_sk(sk)->pinet6.
>
> In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock practically
> point to the same address, when smc_create_clcsk() stores the newly
> created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6 is corrupted
> into clcsock. This causes NULL pointer dereference and various other
> memory corruptions.
>
> To solve this, we need to add a smc6_sock structure for ipv6_pinfo_offset
> initialization and modify the smc_sock structure.
>
> [ 278.629552][T28696] ==================================================================
> [ 278.631367][T28696] BUG: KASAN: null-ptr-deref in txopt_get+0x102/0x430
> [ 278.632724][T28696] Read of size 4 at addr 0000000000000200 by task syz.0.2965/28696
> [ 278.634802][T28696]
> [ 278.635236][T28696] CPU: 0 UID: 0 PID: 28696 Comm: syz.0.2965 Not tainted 6.11.0-rc2 #3
> [ 278.637458][T28696] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> [ 278.639426][T28696] Call Trace:
> [ 278.639833][T28696] <TASK>
> [ 278.640190][T28696] dump_stack_lvl+0x116/0x1b0
> [ 278.640844][T28696] ? txopt_get+0x102/0x430
> [ 278.641620][T28696] kasan_report+0xbd/0xf0
> [ 278.642440][T28696] ? txopt_get+0x102/0x430
> [ 278.643291][T28696] kasan_check_range+0xf4/0x1a0
> [ 278.644163][T28696] txopt_get+0x102/0x430
> [ 278.644940][T28696] ? __pfx_txopt_get+0x10/0x10
> [ 278.645877][T28696] ? selinux_netlbl_socket_setsockopt+0x1d0/0x420
> [ 278.646972][T28696] calipso_sock_getattr+0xc6/0x3e0
> [ 278.647630][T28696] calipso_sock_getattr+0x4b/0x80
> [ 278.648349][T28696] netlbl_sock_getattr+0x63/0xc0
> [ 278.649318][T28696] selinux_netlbl_socket_setsockopt+0x1db/0x420
> [ 278.650471][T28696] ? __pfx_selinux_netlbl_socket_setsockopt+0x10/0x10
> [ 278.652217][T28696] ? find_held_lock+0x2d/0x120
> [ 278.652231][T28696] selinux_socket_setsockopt+0x66/0x90
> [ 278.652247][T28696] security_socket_setsockopt+0x57/0xb0
> [ 278.652278][T28696] do_sock_setsockopt+0xf2/0x480
> [ 278.652289][T28696] ? __pfx_do_sock_setsockopt+0x10/0x10
> [ 278.652298][T28696] ? __fget_files+0x24b/0x4a0
> [ 278.652308][T28696] ? __fget_light+0x177/0x210
> [ 278.652316][T28696] __sys_setsockopt+0x1a6/0x270
> [ 278.652328][T28696] ? __pfx___sys_setsockopt+0x10/0x10
> [ 278.661787][T28696] ? xfd_validate_state+0x5d/0x180
> [ 278.662821][T28696] __x64_sys_setsockopt+0xbd/0x160
> [ 278.663719][T28696] ? lockdep_hardirqs_on+0x7c/0x110
> [ 278.664690][T28696] do_syscall_64+0xcb/0x250
> [ 278.665507][T28696] entry_SYSCALL_64_after_hwframe+0x77/0x7f
> [ 278.666618][T28696] RIP: 0033:0x7fe87ed9712d
> [ 278.667236][T28696] Code: 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa 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 a8 ff ff ff f7 d8 64 89 01 48
> [ 278.670801][T28696] RSP: 002b:00007fe87faa4fa8 EFLAGS: 00000246 ORIG_RAX: 0000000000000036
> [ 278.671832][T28696] RAX: ffffffffffffffda RBX: 00007fe87ef35f80 RCX: 00007fe87ed9712d
> [ 278.672806][T28696] RDX: 0000000000000036 RSI: 0000000000000029 RDI: 0000000000000003
> [ 278.674263][T28696] RBP: 00007fe87ee1bd8a R08: 0000000000000018 R09: 0000000000000000
> [ 278.675967][T28696] R10: 0000000020000000 R11: 0000000000000246 R12: 0000000000000000
> [ 278.677953][T28696] R13: 000000000000000b R14: 00007fe87ef35f80 R15: 00007fe87fa85000
> [ 278.679321][T28696] </TASK>
> [ 278.679917][T28696] ==================================================================
>
Reported-by: syzbot+f69bfae0a4eb29976e44@syzkaller.appspotmail.com
Tested-by: syzbot+f69bfae0a4eb29976e44@syzkaller.appspotmail.com
I think the root cause of this syzbot report and the above bug report is the same.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/smc: prevent NULL pointer dereference in txopt_get
2024-08-10 17:22 [PATCH net] net/smc: prevent NULL pointer dereference in txopt_get Jeongjun Park
2024-08-13 4:06 ` Jeongjun Park
@ 2024-08-13 8:05 ` Gerd Bayer
2024-08-13 8:52 ` D. Wythe
2024-08-13 9:26 ` Jeongjun Park
1 sibling, 2 replies; 6+ messages in thread
From: Gerd Bayer @ 2024-08-13 8:05 UTC (permalink / raw)
To: Jeongjun Park, wenjia, jaka
Cc: alibuda, tonylu, guwen, davem, edumazet, kuba, pabeni, dust.li,
linux-s390, netdev, linux-kernel
On Sun, 2024-08-11 at 02:22 +0900, Jeongjun Park wrote:
> Since smc_inet6_prot does not initialize ipv6_pinfo_offset,
> inet6_create() copies an incorrect address value, sk + 0 (offset), to
> inet_sk(sk)->pinet6.
>
> In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock
> practically point to the same address, when smc_create_clcsk() stores
> the newly created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6
> is corrupted into clcsock. This causes NULL pointer dereference and
> various other memory corruptions.
>
> To solve this, we need to add a smc6_sock structure for
> ipv6_pinfo_offset initialization and modify the smc_sock structure.
I can not argue substantially with that... There's very little IPv6
testing that I'm aware of. But do you really need to move that much
code around and change whitespace for you fix?
[--- snip ---]
> Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> ---
> net/smc/smc.h | 19 ++++++++++---------
> net/smc/smc_inet.c | 24 +++++++++++++++---------
> 2 files changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index 34b781e463c4..f4d9338b5ed5 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -284,15 +284,6 @@ struct smc_connection {
>
> struct smc_sock { /* smc sock
> container */
> struct sock sk;
> - struct socket *clcsock; /* internal tcp
> socket */
> - void (*clcsk_state_change)(struct sock
> *sk);
> - /* original
> stat_change fct. */
> - void (*clcsk_data_ready)(struct sock
> *sk);
> - /* original
> data_ready fct. */
> - void (*clcsk_write_space)(struct sock
> *sk);
> - /* original
> write_space fct. */
> - void (*clcsk_error_report)(struct sock
> *sk);
> - /* original
> error_report fct. */
> struct smc_connection conn; /* smc connection */
> struct smc_sock *listen_smc; /* listen
> parent */
> struct work_struct connect_work; /* handle non-
> blocking connect*/
> @@ -325,6 +316,16 @@ struct smc_sock { /*
> smc sock container */
> /* protects clcsock
> of a listen
> * socket
> * */
> + struct socket *clcsock; /* internal tcp
> socket */
> + void (*clcsk_state_change)(struct sock
> *sk);
> + /* original
> stat_change fct. */
> + void (*clcsk_data_ready)(struct sock
> *sk);
> + /* original
> data_ready fct. */
> + void (*clcsk_write_space)(struct sock
> *sk);
> + /* original
> write_space fct. */
> + void (*clcsk_error_report)(struct sock
> *sk);
> + /* original
> error_report fct. */
> +
> };
>
> #define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
> diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
> index bece346dd8e9..3c54faef6042 100644
> --- a/net/smc/smc_inet.c
> +++ b/net/smc/smc_inet.c
> @@ -60,16 +60,22 @@ static struct inet_protosw smc_inet_protosw = {
> };
>
> #if IS_ENABLED(CONFIG_IPV6)
> +struct smc6_sock {
> + struct smc_sock smc;
> + struct ipv6_pinfo np;
> +};
> +
> static struct proto smc_inet6_prot = {
> - .name = "INET6_SMC",
> - .owner = THIS_MODULE,
> - .init = smc_inet_init_sock,
> - .hash = smc_hash_sk,
> - .unhash = smc_unhash_sk,
> - .release_cb = smc_release_cb,
> - .obj_size = sizeof(struct smc_sock),
> - .h.smc_hash = &smc_v6_hashinfo,
> - .slab_flags = SLAB_TYPESAFE_BY_RCU,
> + .name = "INET6_SMC",
> + .owner = THIS_MODULE,
> + .init = smc_inet_init_sock,
> + .hash = smc_hash_sk,
> + .unhash = smc_unhash_sk,
> + .release_cb = smc_release_cb,
> + .obj_size = sizeof(struct smc6_sock),
> + .h.smc_hash = &smc_v6_hashinfo,
> + .slab_flags = SLAB_TYPESAFE_BY_RCU,
> + .ipv6_pinfo_offset = offsetof(struct smc6_sock, np),
The line above together with the definition of struct smc6_sock seem to
be the only changes relevant to fixing the issue, IMHO.
> };
>
> static const struct proto_ops smc_inet6_stream_ops = {
> --
>
Thanks, Gerd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/smc: prevent NULL pointer dereference in txopt_get
2024-08-13 8:05 ` Gerd Bayer
@ 2024-08-13 8:52 ` D. Wythe
2024-08-13 9:31 ` Jeongjun Park
2024-08-13 9:26 ` Jeongjun Park
1 sibling, 1 reply; 6+ messages in thread
From: D. Wythe @ 2024-08-13 8:52 UTC (permalink / raw)
To: Gerd Bayer, Jeongjun Park, wenjia, jaka
Cc: tonylu, guwen, davem, edumazet, kuba, pabeni, dust.li, linux-s390,
netdev, linux-kernel
On 8/13/24 4:05 PM, Gerd Bayer wrote:
> On Sun, 2024-08-11 at 02:22 +0900, Jeongjun Park wrote:
>> Since smc_inet6_prot does not initialize ipv6_pinfo_offset,
>> inet6_create() copies an incorrect address value, sk + 0 (offset), to
>> inet_sk(sk)->pinet6.
>>
>> In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock
>> practically point to the same address, when smc_create_clcsk() stores
>> the newly created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6
>> is corrupted into clcsock. This causes NULL pointer dereference and
>> various other memory corruptions.
>>
>> To solve this, we need to add a smc6_sock structure for
>> ipv6_pinfo_offset initialization and modify the smc_sock structure.
> I can not argue substantially with that... There's very little IPv6
> testing that I'm aware of. But do you really need to move that much
> code around and change whitespace for you fix?
>
> [--- snip ---]
>
>
>> Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
>> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
>> ---
>> net/smc/smc.h | 19 ++++++++++---------
>> net/smc/smc_inet.c | 24 +++++++++++++++---------
>> 2 files changed, 25 insertions(+), 18 deletions(-)
>>
>> diff --git a/net/smc/smc.h b/net/smc/smc.h
>> index 34b781e463c4..f4d9338b5ed5 100644
>> --- a/net/smc/smc.h
>> +++ b/net/smc/smc.h
>> @@ -284,15 +284,6 @@ struct smc_connection {
>>
>> struct smc_sock { /* smc sock
>> container */
>> struct sock sk;
>> - struct socket *clcsock; /* internal tcp
>> socket */
>> - void (*clcsk_state_change)(struct sock
>> *sk);
>> - /* original
>> stat_change fct. */
>> - void (*clcsk_data_ready)(struct sock
>> *sk);
>> - /* original
>> data_ready fct. */
>> - void (*clcsk_write_space)(struct sock
>> *sk);
>> - /* original
>> write_space fct. */
>> - void (*clcsk_error_report)(struct sock
>> *sk);
>> - /* original
>> error_report fct. */
>> struct smc_connection conn; /* smc connection */
>> struct smc_sock *listen_smc; /* listen
>> parent */
>> struct work_struct connect_work; /* handle non-
>> blocking connect*/
>> @@ -325,6 +316,16 @@ struct smc_sock { /*
>> smc sock container */
>> /* protects clcsock
>> of a listen
>> * socket
>> * */
>> + struct socket *clcsock; /* internal tcp
>> socket */
>> + void (*clcsk_state_change)(struct sock
>> *sk);
>> + /* original
>> stat_change fct. */
>> + void (*clcsk_data_ready)(struct sock
>> *sk);
>> + /* original
>> data_ready fct. */
>> + void (*clcsk_write_space)(struct sock
>> *sk);
>> + /* original
>> write_space fct. */
>> + void (*clcsk_error_report)(struct sock
>> *sk);
>> + /* original
>> error_report fct. */
>> +
>> };
Hi Jeongjun,
I have no problem with this fix, thank you for your assistance.
But, what this here was for ?
>>
>> #define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
>> diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
>> index bece346dd8e9..3c54faef6042 100644
>> --- a/net/smc/smc_inet.c
>> +++ b/net/smc/smc_inet.c
>> @@ -60,16 +60,22 @@ static struct inet_protosw smc_inet_protosw = {
>> };
>>
>> #if IS_ENABLED(CONFIG_IPV6)
>> +struct smc6_sock {
>> + struct smc_sock smc;
>> + struct ipv6_pinfo np;
>> +};
>> +
>> static struct proto smc_inet6_prot = {
>> - .name = "INET6_SMC",
>> - .owner = THIS_MODULE,
>> - .init = smc_inet_init_sock,
>> - .hash = smc_hash_sk,
>> - .unhash = smc_unhash_sk,
>> - .release_cb = smc_release_cb,
>> - .obj_size = sizeof(struct smc_sock),
>> - .h.smc_hash = &smc_v6_hashinfo,
>> - .slab_flags = SLAB_TYPESAFE_BY_RCU,
>> + .name = "INET6_SMC",
>> + .owner = THIS_MODULE,
>> + .init = smc_inet_init_sock,
>> + .hash = smc_hash_sk,
>> + .unhash = smc_unhash_sk,
>> + .release_cb = smc_release_cb,
>> + .obj_size = sizeof(struct smc6_sock),
>> + .h.smc_hash = &smc_v6_hashinfo,
>> + .slab_flags = SLAB_TYPESAFE_BY_RCU,
>> + .ipv6_pinfo_offset = offsetof(struct smc6_sock, np),
Since you have done alignment, why not align the '='.
> The line above together with the definition of struct smc6_sock seem to
> be the only changes relevant to fixing the issue, IMHO.
>
>> };
>>
>> static const struct proto_ops smc_inet6_stream_ops = {
>> --
>>
> Thanks, Gerd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/smc: prevent NULL pointer dereference in txopt_get
2024-08-13 8:05 ` Gerd Bayer
2024-08-13 8:52 ` D. Wythe
@ 2024-08-13 9:26 ` Jeongjun Park
1 sibling, 0 replies; 6+ messages in thread
From: Jeongjun Park @ 2024-08-13 9:26 UTC (permalink / raw)
To: gbayer
Cc: aha310510, alibuda, davem, dust.li, edumazet, guwen, jaka, kuba,
linux-kernel, linux-s390, netdev, pabeni, tonylu, wenjia
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 6357 bytes --]
>
> On Sun, 2024-08-11 at 02:22 +0900, Jeongjun Park wrote:
> > Since smc_inet6_prot does not initialize ipv6_pinfo_offset,
> > inet6_create() copies an incorrect address value, sk + 0 (offset), to
> > inet_sk(sk)->pinet6.
> >
> > In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock
> > practically point to the same address, when smc_create_clcsk() stores
> > the newly created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6
> > is corrupted into clcsock. This causes NULL pointer dereference and
> > various other memory corruptions.
> >
> > To solve this, we need to add a smc6_sock structure for
> > ipv6_pinfo_offset initialization and modify the smc_sock structure.
>
> I can not argue substantially with that... There's very little IPv6
> testing that I'm aware of. But do you really need to move that much
> code around and change whitespace for you fix?
My intention was to add spaces to align the code to the length of the code
added to smc_inet6_prot, but I think I accidentally added a tab character.
Sorry for the confusion. I'll send you the v2 patch right away.
> [--- snip ---]
>
>
> > Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
> > Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> > ---
> > net/smc/smc.h | 19 ++++++++++---------
> > net/smc/smc_inet.c | 24 +++++++++++++++---------
> > 2 files changed, 25 insertions(+), 18 deletions(-)
> >
> > diff --git a/net/smc/smc.h b/net/smc/smc.h
> > index 34b781e463c4..f4d9338b5ed5 100644
> > --- a/net/smc/smc.h
> > +++ b/net/smc/smc.h
> > @@ -284,15 +284,6 @@ struct smc_connection {
> >
> > struct smc_sock { /* smc sock
> > container */
> > struct sock sk;
> > - struct socket *clcsock; /* internal tcp
> > socket */
> > - void (*clcsk_state_change)(struct sock
> > *sk);
> > - /* original
> > stat_change fct. */
> > - void (*clcsk_data_ready)(struct sock
> > *sk);
> > - /* original
> > data_ready fct. */
> > - void (*clcsk_write_space)(struct sock
> > *sk);
> > - /* original
> > write_space fct. */
> > - void (*clcsk_error_report)(struct sock
> > *sk);
> > - /* original
> > error_report fct. */
> > struct smc_connection conn; /* smc connection */
> > struct smc_sock *listen_smc; /* listen
> > parent */
> > struct work_struct connect_work; /* handle non-
> > blocking connect*/
> > @@ -325,6 +316,16 @@ struct smc_sock { /*
> > smc sock container */
> > /* protects clcsock
> > of a listen
> > * socket
> > * */
> > + struct socket *clcsock; /* internal tcp
> > socket */
> > + void (*clcsk_state_change)(struct sock
> > *sk);
> > + /* original
> > stat_change fct. */
> > + void (*clcsk_data_ready)(struct sock
> > *sk);
> > + /* original
> > data_ready fct. */
> > + void (*clcsk_write_space)(struct sock
> > *sk);
> > + /* original
> > write_space fct. */
> > + void (*clcsk_error_report)(struct sock
> > *sk);
> > + /* original
> > error_report fct. */
> > +
> > };
> >
> > #define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
> > diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
> > index bece346dd8e9..3c54faef6042 100644
> > --- a/net/smc/smc_inet.c
> > +++ b/net/smc/smc_inet.c
> > @@ -60,16 +60,22 @@ static struct inet_protosw smc_inet_protosw = {
> > };
> >
> > #if IS_ENABLED(CONFIG_IPV6)
> > +struct smc6_sock {
> > + struct smc_sock smc;
> > + struct ipv6_pinfo np;
> > +};
> > +
> > static struct proto smc_inet6_prot = {
> > - .name = "INET6_SMC",
> > - .owner = THIS_MODULE,
> > - .init = smc_inet_init_sock,
> > - .hash = smc_hash_sk,
> > - .unhash = smc_unhash_sk,
> > - .release_cb = smc_release_cb,
> > - .obj_size = sizeof(struct smc_sock),
> > - .h.smc_hash = &smc_v6_hashinfo,
> > - .slab_flags = SLAB_TYPESAFE_BY_RCU,
> > + .name = "INET6_SMC",
> > + .owner = THIS_MODULE,
> > + .init = smc_inet_init_sock,
> > + .hash = smc_hash_sk,
> > + .unhash = smc_unhash_sk,
> > + .release_cb = smc_release_cb,
> > + .obj_size = sizeof(struct smc6_sock),
> > + .h.smc_hash = &smc_v6_hashinfo,
> > + .slab_flags = SLAB_TYPESAFE_BY_RCU,
> > + .ipv6_pinfo_offset = offsetof(struct smc6_sock, np),
>
> The line above together with the definition of struct smc6_sock seem to
> be the only changes relevant to fixing the issue, IMHO.
However, modifying the smc_sock structure is absolutely necessary. This is
because smc_sk(sk)->clcsock and inet_sk(sk)->pinet6 point to the same
address in the current smc_sock structure definition, so when
smc_create_clcsk() is called from inet6_create(), the already initialized
inet_sk(sk)->pinet6 will be overwritten by clcsock.
>
> > };
> >
> > static const struct proto_ops smc_inet6_stream_ops = {
> > --
> >
>
> Thanks, Gerd
Regards,
Jeongjun Park
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/smc: prevent NULL pointer dereference in txopt_get
2024-08-13 8:52 ` D. Wythe
@ 2024-08-13 9:31 ` Jeongjun Park
0 siblings, 0 replies; 6+ messages in thread
From: Jeongjun Park @ 2024-08-13 9:31 UTC (permalink / raw)
To: alibuda
Cc: aha310510, davem, dust.li, edumazet, gbayer, guwen, jaka, kuba,
linux-kernel, linux-s390, netdev, pabeni, tonylu, wenjia
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 6265 bytes --]
D. Wythe wrote:
> On 8/13/24 4:05 PM, Gerd Bayer wrote:
> > On Sun, 2024-08-11 at 02:22 +0900, Jeongjun Park wrote:
> >> Since smc_inet6_prot does not initialize ipv6_pinfo_offset,
> >> inet6_create() copies an incorrect address value, sk + 0 (offset), to
> >> inet_sk(sk)->pinet6.
> >>
> >> In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock
> >> practically point to the same address, when smc_create_clcsk() stores
> >> the newly created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6
> >> is corrupted into clcsock. This causes NULL pointer dereference and
> >> various other memory corruptions.
> >>
> >> To solve this, we need to add a smc6_sock structure for
> >> ipv6_pinfo_offset initialization and modify the smc_sock structure.
> > I can not argue substantially with that... There's very little IPv6
> > testing that I'm aware of. But do you really need to move that much
> > code around and change whitespace for you fix?
> >
> > [--- snip ---]
> >
> >
> >> Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
> >> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> >> ---
> >> net/smc/smc.h | 19 ++++++++++---------
> >> net/smc/smc_inet.c | 24 +++++++++++++++---------
> >> 2 files changed, 25 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/net/smc/smc.h b/net/smc/smc.h
> >> index 34b781e463c4..f4d9338b5ed5 100644
> >> --- a/net/smc/smc.h
> >> +++ b/net/smc/smc.h
> >> @@ -284,15 +284,6 @@ struct smc_connection {
> >>
> >> struct smc_sock { /* smc sock
> >> container */
> >> struct sock sk;
> >> - struct socket *clcsock; /* internal tcp
> >> socket */
> >> - void (*clcsk_state_change)(struct sock
> >> *sk);
> >> - /* original
> >> stat_change fct. */
> >> - void (*clcsk_data_ready)(struct sock
> >> *sk);
> >> - /* original
> >> data_ready fct. */
> >> - void (*clcsk_write_space)(struct sock
> >> *sk);
> >> - /* original
> >> write_space fct. */
> >> - void (*clcsk_error_report)(struct sock
> >> *sk);
> >> - /* original
> >> error_report fct. */
> >> struct smc_connection conn; /* smc connection */
> >> struct smc_sock *listen_smc; /* listen
> >> parent */
> >> struct work_struct connect_work; /* handle non-
> >> blocking connect*/
> >> @@ -325,6 +316,16 @@ struct smc_sock { /*
> >> smc sock container */
> >> /* protects clcsock
> >> of a listen
> >> * socket
> >> * */
> >> + struct socket *clcsock; /* internal tcp
> >> socket */
> >> + void (*clcsk_state_change)(struct sock
> >> *sk);
> >> + /* original
> >> stat_change fct. */
> >> + void (*clcsk_data_ready)(struct sock
> >> *sk);
> >> + /* original
> >> data_ready fct. */
> >> + void (*clcsk_write_space)(struct sock
> >> *sk);
> >> + /* original
> >> write_space fct. */
> >> + void (*clcsk_error_report)(struct sock
> >> *sk);
> >> + /* original
> >> error_report fct. */
> >> +
> >> };
>
> Hi Jeongjun,
>
> I have no problem with this fix, thank you for your assistance.
> But, what this here was for ?
Sorry for the confusion. It looks like the tab character was accidentally
used. We will fix that and send you a v2 patch.
>
>
> >>
> >> #define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
> >> diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
> >> index bece346dd8e9..3c54faef6042 100644
> >> --- a/net/smc/smc_inet.c
> >> +++ b/net/smc/smc_inet.c
> >> @@ -60,16 +60,22 @@ static struct inet_protosw smc_inet_protosw = {
> >> };
> >>
> >> #if IS_ENABLED(CONFIG_IPV6)
> >> +struct smc6_sock {
> >> + struct smc_sock smc;
> >> + struct ipv6_pinfo np;
> >> +};
> >> +
> >> static struct proto smc_inet6_prot = {
> >> - .name = "INET6_SMC",
> >> - .owner = THIS_MODULE,
> >> - .init = smc_inet_init_sock,
> >> - .hash = smc_hash_sk,
> >> - .unhash = smc_unhash_sk,
> >> - .release_cb = smc_release_cb,
> >> - .obj_size = sizeof(struct smc_sock),
> >> - .h.smc_hash = &smc_v6_hashinfo,
> >> - .slab_flags = SLAB_TYPESAFE_BY_RCU,
> >> + .name = "INET6_SMC",
> >> + .owner = THIS_MODULE,
> >> + .init = smc_inet_init_sock,
> >> + .hash = smc_hash_sk,
> >> + .unhash = smc_unhash_sk,
> >> + .release_cb = smc_release_cb,
> >> + .obj_size = sizeof(struct smc6_sock),
> >> + .h.smc_hash = &smc_v6_hashinfo,
> >> + .slab_flags = SLAB_TYPESAFE_BY_RCU,
> >> + .ipv6_pinfo_offset = offsetof(struct smc6_sock, np),
>
> Since you have done alignment, why not align the '='.
>
> > The line above together with the definition of struct smc6_sock seem to
> > be the only changes relevant to fixing the issue, IMHO.
> >
> >> };
> >>
> >> static const struct proto_ops smc_inet6_stream_ops = {
> >> --
> >>
> > Thanks, Gerd
Regards,
Jeongjun Park
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-08-13 9:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-10 17:22 [PATCH net] net/smc: prevent NULL pointer dereference in txopt_get Jeongjun Park
2024-08-13 4:06 ` Jeongjun Park
2024-08-13 8:05 ` Gerd Bayer
2024-08-13 8:52 ` D. Wythe
2024-08-13 9:31 ` Jeongjun Park
2024-08-13 9:26 ` Jeongjun Park
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).