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

[Sabrina Dubroca <sd@queasysnail.net>]

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@wdc.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.

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?


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 :)


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);
}


-- 
Sabrina