linux-kernel - Re: [PATCH net] crypto: chtls: Fix possible null pointer dereferences

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]

Date: Thu, 11 Apr 2024 08:20:23 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Aleksandr Mishin <amishin@...rgos.ru>, Atul Gupta
 <atul.gupta@...lsio.com>
Cc: Ayush Sawal <ayush.sawal@...lsio.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, David Ahern <dsahern@...nel.org>, Michael Werner
 <werner@...lsio.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
  lvc-project@...uxtesting.org
Subject: Re: [PATCH net] crypto: chtls: Fix possible null pointer
 dereferences

On Tue, 2024-04-09 at 12:05 +0300, Aleksandr Mishin wrote:
> In chtls_pass_accept_rpl() and chtls_select_mss() __sk_dst_get() may
> return NULL which is later dereferenced. Fix this bug by adding NULL check.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

Indeed a more describing changelog would be better given the patch
needs to go much fourther then adding a missing check.

> Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
> Signed-off-by: Aleksandr Mishin <amishin@...rgos.ru>
> ---
>  .../chelsio/inline_crypto/chtls/chtls_cm.c    | 24 +++++++++++++------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> index 6f6525983130..6d88cbc9fbb0 100644
> --- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> +++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> @@ -939,15 +939,15 @@ static void chtls_accept_rpl_arp_failure(void *handle,
>  	sock_put(sk);
>  }
>  
> -static unsigned int chtls_select_mss(const struct chtls_sock *csk,
> +static bool chtls_select_mss(const struct chtls_sock *csk,
>  				     unsigned int pmtu,
> -				     struct cpl_pass_accept_req *req)
> +				     struct cpl_pass_accept_req *req,
> +					 unsigned int *mtu_idx)

Bad indentation above.

More importantly, what about returning a negative value on failure and
avoid the additional parameter? It should also generate a smaller
diffstat.

>  {
>  	struct chtls_dev *cdev;
>  	struct dst_entry *dst;
>  	unsigned int tcpoptsz;
>  	unsigned int iphdrsz;
> -	unsigned int mtu_idx;
>  	struct tcp_sock *tp;
>  	unsigned int mss;
>  	struct sock *sk;
> @@ -955,6 +955,9 @@ static unsigned int chtls_select_mss(const struct chtls_sock *csk,
>  	mss = ntohs(req->tcpopt.mss);
>  	sk = csk->sk;
>  	dst = __sk_dst_get(sk);
> +	if (!dst)
> +		return false;
> +
>  	cdev = csk->cdev;
>  	tp = tcp_sk(sk);
>  	tcpoptsz = 0;

[...]

>  static unsigned int select_rcv_wscale(int space, int wscale_ok, int win_clamp)
> @@ -1016,8 +1019,13 @@ static void chtls_pass_accept_rpl(struct sk_buff *skb,
>  	struct sock *sk;
>  	u32 opt2, hlen;
>  	u64 opt0;
> +	struct dst_entry *dst;

Please respect the reverst x-mass tree order.

>  	sk = skb->sk;
> +	dst = __sk_dst_get(sk);
> +	if (!dst)
> +		return;
> +
>  	tp = tcp_sk(sk);
>  	csk = sk->sk_user_data;
>  	csk->tid = tid;

Thanks,

Paolo