| 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: <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
Powered by blists - more mailing lists