Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <120e6c2c-6122-41db-8c46-7753e9659c70@gmail.com>
Date:   Tue, 24 Oct 2023 10:17:08 +0800
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] net: tls: Fix possible NULL-pointer dereference in
 tls_decrypt_device() and tls_decrypt_sw()
To:     Sabrina Dubroca <sd@queasysnail.net>
Cc:     borisp@nvidia.com, john.fastabend@gmail.com, kuba@kernel.org,
        davem@davemloft.net, edumazet@google.com, pabeni@redhat.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20231023080611.19244-1-hbh25y@gmail.com> <ZTZ9H4aDB45RzrFD@hog>
Content-Language: en-US
From:   Hangyu Hua <hbh25y@gmail.com>
In-Reply-To: <ZTZ9H4aDB45RzrFD@hog>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 23/10/2023 22:03, Sabrina Dubroca wrote:
> 2023-10-23, 16:06:11 +0800, Hangyu Hua wrote:
>> tls_rx_one_record can be called in tls_sw_splice_read and tls_sw_read_sock
>> with msg being NULL. This may lead to null pointer dereferences in
>> tls_decrypt_device and tls_decrypt_sw.
>>
>> Fix this by adding a check.
> 
> Have you actually hit this NULL dereference? I don't see how it can
> happen.
> 
> darg->zc is 0 in both cases, so tls_decrypt_device doesn't call
> skb_copy_datagram_msg.
> 
> tls_decrypt_sw will call tls_decrypt_sg with out_iov = &msg->msg_iter
> (a bogus pointer but no NULL deref yet), and darg->zc is still
> 0. tls_decrypt_sg skips the use of out_iov/out_sg and allocates
> clear_skb, and the next place where it would use out_iov is skipped
> because we have clear_skb.

My bad. I only checked &msg->msg_iter's address in tls_decrypt_sw and 
found it was wrong. Do I need to make a new patch to fix the harmless 
bogus pointer?

> 
> Relevant parts of tls_decrypt_sg:
> 
> static int tls_decrypt_sg(struct sock *sk, struct iov_iter *out_iov,
> 			  struct scatterlist *out_sg,
> 			  struct tls_decrypt_arg *darg)
> {
> [...]
> 	if (darg->zc && (out_iov || out_sg)) {
> 		clear_skb = NULL;
> [...]
> 	} else {
> 		darg->zc = false;
> 
> 		clear_skb = tls_alloc_clrtxt_skb(sk, skb, rxm->full_len);
> [...]
> 	}
> 
> [...]
> 	if (err < 0)
> 		goto exit_free;
> 
> 	if (clear_skb) {
> 		sg_init_table(sgout, n_sgout);
> 		sg_set_buf(&sgout[0], dctx->aad, prot->aad_size);
> 
> 		err = skb_to_sgvec(clear_skb, &sgout[1], prot->prepend_size,
> 				   data_len + prot->tail_size);
> 		if (err < 0)
> 			goto exit_free;
> 	} else if (out_iov) {
> [...]
> 	} else if (out_sg) {
> 		memcpy(sgout, out_sg, n_sgout * sizeof(*sgout));
> 	}
> [...]
> }
>