netdev - Re: [PATCH net] 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: <9102add11cb13e94d3d798290e7d08145e8a6af9.camel@linux.ibm.com>
Date: Tue, 13 Aug 2024 10:05:26 +0200
From: Gerd Bayer <gbayer@...ux.ibm.com>
To: Jeongjun Park <aha310510@...il.com>, wenjia@...ux.ibm.com,
        jaka@...ux.ibm.com
Cc: alibuda@...ux.alibaba.com, tonylu@...ux.alibaba.com,
        guwen@...ux.alibaba.com, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, dust.li@...ux.alibaba.com,
        linux-s390@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net/smc: prevent NULL pointer dereference in
 txopt_get

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?

[--- snip ---]


> Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
> Signed-off-by: Jeongjun Park <aha310510@...il.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.

>  };
>  
>  static const struct proto_ops smc_inet6_stream_ops = {
> --
> 

Thanks, Gerd