Received-SPF: pass (google.com: domain of linux-kernel+bounces-137744-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <5cd26a3a-4cca-4a35-8a4e-22d1d5dad945@intel.com>
Date: Tue, 9 Apr 2024 17:08:54 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net] crypto: chtls: Fix possible null pointer dereferences
To: Aleksandr Mishin <amishin@t-argos.ru>, Atul Gupta <atul.gupta@chelsio.com>
CC: Ayush Sawal <ayush.sawal@chelsio.com>, "David S. Miller"
	<davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski
	<kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, David Ahern
	<dsahern@kernel.org>, Michael Werner <werner@chelsio.com>,
	<netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<lvc-project@linuxtesting.org>
References: <20240409090507.21441-1-amishin@t-argos.ru>
Content-Language: en-US
From: Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20240409090507.21441-1-amishin@t-argos.ru>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?N0thZTRuUUlaRjF0TndIaHVqdDlhNWVubUV6ZzJyb0ZsNHIydnR2Y0RBVDZP?=
 =?utf-8?B?RlpKSWxESExkTlNiNEM3YlVlNFpoTFJLSE85dXdBQnpYaEUyRzd2MFpXTFhX?=
 =?utf-8?B?UlQzMjVZa2t0Q0wzSzUrb3BEOHpNWnJ3aTNUWDdrMURXZFlZQXdqTEx5c0R4?=
 =?utf-8?B?dEtUSDQyZEhxVURjbzRDVnJ2Um9vQVk0dEg4Qk5SYXlLRDFLTEJ1ckorUExh?=
 =?utf-8?B?c2E1d0xQSDQ2R2pvYUp4bzQ1cmJaRXFDMnY5RWJDaklMbjZrYWxrNmZ4RG1o?=
 =?utf-8?B?VS9SQzIzaVJIQ2lSZGY0T2tabTYrQ1hURVhnMGsxWjBmejhQM09scUZNaVo3?=
 =?utf-8?B?dnVwUjVGRUNhdWM2K1B5MisxeFVEVWVtOVdmMEM4aksrTEpzY0EyejFVUmN4?=
 =?utf-8?B?QmwxT2lyRFNCQVJ4VnJFUHA4RHVtR3loa2JDQmdrZ1EvTUJyeGNpUk9DR1Fz?=
 =?utf-8?B?bENyYzBCRlIyeHUrWUpXMGJrbWhKRkpDMi9CYkNqZU5EMHdJTEhPUGxyWnd5?=
 =?utf-8?B?NUlHWXlWS2wyYkJBSjVJeS9hMGV4Tmh1UkNDSnlTOC9MRVowWmcwVEJRYkll?=
 =?utf-8?B?S0t2eHZJdi9FRCs1WUZLZHQyQVZzWEtPejlFakdONlI1SUJCUUN6ak9FVXFB?=
 =?utf-8?B?MDRvY0N2elFBM3FKQURWRnRueWdudW15c3BvUWtCU1ExbVdrSEFGanZvSHJs?=
 =?utf-8?B?UzRaRFgvSHBuR0xRYXh6T1FGRCtEeUNEY1FtdVJrUkE4YjF3WFhzU2NncUha?=
 =?utf-8?B?VmtMRlJ3cXpyWk5vQ2w0d21EZEhkS1RqcUhuUUxLU2J6d0hrVDFVcDJ4Q2Nh?=
 =?utf-8?B?NG41OVRFS3FxVUVnb01CZFJ0aWQxNDJlcU1abmVxcldWenVyOW1BYlp3S3N1?=
 =?utf-8?B?VVIxeGcxUjg1ZjBobXRQcDNhMjdrVHNOV2d2andkcTNVTWo4SzgyRnFseUtt?=
 =?utf-8?B?YXEwL1EwVW5Xd0xXVHBtLytneFcxc01KeGczY3JkTTR5SHgzOTJjb24zYjdY?=
 =?utf-8?B?WVRZd05aempBa1JyZm1pcVluYW5hV29wa1Erb2dyR09sempBMGtqZkhKb203?=
 =?utf-8?B?bnErWW9PTGU2YXBReWZWTkF1dmxWOGdpUXI5YmgxTFVNbVZFQUFIdW5rclZD?=
 =?utf-8?B?a3pVbUgrUVk2MW9VMjAzR0RmTEhBMktNam5qaFZMNjdYQVF3bjVwWHFseG12?=
 =?utf-8?B?aEh4NFFadVB4eVRybVlOZHdNNm1Fc2J1N1I5amZHQVI5M0EzY1h0Wjc5eWpa?=
 =?utf-8?B?SFhOYXRYalRlSldnaHZaakNrU3VMWVo1dHFJOFJUVHorYWU5WXVXckdsOU1S?=
 =?utf-8?B?SWZ2Q1MxOUt5K1cyVVArMEN4bHNRNnRZWVBJbWZ0dWlZRVIzV3dETFozdGJ1?=
 =?utf-8?B?NGgzMFArUVphU3VtdVpaZjhCbU1icCtrN0M3L2k3V0JkUlVXTVpnanozZktF?=
 =?utf-8?B?R2g5SDVCbVF5L2ZFaU1OK2FYRHlrTVBnZ3ZkaVRpZ2lFMHNNK1lVT0hWQ25D?=
 =?utf-8?B?Uk5Kc3FZcDFSNUQ5SzBxdGcvSklVSk44b1IxcE0vK1kxR2dzNHFIVlBCcE9h?=
 =?utf-8?B?aUJFQk1rQUJWWmsyMzg2cHhqMVVOQ0hQd2N5akpOTXkrMUkyQUt2aXE3RjJJ?=
 =?utf-8?B?Rjg5U3FzNm9oSUN4cE1CVFo4TWpadzd2R3NqVkdJRzgwQkZ1cE42djVHUnJ0?=
 =?utf-8?B?Y2ZyVVdjUHhiMC9obzVYbXl3U2FnZ01XLzZ6c0xIQlVkQk9ESmFoWkZYeFov?=
 =?utf-8?B?ZnppcXJkNE1LV0lxV25wRWFHRVdpUmpGWHZuNmM1azNGeVREWjdoMncwM0R1?=
 =?utf-8?B?dEp1Wk1kSkVZcFBQbTRtZEwyL01YZG5QUytMQktKOHFjMy9LWllzamhieGF6?=
 =?utf-8?B?SHd0WVViQWRhclk2OW9Pc25OZERtWmNDdTAwS29PZjIrd2N0bGpXQkhKa0xY?=
 =?utf-8?B?eEVUUEY4ZkFjekVudmg0Q3ZHT3dYdTFhaXNsOWN1Z0FKTlFmMWZXM3liTXdx?=
 =?utf-8?B?emltL3ROcFU5YVhTbUVuSmZTVmgxK1VzRzNrbis0a0VQejhnM1ByRlA1OUFE?=
 =?utf-8?B?OGg3OHRDU0ZibXJlQlVJekRXa2tuNWZyOHgyUjUybk85ZDRjOGNkQWhOM2Vk?=
 =?utf-8?B?emNkSGxJVXZnRi9RYlhWYUFmeWs3R0t2NVd1Tmp6UFE3MjlpdGh2eFlQcmxk?=
 =?utf-8?B?bnc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bf22c7ca-8daa-4805-966d-08dc58f2698f
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Apr 2024 00:08:55.7008
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: TE3DvZH7r2mehnOmNQzG3tQWfjq/i70DWbxJmyuAyFLxg8yHQXXotApY0QCgIiZW/qTMEeCTL2GTdQSKA1KFp0I0Vr9yu9quqS+tJVYX6Mg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7353
X-OriginatorOrg: intel.com


On 4/9/2024 2:05 AM, 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.
> 
> Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>

This seems to do a lot more than simply adding a NULL check, though I
guess thats because it needs to pass the return out somehow properly? I
would have liked more explanation of the requires changes.


> ---
>  .../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)
>  {
>  	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;
> @@ -979,11 +982,11 @@ static unsigned int chtls_select_mss(const struct chtls_sock *csk,
>  	tp->advmss = cxgb4_best_aligned_mtu(cdev->lldi->mtus,
>  					    iphdrsz + tcpoptsz,
>  					    tp->advmss - tcpoptsz,
> -					    8, &mtu_idx);
> +					    8, mtu_idx);
>  	tp->advmss -= iphdrsz;
>  
>  	inet_csk(sk)->icsk_pmtu_cookie = pmtu;
> -	return mtu_idx;
> +	return true;
>  }
>  
>  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;
>  
>  	sk = skb->sk;
> +	dst = __sk_dst_get(sk);
> +	if (!dst)
> +		return;
> +
>  	tp = tcp_sk(sk);
>  	csk = sk->sk_user_data;
>  	csk->tid = tid;
> @@ -1029,8 +1037,10 @@ static void chtls_pass_accept_rpl(struct sk_buff *skb,
>  
>  	OPCODE_TID(rpl5) = cpu_to_be32(MK_OPCODE_TID(CPL_PASS_ACCEPT_RPL,
>  						     csk->tid));
> -	csk->mtu_idx = chtls_select_mss(csk, dst_mtu(__sk_dst_get(sk)),
> -					req);
> +	if (!chtls_select_mss(csk, dst_mtu(dst),
> +					req, &csk->mtu_idx))
> +		return;
> +
>  	opt0 = TCAM_BYPASS_F |
>  	       WND_SCALE_V(RCV_WSCALE(tp)) |
>  	       MSS_IDX_V(csk->mtu_idx) |