| 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
| ||
|
Message-ID: <56255393-cae8-4cdf-9c91-b8ddf0bd2de2@linux.alibaba.com>
Date: Wed, 14 Aug 2024 10:42:47 +0800
From: "D. Wythe" <alibuda@...ux.alibaba.com>
To: Jeongjun Park <aha310510@...il.com>
Cc: wenjia@...ux.ibm.com, jaka@...ux.ibm.com, gbayer@...ux.ibm.com,
tonylu@...ux.alibaba.com, guwen@...ux.alibaba.com, davem@...emloft.net,
dust.li@...ux.alibaba.com, edumazet@...gle.com, pabeni@...hat.com,
kuba@...nel.org, linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
netdev@...r.kernel.org, syzbot+f69bfae0a4eb29976e44@...kaller.appspotmail.com
Subject: Re: [PATCH net,v3] net/smc: prevent NULL pointer dereference in
txopt_get
On 8/14/24 10:25 AM, D. Wythe wrote:
>
>
> On 8/13/24 7:48 PM, Jeongjun Park wrote:
>> D. Wythe wrote:
>>>
>>>
>>> On 8/13/24 6:07 PM, 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.
>>>>
>>>> Reported-by: syzbot+f69bfae0a4eb29976e44@...kaller.appspotmail.com
>>>> Tested-by: syzbot+f69bfae0a4eb29976e44@...kaller.appspotmail.com
>>>> Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
>>>> Signed-off-by: Jeongjun Park <aha310510@...il.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..25f34fd65e8d 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;
>>>> +};
>>> I prefer to:
>>>
>>> struct ipv6_pinfo inet6;
>> Okay, I'll write a v4 patch and send it to you tomorrow.
>>
>> Regards,
>> Jeongjun Park
>
> Before you issue the v4, I still don't know why you move clcsk_xxx
> from smc_connection
> to smc_sock, can you explain it ?
I misread it, it seems you're moving them from head to tail, but still,
the same question,
why move it ?
Thanks
D. Wythe
>
> Also, regarding alignment, it's okay for me whether it's aligned or
> not，But I checked the styles of other types of
> structures and did not strictly require alignment, so I now feel that
> there is no need to
> modify so much to do alignment.
>
> D. Wythe
>
>>
>>>> +
>>>> 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 = {
>>>> --
>
Powered by blists - more mailing lists