>
> 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@xxxxxxxxx>
> > ---
> > 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
