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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20240814150558.46178-1-aha310510@gmail.com>
Date: Thu, 15 Aug 2024 00:05:58 +0900
From: Jeongjun Park <aha310510@...il.com>
To: wintera@...ux.ibm.com,
	alibuda@...ux.alibaba.com,
	gbayer@...ux.ibm.com,
	guwen@...ux.alibaba.com,
	jaka@...ux.ibm.com,
	tonylu@...ux.alibaba.com,
	wenjia@...ux.ibm.com
Cc: 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

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?

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);
 						/* 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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ