netdev - Re: [PATCH] crypto: chtls: Add check alloc

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: <20250117132914.GM6206@kernel.org>
Date: Fri, 17 Jan 2025 13:29:14 +0000
From: Simon Horman <horms@...nel.org>
To: Charles Han <hanchunchao@...pur.com>
Cc: ayush.sawal@...lsio.com, andrew+netdev@...n.ch, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	atul.gupta@...lsio.com, werner@...lsio.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] crypto: chtls: Add check alloc_skb() returned value

On Fri, Jan 17, 2025 at 11:13:28AM +0800, Charles Han wrote:
> alloc_skb() can return a NULL pointer on failure.But these returned
> value in send_defer_abort_rpl() and chtls_close_conn()  not checked.
> 
> Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
> Signed-off-by: Charles Han <hanchunchao@...pur.com>
> ---
>  .../net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c  | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> 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..725cce34f25a 100644
> --- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> +++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> @@ -306,6 +306,10 @@ static void chtls_close_conn(struct sock *sk)
>  	tid = csk->tid;
>  
>  	skb = alloc_skb(len, GFP_KERNEL | __GFP_NOFAIL);
> +	if (!skb) {
> +		pr_warn("%s: cannot allocate skb!\n", __func__);
> +		return;
> +	}
>  	req = (struct cpl_close_con_req *)__skb_put(skb, len);
>  	memset(req, 0, len);
>  	req->wr.wr_hi = htonl(FW_WR_OP_V(FW_TP_WR) |
> @@ -1991,6 +1995,11 @@ static void send_defer_abort_rpl(struct chtls_dev *cdev, struct sk_buff *skb)
>  
>  	reply_skb = alloc_skb(sizeof(struct cpl_abort_rpl),
>  			      GFP_KERNEL | __GFP_NOFAIL);
> +	if (!reply_skb) {
> +		pr_warn("%s: cannot allocate skb!\n", __func__);
> +		return;
> +	}
> +
>  	__skb_put(reply_skb, sizeof(struct cpl_abort_rpl));
>  	set_abort_rpl_wr(reply_skb, GET_TID(req),
>  			 (req->status & CPL_ABORT_NO_RST));

Hi Charles,

I agree that not checking for NULL skbs will very soon lead
to a NULL pointer dereference. But I wonder if this patch leads
us to a better place. Because by returning on skb allocation
failure in each of the above cases, don't we end up with
an inconsistent state?

Also, the above notwithstanding, I do wonder if:
a) the warnings should be errors
b) they should be rate limited