netdev - Re: [PATCH net,v3] 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] [day] [month] [year] [list]

Message-ID: <CAO9qdTG=aspVkmB9zSh1x-5QLc-FBkxsnPfVErPVmCR3saCe9A@mail.gmail.com>
Date: Wed, 14 Aug 2024 19:37:20 +0900
From: Jeongjun Park <aha310510@...il.com>
To: "D. Wythe" <alibuda@...ux.alibaba.com>
Cc: 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, 
	syzbot+f69bfae0a4eb29976e44@...kaller.appspotmail.com, 
	tonylu@...ux.alibaba.com, wenjia@...ux.ibm.com
Subject: Re: [PATCH net,v3] net/smc: prevent NULL pointer dereference in txopt_get

2024년 8월 14일 (수) 오후 3:00, D. Wythe <alibuda@...ux.alibaba.com>님이 작성:
>
>
>
> On 8/14/24 11:58 AM, Jeongjun Park wrote:
> > Because clcsk_*, like clcsock, is initialized during the smc init process,
> > the code was moved to prevent clcsk_* from having an address like
> > inet_sk(sk)->pinet6, thereby preventing the previously initialized values
> > from being tampered with.
>
> I don't agree with your approach, but I finally got the problem you
> described. In fact, the issue here is that smc_sock should also be an
> inet_sock, whereas currently it's just a sock. Therefore, the best
> solution would be to embed an inet_sock within smc_sock rather than
> performing this movement as you suggested.
>
> struct smc_sock {               /* smc sock container */
>      union {
>          struct sock         sk;
>          struct inet_sock    inet;
>      };
>
>      ...
>
> Thanks.
> D. Wythe
>

I tested it myself and it doesn't trigger the existing issue, so
I'll write a v4 patch with this code and send it to you.

Regards,
Jeongjun Park

>
> >
> > Additionally, if you don't need alignment in smc_inet6_prot , I'll modify
> > the patch to only add the necessary code without alignment.
> >
> > Regards,
> > Jeongjun Park
>
>
> >>
> >>> 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 = {
> >>>>>> --
>