linux-kernel - Re: [PATCH net,v4] net/smc: prevent NULL pointer dereference in txopt

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAO9qdTGFGxgD_8RYQKTx9NJbwa0fiFziFyx2FJpnYk3ZvFbUmw@mail.gmail.com>
Date: Thu, 15 Aug 2024 12:15:14 +0900
From: Jeongjun Park <aha310510@...il.com>
To: "D. Wythe" <alibuda@...ux.alibaba.com>
Cc: wintera@...ux.ibm.com, gbayer@...ux.ibm.com, guwen@...ux.alibaba.com, 
	jaka@...ux.ibm.com, tonylu@...ux.alibaba.com, wenjia@...ux.ibm.com, 
	davem@...emloft.net, dust.li@...ux.alibaba.com, edumazet@...gle.com, 
	kuba@...nel.org, linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org, 
	netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH net,v4] net/smc: prevent NULL pointer dereference in txopt_get

2024년 8월 15일 (목) 오전 11:51, D. Wythe <alibuda@...ux.alibaba.com>님이 작성:
>
>
>
> On 8/14/24 11:05 PM, Jeongjun Park wrote:
> > Alexandra Winter wrote:
> >> On 14.08.24 15:11, D. Wythe wrote:
> >>>      struct smc_sock {                /* smc sock container */
> >>> -    struct sock        sk;
> >>> +    union {
> >>> +        struct sock        sk;
> >>> +        struct inet_sock    inet;
> >>> +    };
> >>
> >> I don't see a path where this breaks, but it looks risky to me.
> >> Is an smc_sock always an inet_sock as well? Then can't you go with smc_sock->inet_sock->sk ?
> >> Or only in the IPPROTO SMC case, and in the AF_SMC case it is not an inet_sock?
>
>
> There is no smc_sock->inet_sock->sk before. And this part here was to
> make smc_sock also
> be an inet_sock.
>
> For IPPROTO_SMC, smc_sock should be an inet_sock, but it is not before.
> So, the initialization of certain fields
> in smc_sock(for example, clcsk) will overwrite modifications made to the
> inet_sock part in inet(6)_create.
>
> For AF_SMC,  the only problem is that  some space will be wasted. Since
> AF_SMC don't care the inet_sock part.
> However, make the use of sock by AF_SMC and IPPROTO_SMC separately for
> the sake of avoid wasting some space
> is a little bit extreme.
>

Okay. I think using inet_sock instead of sock is also a good idea, but I
understand for now.

However, for some reason this patch status has become Changes Requested
, so we will split the patch into two and resend the v5 patch.

Regards,
Jeongjun Park

>
> > hmm... then how about changing it to something like this?
> >
> > @@ -283,7 +283,7 @@ struct smc_connection {
> >   };
> >
> >   struct smc_sock {                           /* smc sock container */
> > -     struct sock             sk;
> > +     struct inet_sock        inet;
> >       struct socket           *clcsock;       /* internal tcp socket */
> >       void                    (*clcsk_state_change)(struct sock *sk);
>
>
> Don't.
>
> >                                               /* original stat_change fct. */
> > @@ -327,7 +327,7 @@ struct smc_sock {                         /* smc sock container */
> >                                                * */
> >   };
> >
> > -#define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
> > +#define smc_sk(ptr) container_of_const(ptr, struct smc_sock, inet.sk)
> >
> >   static inline void smc_init_saved_callbacks(struct smc_sock *smc)
> >   {
> >
> > It is definitely not normal to make the first member of smc_sock as sock.
> >
> > Therefore, I think it would be appropriate to modify it to use inet_sock
> > as the first member like other protocols (sctp, dccp) and access sk in a
> > way like &smc->inet.sk.
> >
> > Although this fix would require more code changes, we tested the bug and
> > confirmed that it was not triggered and the functionality was working
> > normally.
> >
> > What do you think?
> >
> > Regards,
> > Jeongjun Park
>