Re: [PATCH net-next 3/6] af_iucv: convert to getsockopt_iter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unlocked access to `iucv->hs_dev` in the `SO_MSGSIZE` case can lead to a NULL pointer dereference or use-after-free.
--

commit f3b71ba02ea8266fcb82046dd3417eeb5b94f2d5
Author: Breno Leitao <leitao@debian.org>

af_iucv: convert to getsockopt_iter

This commit converts the IUCV socket getsockopt implementation to use the
getsockopt_iter callback with sockopt_t. It replaces direct user copy
functions with copy_to_iter.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index 72dfccd4e3d58..7a5ce93a83d9c 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c

[ ... ]

> @@ -1535,7 +1536,7 @@ static int iucv_sock_setsockopt(struct socket *sock, int level, int optname,
>  }
>  
>  static int iucv_sock_getsockopt(struct socket *sock, int level, int optname,
> -				char __user *optval, int __user *optlen)
> +				sockopt_t *opt)
>  {
>  	struct sock *sk = sock->sk;

[ ... ]

> @@ -1574,9 +1573,8 @@ static int iucv_sock_getsockopt(struct socket *sock, int level, int optname,

While reviewing this conversion, I noticed a pre-existing issue in this
function. Does the SO_MSGSIZE case need socket lock protection when accessing
iucv->hs_dev?

In iucv_sock_getsockopt():

        case SO_MSGSIZE:
                if (sk->sk_state == IUCV_OPEN)
                        return -EBADFD;
                val = (iucv->hs_dev) ? iucv->hs_dev->mtu -
                                sizeof(struct af_iucv_trans_hdr) - ETH_HLEN :
                                0x7fffffff;
                break;

If iucv->hs_dev is cleared concurrently by another thread holding the lock
(such as during iucv_sock_close), could the compiler emit two separate
memory loads for hs_dev since READ_ONCE() isn't used?

If the pointer is set to NULL between the check and the dereference for mtu,
could this result in a NULL pointer dereference, or a use-after-free if the
socket held the final reference to the net_device being unregistered?

> -	if (put_user(len, optlen))
> -		return -EFAULT;
> -	if (copy_to_user(optval, &val, len))
> +	opt->optlen = len;
> +	if (copy_to_iter(&val, len, &opt->iter_out) != len)
>  		return -EFAULT;
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-getsock_four-v1-0-fe7f0e756fac@debian.org?part=3