Re: [PATCH net] net/smc: prevent NULL pointer dereference in txopt_get

[Jeongjun Park <aha310510@gmail.com>]

[-- 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