Re: [PATCH net-next 2/3] tcp: convert do_tcp_getsockopt() to sockopt_t

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 2

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).

- [High] The commit message identifies a bug class: copy_to_iter() runs
  the hardened-usercopy check on a kernel stack source.
- [Low] TCP_SAVED_SYN now consumes and frees tp->saved_syn before optlen
  is written back.
- [Low] bpf: non-standard multi-line comment style in
  sol_tcp_getsockopt()

--

tcp: convert do_tcp_getsockopt() to sockopt_t

This converts do_tcp_getsockopt() from a sockptr_t optval/optlen pair to
sockopt_t and removes the tcp_ao_get_sockptr() bridge. tcp_getsockopt()
now always writes optlen back. BPF callers go through a new
sol_tcp_getsockopt() helper, which calls do_tcp_getsockopt() with page
faults disabled so the hardened usercopy check skips find_vmap_area().

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 9b8d5191c4ba2..0d5afe1c5606b 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5686,6 +5686,28 @@ static int bpf_sol_tcp_setsockopt(struct sock *sk, int optname,
>  	return 0;
>  }
>  
> +static int sol_tcp_getsockopt(struct sock *sk, int optname, char *optval,
> +			      int *optlen)
> +{
> +	struct kvec kvec;
> +	sockopt_t opt;
> +	int err;
> +
> +	err = sockptr_to_sockopt(&opt, KERNEL_SOCKPTR(optval),
> +				 KERNEL_SOCKPTR(optlen), &kvec);
> +	if (err)
> +		return err;
> +
> +	/* BPF may call this in softirq: with page faults disabled, the
> +	 * usercopy check skips find_vmap_area() and its lock.
> +	 */
> +	pagefault_disable();
> +	err = do_tcp_getsockopt(sk, SOL_TCP, optname, &opt);
> +	pagefault_enable();

[Severity: Low]
This is a minor style nit. net/core/filter.c follows the BPF comment
style, which wants the opening /* of a multi-line comment on its own
line, with the text starting on the next line. That applies to new code
even where older comments in the file still use the net style.

The new comment in sol_tcp_getsockopt() puts the text on the opening
line. Could it be reformatted as:

	/*
	 * BPF may call this in softirq: with page faults disabled, the
	 * usercopy check skips find_vmap_area() and its lock.
	 */

If the pagefault_disable() moves to a shared BPF entry point, as asked
above, the same applies to the comment wherever it ends up.

[Severity: High]
Is the same softirq problem still present in the SOL_IP and SOL_IPV6 BPF
getsockopt paths in this file?

sol_ip_sockopt() and sol_ipv6_sockopt() build the same kvec-backed
sockopt_t, and they call the converted getters with no guard:

net/core/filter.c:sol_ip_sockopt() {
    ...
		err = sockptr_to_sockopt(&opt, KERNEL_SOCKPTR(optval),
					 KERNEL_SOCKPTR(optlen), &kvec);
		if (err)
			return err;

		err = do_ip_getsockopt(sk, SOL_IP, optname, &opt);
		*optlen = opt.optlen;
		return err;
    ...
}

sol_ipv6_sockopt() does the same with do_ipv6_getsockopt(). Both
do_ip_getsockopt() and do_ipv6_getsockopt() end with:

	if (copy_to_iter(&val, len, &sopt->iter_out) != len)

Here val is a stack local and len is not a compile-time constant.

Take a sock_ops program running from a softirq callback such as
BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB, or a TCP CC struct_ops program, that
calls bpf_getsockopt(SOL_IP, IP_TOS). Wouldn't it go down this path?

__bpf_getsockopt()
  sol_ip_sockopt()
    do_ip_getsockopt()
      copy_to_iter()
        check_copy_size()
          __check_object_size()
            check_heap_object()
              find_vmap_area()

The IRQ stack is not current's task stack, and check_heap_object() only
skips the lookup when page faults are disabled:

mm/usercopy.c:check_heap_object() {
    ...
	if (is_vmalloc_addr(ptr) && !pagefault_disabled()) {
		struct vmap_area *area = find_vmap_area(addr);
    ...
}

On CONFIG_HARDENED_USERCOPY plus CONFIG_VMAP_STACK kernels, this looks
like the same inconsistent lock state on the find_vmap_area() lock. That
lock is taken with plain spin_lock(), so a softirq that interrupts a
process-context holder on the same CPU could also deadlock. The same
applies to bpf_getsockopt(SOL_IP, IP_TRANSPARENT) and to
bpf_getsockopt(SOL_IPV6, IPV6_TCLASS / IPV6_AUTOFLOWLABEL /
IPV6_TRANSPARENT).

The commit message names sol_ip_sockopt() as the model for the optlen
handling, but that function has the same exposure.

Would it be better to put the pagefault_disable() at the shared BPF entry
(__bpf_getsockopt() / _bpf_getsockopt()), or in the kernel-buffer
sockopt_t setup? Otherwise every per-level wrapper has to remember it.

[ ... ]

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 552e8fe22e7e2..de287845e6c0d 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c

[ ... ]

> @@ -4904,13 +4859,22 @@ int tcp_getsockopt(struct sock *sk, int level, int optname, char __user *optval,
>  		   int __user *optlen)
>  {
>  	struct inet_connection_sock *icsk = inet_csk(sk);
> +	sockopt_t sopt;
> +	int err;
>  
>  	if (level != SOL_TCP)
>  		/* Paired with WRITE_ONCE() in do_ipv6_setsockopt() and tcp_v6_connect() */
>  		return READ_ONCE(icsk->icsk_af_ops)->getsockopt(sk, level, optname,
>  								optval, optlen);
> -	return do_tcp_getsockopt(sk, level, optname, USER_SOCKPTR(optval),
> -				 USER_SOCKPTR(optlen));
> +
> +	err = sockopt_init_user(&sopt, optval, optlen);
> +	if (err)
> +		return err;
> +
> +	err = do_tcp_getsockopt(sk, level, optname, &sopt);
> +	if (put_user(sopt.optlen, optlen))
> +		return -EFAULT;
> +	return err;

[Severity: Low]
Can TCP_SAVED_SYN now return -EFAULT after the saved SYN has already been
freed?

In do_tcp_getsockopt(), the TCP_SAVED_SYN success branch copies
tp->saved_syn->data to iter_out, calls tcp_saved_syn_free(tp) and returns
0. Only after that does this put_user() write optlen, and if it fails the
0 becomes -EFAULT.

Before this patch, optlen was written before optval and before
tcp_saved_syn_free(). Every -EFAULT return therefore left tp->saved_syn
in place, and the caller could retry.

Now suppose optlen is readable but not writable. get_user() in
sockopt_init_user() succeeds and the put_user() here fails. The caller
gets -EFAULT, but the one-shot saved SYN has already been freed.

TCP_ZEROCOPY_RECEIVE also writes optlen here after tcp_zerocopy_receive()
has consumed data. That option could already return -EFAULT after
consuming data when optval was unwritable, though.

The commit message mentions that an unwritable optlen now gives -EFAULT.
It does not mention that TCP_SAVED_SYN frees its saved SYN before
returning that error.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261112-getsockopt_phase7-v1-0-1eb759dd0929%40debian.org