linux-kernel - Re: [PATCH v4 2/2] selftests: tls: add tls record_size

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aNQvZnCWhymiXYPO@krikkit>
Date: Wed, 24 Sep 2025 19:50:30 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Wilfred Mallawa <wilfred.opensource@...il.com>
Cc: netdev@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, horms@...nel.org, corbet@....net,
	john.fastabend@...il.com, shuah@...nel.org,
	Wilfred Mallawa <wilfred.mallawa@....com>
Subject: Re: [PATCH v4 2/2] selftests: tls: add tls record_size_limit test

[got a bit distracted while writing this so Simon got to the process
stuff before me, but I'll leave it in:]

BTW, a few details about process: since this is a new feature, the
subject prefix should be [PATCH net-next v4 n/m] (new stuff targets
the net-next tree), and the patches should be based on the net-next
tree [1] (I'm not sure what you based this on, git am complained on
both net and net-next for this patch). More info about this in the
docs [2].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/
[2] https://docs.kernel.org/process/maintainer-netdev.html
    (in case you're not aware: also note the bits about "merge window"
    which will quite likely become relevant in a few days)


2025-09-23, 15:32:07 +1000, Wilfred Mallawa wrote:
> From: Wilfred Mallawa <wilfred.mallawa@....com>
> 
> Test that outgoing plaintext records respect the tls record_size_limit
> set using setsockopt(). The record size limit is set to be 128, thus,
> in all received records, the plaintext must not exceed this amount.
> 
> Also test that setting a new record size limit whilst a pending open
> record exists is handled correctly by discarding the request.
> 
> Suggested-by: Sabrina Dubroca <sd@...asysnail.net>
> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@....com>

Thanks for adding this patch.
(and for the tag :))

> ---
>  tools/testing/selftests/net/tls.c | 149 ++++++++++++++++++++++++++++++
>  1 file changed, 149 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
> index 0f5640d8dc7f..c5bd431d5af3 100644
> --- a/tools/testing/selftests/net/tls.c
> +++ b/tools/testing/selftests/net/tls.c
> @@ -24,6 +24,7 @@
>  #include "../kselftest_harness.h"
>  
>  #define TLS_PAYLOAD_MAX_LEN 16384
> +#define TLS_TX_RECORD_SIZE_LIM 5

nit: That should not be needed if you run `make headers_install`
before compiling the selftest:

make -s headers_install ; make -C tools/testing/selftests/net tls
make: Entering directory '/home/sab/linux/net/tools/testing/selftests/net'
gcc -Wall -Wl,--no-as-needed -O2 -g -I../../../../usr/include/ -isystem /home/sab/linux/net/tools/testing/selftests/../../../usr/include -I../ -D_GNU_SOURCE=     tls.c   -o tls

and that will find the new constant defined in the previous patch
using the headers from the current kernel tree, instead of those in
the system.


[...]
> +TEST(tx_record_size)
> +{
> +	struct tls_crypto_info_keys tls12;
> +	int cfd, ret, fd, rx_len, overhead;
> +	size_t total_plaintext_rx = 0;
> +	__u8 tx[1024], rx[2000];
> +	__u8 *rec;
> +	__u16 limit = 128;
> +	__u16 opt = 0;
> +	__u8 rec_header_len = 5;

gcc complains about unused variables, I guess leftovers from
extracting parse_tls_records:

tls.c: In function ‘tx_record_size’:
tls.c:2840:14: warning: unused variable ‘rec_header_len’ [-Wunused-variable]
 2840 |         __u8 rec_header_len = 5;
      |              ^~~~~~~~~~~~~~
tls.c:2837:15: warning: unused variable ‘rec’ [-Wunused-variable]
 2837 |         __u8 *rec;
      |               ^~~
tls.c: In function ‘tx_record_size_open_rec’:
tls.c:2893:14: warning: unused variable ‘rec_header_len’ [-Wunused-variable]
 2893 |         __u8 rec_header_len = 5;
      |              ^~~~~~~~~~~~~~
tls.c:2891:15: warning: unused variable ‘rec’ [-Wunused-variable]
 2891 |         __u8 *rec;
      |               ^~~


> +	unsigned int optlen = sizeof(opt);
> +	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);
> +
> +	ret = getsockopt(cfd, SOL_TLS, TLS_TX_RECORD_SIZE_LIM, &opt, &optlen);
> +	ASSERT_EQ(ret, 0);
> +	ASSERT_EQ(limit, opt);
> +	ASSERT_EQ(optlen, sizeof(limit));

nit: Maybe a few of those should be EXPECT_EQ? (ASSERT_* stops the
test, EXPECT_* will run the rest of the test)

Getting the wrong value back from this getsockopt is worth noting but
there's value in running the traffic through anyway?

> +
> +	memset(tx, 0, sizeof(tx));
> +	EXPECT_EQ(send(cfd, tx, sizeof(tx), 0), sizeof(tx));

But this one should maybe be an ASSERT because trying to parse records
from whatever data we managed to send (if any) may not make much
sense?

(just some thoughts, this is not a "strict requirement" to change
anything in the patch)


> +	close(cfd);
> +
> +	ret = recv(fd, rx, sizeof(rx), 0);
> +	memcpy(&rx_len, rx + 3, 2);
> +	rx_len = htons(rx_len);

nit: set but not used (also in tx_record_size_open_rec)

-- 
Sabrina