netdev - Re: [PATCH v2] net/tls: support maximum record size limit

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: <00d28a79b597128b33b53873597f7ba2808ebbe6.camel@wdc.com>
Date: Thu, 4 Sep 2025 23:31:23 +0000
From: Wilfred Mallawa <wilfred.mallawa@....com>
To: "sd@...asysnail.net" <sd@...asysnail.net>
CC: "corbet@....net" <corbet@....net>, "dlemoal@...nel.org"
	<dlemoal@...nel.org>, Alistair Francis <Alistair.Francis@....com>,
	"davem@...emloft.net" <davem@...emloft.net>, "john.fastabend@...il.com"
	<john.fastabend@...il.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "kuba@...nel.org" <kuba@...nel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>, "horms@...nel.org"
	<horms@...nel.org>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"pabeni@...hat.com" <pabeni@...hat.com>
Subject: Re: [PATCH v2] net/tls: support maximum record size limit

On Wed, 2025-09-03 at 10:21 +0200, Sabrina Dubroca wrote:
> 2025-09-02, 22:50:53 +0000, Wilfred Mallawa wrote:
> > On Tue, 2025-09-02 at 18:07 +0200, Sabrina Dubroca wrote:
> > > 2025-09-02, 13:38:10 +1000, Wilfred Mallawa wrote:
> > > > From: Wilfred Mallawa <wilfred.mallawa@....com>
> > Hey Sabrina,
> > > A selftest would be nice (tools/testing/selftests/net/tls.c), but
> > > I'm
> > > not sure what we could do on the "RX" side to check that we are
> > > respecting the size restriction. Use a basic TCP socket and try
> > > to
> > > parse (and then discard without decrypting) records manually out
> > > of
> > > the stream and see if we got the length we wanted?
> > > 
> > So far I have just been using an NVMe TCP Target with TLS enabled
> > and
> > checking that the targets RX record sizes are <= negotiated size in
> > tls_rx_one_record(). I didn't check for this patch and the bug
> > below
> > got through...my bad!
> > 
> > Is it possible to get the exact record length into the testing
> > layer?
> 
> Not really, unless we come up with some mechanism using probes. I
> wouldn't go that route unless we don't have any other choice.
> 
> > Wouldn't the socket just return N bytes received which doesn't
> > necessarily correlate to a record size?
> 
> Yes. That's why I suggested only using ktls on one side of the test,
> and parsing the records out of the raw stream of bytes on the RX
> side.
> 
Ah okay I see.
> Actually, control records don't get aggregated on read, so sending a
> large non-data buffer should result in separate limit-sized reads.
> But
> this makes me wonder if this limit is supposed to apply to control
> records, and how the userspace library/application is supposed to
> deal
> with the possible splitting of those records?
> 
Good point, from the spec, "When the "record_size_limit" extension is
negotiated, an endpoint MUST NOT generate a protected record with
plaintext that is larger than the RecordSizeLimit value it receives
from its peer. Unprotected messages are not subject to this limit." [1]

From what I understand, as long as it in encrypted. It must respect the
record size limit?

In regards to user-space, do you mean for TX or RX? For TX, there
shouldn't need to be any changes as record splitting occurs in the
kernel. For RX, I am not too sure, but this patch shouldn't change
anything for that case?

[1] https://datatracker.ietf.org/doc/html/rfc8449#section-4
> 
> Here's a rough example of what I had in mind. The hardcoded cipher
> overhead is a bit ugly but I don't see a way around it. Sanity check
> at the end is probably not needed. I didn't write the loop because I
> haven't had enough coffee yet to get that right :)
> 
Ha! Great! Thanks for the example. I am not too familiar with the self
tests in the kernel. But will try to it for the next round.

Thanks,
Wilfred
> 
> TEST(tx_record_size)
> {
> 	struct tls_crypto_info_keys tls12;
> 	int cfd, ret, fd, len, overhead;
> 	char buf[1000], buf2[2000];
> 	__u16 limit = 100;
> 	bool notls;
> 
> 	tls_crypto_info_init(TLS_1_2_VERSION,
> TLS_CIPHER_AES_CCM_128,
> 			     &tls12, 0);
> 
> 	ulp_sock_pair(_metadata, &fd, &cfd, &notls);
> 
> 	if (notls)
> 		exit(KSFT_SKIP);
> 
> 	/* Don't install keys on fd, we'll parse raw records */
> 	ret = setsockopt(cfd, SOL_TLS, TLS_TX, &tls12, tls12.len);
> 	ASSERT_EQ(ret, 0);
> 
> 	ret = setsockopt(cfd, SOL_TLS, TLS_TX_RECORD_SIZE_LIM,
> &limit, sizeof(limit));
> 	ASSERT_EQ(ret, 0);
> 
> 	EXPECT_EQ(send(cfd, buf, sizeof(buf), 0), sizeof(buf));
> 	close(cfd);
> 
> 	ret = recv(fd, buf2, sizeof(buf2), 0);
> 	memcpy(&len, buf2 + 3, 2);
> 	len = htons(len);
> 
> 	/* 16B tag + 8B IV -- record header (5B) is not counted but
> we'll need it to walk the record stream */
> 	overhead = 16 + 8;
> 
> 	// TODO should be <= limit since we may not have filled
> every
> 	// record (especially the last one), and loop over all the
> 	// records we got
> 	// next record starts at buf2 + (limit + overhead + 5)
> 	ASSERT_EQ(len, limit + overhead);
> 	/* sanity check that it's a TLS header for application data
> */
> 	ASSERT_EQ(buf2[0], 23);
> 	ASSERT_EQ(buf2[1], 0x3);
> 	ASSERT_EQ(buf2[2], 0x3);
> 
> 	close(fd);
> }
>