linux-kernel - 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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0ba1e9814048e52b1b7cb4f772ad30bdd3a0cbbd.camel@wdc.com>
Date: Tue, 2 Sep 2025 22:50:53 +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>, "davem@...emloft.net" <davem@...emloft.net>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"john.fastabend@...il.com" <john.fastabend@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Alistair
 Francis <Alistair.Francis@....com>, "kuba@...nel.org" <kuba@...nel.org>,
	"horms@...nel.org" <horms@...nel.org>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v2] net/tls: support maximum record size limit

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>
> > 
> > During a handshake, an endpoint may specify a maximum record size
> > limit.
> > Currently, the kernel defaults to TLS_MAX_PAYLOAD_SIZE (16KB) for
> > the
> > maximum record size. Meaning that, the outgoing records from the
> > kernel
> > can exceed a lower size negotiated during the handshake. In such a
> > case,
> > the TLS endpoint must send a fatal "record_overflow" alert [1], and
> > thus the record is discarded.
> > 
> > Upcoming Western Digital NVMe-TCP hardware controllers implement
> > TLS
> > support. For these devices, supporting TLS record size negotiation
> > is
> > necessary because the maximum TLS record size supported by the
> > controller
> > is less than the default 16KB currently used by the kernel.
> > 
> > This patch adds support for retrieving the negotiated record size
> > limit
> > during a handshake, and enforcing it at the TLS layer such that
> > outgoing
> > records are no larger than the size negotiated. This patch depends
> > on
> > the respective userspace support in tlshd and GnuTLS [2].
> > 
> > [1] https://www.rfc-editor.org/rfc/rfc8449
> > [2] https://gitlab.com/gnutls/gnutls/-/merge_requests/2005
> > 
> > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@....com>
> > ---
> >  Documentation/networking/tls.rst |  7 ++++++
> >  include/net/tls.h                |  1 +
> >  include/uapi/linux/tls.h         |  2 ++
> >  net/tls/tls_main.c               | 39
> > ++++++++++++++++++++++++++++++--
> >  net/tls/tls_sw.c                 |  4 ++++
> >  5 files changed, 51 insertions(+), 2 deletions(-)
> 
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?
Wouldn't the socket just return N bytes received which doesn't
necessarily correlate to a record size?
> 
> > diff --git a/include/net/tls.h b/include/net/tls.h
> > index 857340338b69..c9a3759f27ca 100644
> > --- a/include/net/tls.h
> > +++ b/include/net/tls.h
> > @@ -226,6 +226,7 @@ struct tls_context {
> >  	u8 rx_conf:3;
> >  	u8 zerocopy_sendfile:1;
> >  	u8 rx_no_pad:1;
> > +	u16 record_size_limit;
> 
> Maybe "tx_record_size_limit", since it's not intended for RX?
> 
> I don't know if the kernel will ever have a need to enforce the RX
> record size, but it would maybe avoid future head-scratching "why is
> this not used on the RX path?"
Ah good point, I think this makes sense.
> 
> 
> 
> > diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> > index a3ccb3135e51..1098c01f2749 100644
> > --- a/net/tls/tls_main.c
> > +++ b/net/tls/tls_main.c
> > @@ -812,6 +812,31 @@ static int do_tls_setsockopt_no_pad(struct
> > sock *sk, sockptr_t optval,
> >  	return rc;
> >  }
> >  
> > +static int do_tls_setsockopt_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;
> > +
> > +	ctx->record_size_limit = value;
> > +
> > +	return 0;
> > +}
> > +
> >  static int do_tls_setsockopt(struct sock *sk, int optname,
> > sockptr_t optval,
> >  			     unsigned int optlen)
> >  {
> > @@ -833,6 +858,9 @@ static int do_tls_setsockopt(struct sock *sk,
> > int optname, sockptr_t optval,
> >  	case TLS_RX_EXPECT_NO_PAD:
> >  		rc = do_tls_setsockopt_no_pad(sk, optval, optlen);
> >  		break;
> > +	case TLS_TX_RECORD_SIZE_LIM:
> > +		rc = do_tls_setsockopt_record_size(sk, optval,
> > optlen);
> > +		break;
> 
> Adding the corresponding changes to do_tls_getsockopt would also be
> good.
> 
okay, I will add that.
> 
> > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> > index bac65d0d4e3e..9f9359f591d3 100644
> > --- a/net/tls/tls_sw.c
> > +++ b/net/tls/tls_sw.c
> > @@ -1033,6 +1033,7 @@ static int tls_sw_sendmsg_locked(struct sock
> > *sk, struct msghdr *msg,
> >  	unsigned char record_type = TLS_RECORD_TYPE_DATA;
> >  	bool is_kvec = iov_iter_is_kvec(&msg->msg_iter);
> >  	bool eor = !(msg->msg_flags & MSG_MORE);
> > +	u16 record_size_limit;
> >  	size_t try_to_copy;
> >  	ssize_t copied = 0;
> >  	struct sk_msg *msg_pl, *msg_en;
> > @@ -1058,6 +1059,9 @@ static int tls_sw_sendmsg_locked(struct sock
> > *sk, struct msghdr *msg,
> >  		}
> >  	}
> >  
> > +	record_size_limit = tls_ctx->record_size_limit ?
> > +			    tls_ctx->record_size_limit :
> > TLS_MAX_PAYLOAD_SIZE;
> 
> As Simon said (good catch Simon :)), this isn't used anywhere. Are
> you
> sure this patch works? The previous version had a hunk in
> tls_sw_sendmsg_locked that looks like what I would expect.
> 
This is a bug! I missed adding that hunk.
> And the the offloaded TX path (in net/tls/tls_device.c) would also
> need similar changes.
> 
Okay, will add in V3.
> 
> I'm wondering if it's better to add this conditional, or just
> initialize record_size_limit to TLS_MAX_PAYLOAD_SIZE as we set up the
> tls_context. Then we only have to replace TLS_MAX_PAYLOAD_SIZE with
> tls_ctx->record_size_limit in a few places?
Yeah I think sounds better, will add for V3.

Thanks for the feedback!

Regards,
Wilfred