linux-kernel - Re: [PATCH v3] 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: <c7104a0d58c674bb1fec1a6093be6085ad0be0e0.camel@gmail.com>
Date: Thu, 18 Sep 2025 11:42:23 +1000
From: Wilfred Mallawa <wilfred.opensource@...il.com>
To: Sabrina Dubroca <sd@...asysnail.net>, kuba@...nel.org
Cc: davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
 horms@...nel.org, 	corbet@....net, john.fastabend@...il.com,
 netdev@...r.kernel.org, 	linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, 	alistair.francis@....com, dlemoal@...nel.org
Subject: Re: [PATCH v3] net/tls: support maximum record size limit

On Thu, 2025-09-04 at 12:10 +0200, Sabrina Dubroca wrote:
> 2025-09-03, 11:47:57 +1000, Wilfred Mallawa wrote:
> > +static int do_tls_setsockopt_tx_record_size(struct sock *sk,
> > sockptr_t optval,
> > +					    unsigned int optlen)
> > +{
> > +	struct tls_context *ctx = tls_get_ctx(sk);
> > +	u16 value;
> > +
> > +	if (sockptr_is_null(optval) || optlen != sizeof(value))
> > +		return -EINVAL;
> > +
> > +	if (copy_from_sockptr(&value, optval, sizeof(value)))
> > +		return -EFAULT;
> > +
> > +	if (ctx->prot_info.version == TLS_1_2_VERSION &&
> > +	    value > TLS_MAX_PAYLOAD_SIZE)
> > +		return -EINVAL;
> > +
> > +	if (ctx->prot_info.version == TLS_1_3_VERSION &&
> > +	    value > TLS_MAX_PAYLOAD_SIZE + 1)
> > +		return -EINVAL;
> 
> The RFC is not very explicit about this, but I think this +1 for
> TLS1.3 is to allow an actual payload of TLS_MAX_PAYLOAD_SIZE and save
> 1B of room for the content_type that gets appended.
> 
>    This value is the length of the plaintext of a protected record. 
> The
>    value includes the content type and padding added in TLS 1.3 (that
>    is, the complete length of TLSInnerPlaintext).
> 
> AFAIU we don't actually want to stuff TLS_MAX_PAYLOAD_SIZE+1 bytes of
> payload into a record.
> 
> If we set tx_record_size_limit to TLS_MAX_PAYLOAD_SIZE+1, we'll end
> up
> sending a record with a plaintext of TLS_MAX_PAYLOAD_SIZE+2 bytes
> (TLS_MAX_PAYLOAD_SIZE+1 of payload, then 1B of content_type), and a
> "normal" implementation will reject the record since it's too big
> (ktls does that in net/tls/tls_sw.c:tls_rx_msg_size).
> 
> So we should subtract 1 from the userspace-provided value for 1.3,
> and
> then add it back in getsockopt/tls_get_info.
Yeah good point. I will fix this up
> 
> Or maybe userspace should provide the desired payload limit, instead
> of the raw record_size_limit it got from the extension (ie, do -1
> when
> needed before calling the setsockopt). Then we should rename this
> "tx_payload_size_limit" (and adjust the docs) to make it clear it's
> not the raw record_size_limit.
> 
> The "tx_payload_size_limit" approach is maybe a little bit simpler
> (not having to add/subtract 1 in a few places - I think userspace
> would only have to do it in one place).
> 
> 
> Wilfred, Jakub, what do you think?
I'm in favour of keeping the RFC definition, as this would mean
userspace only has to worry about passing up the value received from
the record size limit extension.

For V4, I'll add a note in the docs regarding the TLS 1.3 ContentType,
and fixup tx_record_size_limit to account for ContentType.

Regards,
Wilfred
> 
> 
> > +	ctx->tx_record_size_limit = value;
> > +
> > +	return 0;
> > +}