Re: [PATCH net v2] tls: keep TX and RX hardware offload on the same device

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 1 · High: 0 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] tls: type confusion and UAF in tls_device_down() when TX is
  not TLS_HW

Pre-existing issues:
- [High] tls_set_sw_offload() (net/tls/tls_sw.c:2594) sets the shared
  ctx->push_pending_record = tls_sw_push_pending_record unconditionally,…

--

tls: keep TX and RX hardware offload on the same device

This patch adds tls_device_can_attach(), which is checked under
device_offload_lock before each tls_dev_add() in tls_set_device_offload()
and tls_set_device_offload_rx(). A second direction is only offloaded if
it resolves to the netdev already bound to the context; otherwise setup
falls back to software for that direction.

> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index f11d0528fc431..9df183d0dd972 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c

[ ... ]

> @@ -1227,6 +1246,11 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
>  		goto release_lock;
>  	}
>  
> +	if (!tls_device_can_attach(ctx, sk, netdev)) {
> +		rc = -EOPNOTSUPP;
> +		goto release_lock;
> +	}
> +

[Severity: Critical]
This isn't introduced by this patch, but the patch now deliberately
steers sockets into the state that hits it, so it seems worth raising
here.

When RX is on hardware and TX is declined by tls_device_can_attach(),
do_tls_setsockopt_conf() falls back to TLS_SW for TX (or TX is simply
never configured and stays TLS_BASE), while ctx stays on
tls_device_list bound to the RX device. If that device later goes down,
tls_device_down() does this for every matching ctx:

	WRITE_ONCE(ctx->sk->sk_validate_xmit_skb,
		   tls_validate_xmit_skb_sw);

without looking at ctx->tx_conf. The tls_dev_del() calls further down
are guarded by tx_conf == TLS_HW and rx_conf == TLS_HW, but this store
is not.

>From then on every skb with TCP payload on the socket goes through
validate_xmit_skb() -> tls_validate_xmit_skb_sw() -> tls_sw_fallback(),
which casts tls_ctx->priv_ctx_tx with tls_offload_ctx_tx() and passes
the result to fill_sg_in(), which takes ctx->lock and walks
ctx->records_list through tls_get_record().

With tx_conf == TLS_BASE, priv_ctx_tx is NULL. Isn't this a NULL
pointer dereference on the first data segment sent after the device
goes down? With tx_conf == TLS_SW, priv_ctx_tx is the struct
tls_sw_context_tx from init_ctx_tx(), so fill_sg_in() would take a
spinlock and walk a list that are really unrelated fields of that
structure.

Close looks worse still. tls_sk_proto_close() frees priv_ctx_tx via
tls_sw_free_ctx_tx() before calling ctx->sk_proto->close(), but with
rx_conf == TLS_HW the tls_context and the sk_validate_xmit_skb hook stay
around until tls_device_sk_destruct(). Data pushed out by tcp_close()
or retransmitted afterwards would run tls_sw_fallback() on freed
memory.

The updated tls-offload.rst says a declined direction "uses software
instead", and NETDEV_DOWN clearing the binding is now an explicit
reason to decline TX, so the patch relies on this combination working.

Should tls_device_down() only switch sk_validate_xmit_skb when TX is
really device offloaded? ctx->tx_conf is only set by
do_tls_setsockopt_conf() after tls_set_device_offload() has dropped
device_offload_lock, so I'm not sure tx_conf alone is a reliable test
there. Would comparing against tls_validate_xmit_skb, or having
tls_validate_xmit_skb_sw() bail out when TX is not TLS_HW, work better?

[Severity: High]
This isn't a bug introduced by this patch, but it is related to the new
tls-offload.rst wording that a declined direction "is handled in software
as if offload had never been tried".

Suppose TX is already on hardware and RX is rejected here. Then
do_tls_setsockopt_conf() falls back to tls_set_sw_offload(sk, 0, ...),
and that function sets the shared TX callback whatever the direction:

net/tls/tls_sw.c:tls_set_sw_offload() {
    ...
	ctx->push_pending_record = tls_sw_push_pending_record;
    ...
}

This replaces the callback that tls_set_device_offload() installed:

	ctx->push_pending_record = tls_device_push_pending_record;

The hardware RX success path also calls tls_set_sw_offload(sk, 0, NULL),
so this could already happen before this patch.

Now suppose device TX leaves an open record, for example because
sendmsg() with MSG_MORE set pending_open_record_frags. A later sendmsg()
with a TLS_SET_RECORD_TYPE cmsg then takes this path:

tls_device_sendmsg()
  tls_process_cmsg()
    tls_handle_open_record()
      ctx->push_pending_record() -> tls_sw_push_pending_record()

tls_sk_proto_cleanup() reaches the same callback on close when
sk_write_pending is set. tls_sw_push_pending_record() does:

	struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
	struct tls_rec *rec = ctx->open_rec;

but priv_ctx_tx is really a struct tls_offload_context_tx here.

Doesn't this read an unrelated field of tls_offload_context_tx as a
struct tls_rec pointer?

If that field is NULL, the open device record is not flushed before the
record type changes. Data that is already buffered then goes out under
the new record type. If the field is non-NULL, rec->msg_plaintext is
read through a garbage pointer.

This seems to go back to 3c4d7559159b (2017). Could tls_set_sw_offload()
set push_pending_record only when tx is set? Then RX setup, hardware or
software fallback, would leave the device TX callback in place.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020214500.972308-1-rjethwani%40purestorage.com