[PATCH net] net/iucv: fix locking in .getsockopt

[PATCH net] net/iucv: fix locking in .getsockopt

[Breno Leitao]

Mirror iucv_sock_setsockopt() and wrap the whole switch in
lock_sock()/release_sock(). The pre-existing SO_MSGLIMIT-only lock
becomes redundant and is removed.

Any AF_IUCV HIPER user can potentially crash the kernel by racing
recvmsg() with getsockopt(SO_MSGSIZE): the SO_MSGSIZE arm dereferences
iucv->hs_dev->mtu after iucv_sock_close() (called from the racing
recvmsg()) has set hs_dev to NULL, producing a NULL pointer dereference
oops.

Suggested-by: Stanislav Fomichev <sdf.kernel@gmail.com>
Fixes: 51363b8751a6 ("af_iucv: allow retrieval of maximum message size")
Signed-off-by: Breno Leitao <leitao@debian.org>
---
Note1: Not using goto/label to simplify the merge conflict that might
happen with the migration of af_iucv to getsockopt_iter

Note2: This was only compile-tested.
---
 net/iucv/af_iucv.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 72dfccd4e3d58..c2dc3338670e8 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1540,7 +1540,7 @@ static int iucv_sock_getsockopt(struct socket *sock, int level, int optname,
 	struct sock *sk = sock->sk;
 	struct iucv_sock *iucv = iucv_sk(sk);
 	unsigned int val;
-	int len;
+	int len, rc;
 
 	if (level != SOL_IUCV)
 		return -ENOPROTOOPT;
@@ -1553,26 +1553,34 @@ static int iucv_sock_getsockopt(struct socket *sock, int level, int optname,
 
 	len = min_t(unsigned int, len, sizeof(int));
 
+	rc = 0;
+
+	lock_sock(sk);
 	switch (optname) {
 	case SO_IPRMDATA_MSG:
 		val = (iucv->flags & IUCV_IPRMDATA) ? 1 : 0;
 		break;
 	case SO_MSGLIMIT:
-		lock_sock(sk);
 		val = (iucv->path != NULL) ? iucv->path->msglim	/* connected */
 					   : iucv->msglimit;	/* default */
-		release_sock(sk);
 		break;
 	case SO_MSGSIZE:
-		if (sk->sk_state == IUCV_OPEN)
-			return -EBADFD;
+		if (sk->sk_state == IUCV_OPEN) {
+			rc = -EBADFD;
+			break;
+		}
 		val = (iucv->hs_dev) ? iucv->hs_dev->mtu -
 				sizeof(struct af_iucv_trans_hdr) - ETH_HLEN :
 				0x7fffffff;
 		break;
 	default:
-		return -ENOPROTOOPT;
+		rc = -ENOPROTOOPT;
+		break;
 	}
+	release_sock(sk);
+
+	if (rc)
+		return rc;
 
 	if (put_user(len, optlen))
 		return -EFAULT;

---
base-commit: dc416e32baaeb620b9809e9e25fc7b30889686e9
change-id: 20260520-af_iucv_fix2-74756a034517

Best regards,
--  
Breno Leitao <leitao@debian.org>

Re: [PATCH net] net/iucv: fix locking in .getsockopt

[Alexandra Winter]

On 21.05.26 16:11, Breno Leitao wrote:
> Mirror iucv_sock_setsockopt() and wrap the whole switch in
> lock_sock()/release_sock(). The pre-existing SO_MSGLIMIT-only lock
> becomes redundant and is removed.
> 
> Any AF_IUCV HIPER user can potentially crash the kernel by racing
> recvmsg() with getsockopt(SO_MSGSIZE): the SO_MSGSIZE arm dereferences
> iucv->hs_dev->mtu after iucv_sock_close() (called from the racing
> recvmsg()) has set hs_dev to NULL, producing a NULL pointer dereference
> oops.
> 
> Suggested-by: Stanislav Fomichev <sdf.kernel@gmail.com>
> Fixes: 51363b8751a6 ("af_iucv: allow retrieval of maximum message size")
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> Note1: Not using goto/label to simplify the merge conflict that might
> happen with the migration of af_iucv to getsockopt_iter
> 
> Note2: This was only compile-tested.
> ---

Thank you very much Breno.

I ran some regression tests which call IUCV getsockopt(SO_MSGSIZE) while
sending and receiving data via IUCV. I have not created a specific
reproducer testcase of the described race, which would require enforcing
a problem in IUCV over HS congestion management which triggers an error
path in recvmsg().

Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
Tested-by: Alexandra Winter <wintera@linux.ibm.com>