| 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
| ||
|
Message-ID: <aLcWOJeAFeM6_U6w@krikkit>
Date: Tue, 2 Sep 2025 18:07:20 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Wilfred Mallawa <wilfred.opensource@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Jonathan Corbet <corbet@....net>,
John Fastabend <john.fastabend@...il.com>,
Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
Alistair Francis <alistair.francis@....com>,
Damien Le'Moal <dlemoal@...nel.org>,
Wilfred Mallawa <wilfred.mallawa@....com>
Subject: Re: [PATCH v2] net/tls: support maximum record size limit
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(-)
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?
> 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?"
> 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.
> 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.
And the the offloaded TX path (in net/tls/tls_device.c) would also
need similar changes.
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?
--
Sabrina
Powered by blists - more mailing lists