| 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: <20240813093147.175682-1-aha310510@gmail.com>
Date: Tue, 13 Aug 2024 18:31:47 +0900
From: Jeongjun Park <aha310510@...il.com>
To: alibuda@...ux.alibaba.com
Cc: aha310510@...il.com,
davem@...emloft.net,
dust.li@...ux.alibaba.com,
edumazet@...gle.com,
gbayer@...ux.ibm.com,
guwen@...ux.alibaba.com,
jaka@...ux.ibm.com,
kuba@...nel.org,
linux-kernel@...r.kernel.org,
linux-s390@...r.kernel.org,
netdev@...r.kernel.org,
pabeni@...hat.com,
tonylu@...ux.alibaba.com,
wenjia@...ux.ibm.com
Subject: Re: [PATCH net] net/smc: prevent NULL pointer dereference in txopt_get
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@...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. */
> >> +
> >> };
>
> 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
Powered by blists - more mailing lists