linux-kernel - Re: [PATCH net,v5,2/2] net/smc: modify smc

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: <20240827035005.159504-1-aha310510@gmail.com>
Date: Tue, 27 Aug 2024 12:50:05 +0900
From: Jeongjun Park <aha310510@...il.com>
To: jaka@...ux.ibm.com
Cc: aha310510@...il.com,
	alibuda@...ux.alibaba.com,
	davem@...emloft.net,
	dust.li@...ux.alibaba.com,
	edumazet@...gle.com,
	guwen@...ux.alibaba.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,
	ubraun@...ux.vnet.ibm.com,
	utz.bacher@...ibm.com,
	wenjia@...ux.ibm.com
Subject: Re: [PATCH net,v5,2/2] net/smc: modify smc_sock structure

Jan Karcher wrote:
>
>
> On 26/08/2024 04:56, D. Wythe wrote:
> >
> >
> > On 8/22/24 3:19 PM, Jan Karcher wrote:
> >>
> >>
> >> On 21/08/2024 13:06, Jeongjun Park wrote:
> >>> Jan Karcher wrote:
> >>>>
> >>>>
> >>
> >> [...]
> >>
> >>>>
> >>>> If so would you mind adding a helper for this check as Paolo suggested
> >>>> and send it?
> >>>> This way we see which change is better for the future.
> >>>
> >>> This is the patch I tested. Except for smc.h and smc_inet.c, the rest is
> >>> just a patch that changes smc->sk to smc->inet.sk. When I tested using
> >>> this patch and c repro, the vulnerability was not triggered.
> >>>
> >>> Regards,
> >>> Jeongjun Park
> >>
> >> Thank you for providing your changes. TBH, I do like only having the
> >> inet socket in our structure.
> >> I did not review it completley since there are, obviously, a lot of
> >> changes.
> >> Testing looks good so far but needs some more time.
> >>
> >> @D. Wythe are there any concerns from your side regarding this solution?
> >>
> >> Thanks,
> >> Jan
> >>
> >
> > Well, I really don't think this is a good idea. As we've mentioned, for
> > AF_SMC, smc_sock should not be treated as inet_sock.
> > While in terms of actual running logic, this approach yields the same
> > result as using a union, but the use of a union clearly indicates
> > that it includes two distinct types of socks.
>
> Fair. I understand both sides here and i do not have a strong opinion.
> One is kinda implicit, the other defines fields we do not use...
> Of course there would be a compromise to define another struct something
> like this:
>
> struct smc_sock_types {
>         struct sock             sk;
>         #if IS_ENABLED(CONFIG_IPV6)
>                 struct ipv6_pinfo       *pinet6;
>         #endif
> };
>
> struct smc_sock {                               /* smc sock container */
>         struct smc_sock_types   socks;
> [...]

If absolutely must use the sock structure in smc_sock, I think it would 
be okay to modify it like the patch below to avoid a lot of code m
odifications.

---
 net/smc/smc.h      | 3 +++
 net/smc/smc_inet.c | 8 +++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/smc/smc.h b/net/smc/smc.h
index 34b781e463c4..ad77d6b6b8d3 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -284,6 +284,9 @@ struct smc_connection {
 
 struct smc_sock {				/* smc sock container */
 	struct sock		sk;
+#if IS_ENABLED(CONFIG_IPV6)
+	struct ipv6_pinfo	*pinet6;
+#endif
 	struct socket		*clcsock;	/* internal tcp socket */
 	void			(*clcsk_state_change)(struct sock *sk);
 						/* original stat_change fct. */
diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
index bece346dd8e9..a5b2041600f9 100644
--- a/net/smc/smc_inet.c
+++ b/net/smc/smc_inet.c
@@ -60,6 +60,11 @@ static struct inet_protosw smc_inet_protosw = {
 };
 
 #if IS_ENABLED(CONFIG_IPV6)
+struct smc6_sock {
+	struct smc_sock		smc;
+	struct ipv6_pinfo	inet6;
+};
+
 static struct proto smc_inet6_prot = {
 	.name		= "INET6_SMC",
 	.owner		= THIS_MODULE,
@@ -67,9 +72,10 @@ static struct proto smc_inet6_prot = {
 	.hash		= smc_hash_sk,
 	.unhash		= smc_unhash_sk,
 	.release_cb	= smc_release_cb,
-	.obj_size	= sizeof(struct smc_sock),
+	.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, inet6),
 };
 
 static const struct proto_ops smc_inet6_stream_ops = {
--

Regards,
Jeongjun Park

>
> That said, don't know if i like this either.
>
> Thanks
> - Jan
>
> >
> > Also, if you have to make this change, perhaps you can give it a try
> >
> > #define smc->sk smc->inet.sk
> >
> > This will save lots of modifications.
> >
> > Thanks,
> > D. Wythe
> >
> >>>
> >>>>
> >>>> The statement that SMC would be more aligned with other AFs is
> >>>> already a
> >>>>    big win in my book.
> >>>>
> >>>> Thanks
> >>>> - Jan
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>> Paolo
> >>>>>
> >>>