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@xxxxxxx> > > > > 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@xxxxxxx> > > --- > > 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
