RE: [PATCH net-next v5] tcp: add TCP_ECN and TCP_ECN_OPTION kfuncs

["Irlanki Sandeep" <irlanki.s@samsung.com>]

> [Severity: High]
> Could this lead to an out-of-bounds write for non-TCP sockets?
> 
> When a UDP socket requests hardware timestamping, the transmission skb gets
> SKBTX_HW_TSTAMP set, which implicitly sets SKBTX_BPF. When the timestamp
> is delivered, __skb_tstamp_tx() triggers bpf_skops_tx_timestamping(),
> invoking the CGROUP_SOCK_OPS BPF program for the UDP socket with
> is_fullsock set to 1.
> 
> If the BPF program calls this kfunc, it bypasses the !is_fullsock check.
> Since there is no verification that the socket protocol is IPPROTO_TCP, it casts
> the smaller udp_sock to a tcp_sock. Writing to ecn_option would then
> corrupt memory past the end of the udp_sock structure.

Thanks for catching this. Fixed in v6 by adding a sk_is_tcp() check to
both bpf_sock_ops_set_ecn_mode() and bpf_sock_ops_set_accecn_option()
to reject non-TCP sockets.

> [Severity: Medium]
> Will this assertion fail due to child sockets inadvertently inheriting the
> listener's ECN overrides?
> 
> When a passive connection is established, tcp_create_openreq_child() in
> net/ipv4/tcp_minisocks.c clones the listener socket via inet_csk_clone_lock().
> This copies the entire tcp_sock structure, including the new ecn_mode and
> ecn_option fields.
> 
> Since these fields are not explicitly reset to TCP_ECN_MODE_UNSPEC during
> the initialization block in tcp_create_openreq_child(), the child socket seems
> to retain the listener's mode, breaking the intended design and causing this
> test assertion to fail.

You are right, thanks. The v5 description and selftest wrongly assumed
accepted sockets go through tcp_init_sock(); they are cloned from the
listener via inet_csk_clone_lock() and inherit both fields.

Rather than reset them in tcp_create_openreq_child(), v6 keeps the
inheritance, since that is how every other tcp_sock setting behaves
(keepalive, TCP_NODELAY, tcp_tx_delay, and congestion control). A server-side BPF program
that sets ecn_option on a listener expects the accepted sockets to
follow it without re-applying it on every PASSIVE_ESTABLISHED_CB.

The description, kernel-doc, comments, and selftest assertions are
updated accordingly in v6.

> [Severity: Low]
> Does this multi-line comment violate the BPF subsystem style rules? The
> required style places the opening /* on its own line.

Fixed in v6 to conform to BPF comment formatting rules in both
selftest files.

v6 has been submitted here:
https://lore.kernel.org/all/20261001132759.1076145-1-irlanki.s@samsung.com/

Thanks,
Sandeep