Re: [PATCH net-next v2 1/7] af_iucv: take socket lock around SO_MSGSIZE getsockopt

[Breno Leitao <leitao@debian.org>]

On Mon, May 18, 2026 at 02:00:01PM +0200, Alexandra Winter wrote:
> On 15.05.26 22:45, Stanislav Fomichev wrote:
> > On 05/15, Breno Leitao wrote:

> >> This is not an exploitable bug. iucv_sock_close() is the only writer
> >> of iucv->hs_dev and only runs from the protocol release callback,
> >> which the socket layer invokes after the last file reference drops.
> >> The getsockopt() syscall holds an fd reference for its entire
> >> duration via fdget()/fdput(), so iucv_sock_close() cannot run
> >> concurrently with the SO_MSGSIZE read on the same socket. There is
> >> no other writer of hs_dev, and the aligned pointer load cannot tear
> >> on any architecture Linux supports, so the existing code cannot
> >> observe a NULL dereference or use-after-free in practice.
> 
> Actually iucv_sock_bind() and afiucv_hs_callback_syn() also write hs_dev,
> but iucv_sock_close() is the only instance that clears it to NULL.
> Maybe there is another wording than 'writer'?

Good point, you are right. I'll reword it to be precise — something
like "iucv_sock_close() is the only writer that clears hs_dev to NULL",
and call out the other writers (iucv_sock_bind() and
afiucv_hs_callback_syn()) so the reader has the full picture.


> >> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> >> index 72dfccd4e3d58..3dd11d7a967c8 100644
> >> --- a/net/iucv/af_iucv.c
> >> +++ b/net/iucv/af_iucv.c
> >> @@ -1566,9 +1566,11 @@ static int iucv_sock_getsockopt(struct socket *sock, int level, int optname,
> >>  	case SO_MSGSIZE:
> >>  		if (sk->sk_state == IUCV_OPEN)
> >>  			return -EBADFD;
> >> +		lock_sock(sk);
> >>  		val = (iucv->hs_dev) ? iucv->hs_dev->mtu -
> >>  				sizeof(struct af_iucv_trans_hdr) - ETH_HLEN :
> >>  				0x7fffffff;
> >> +		release_sock(sk);
> >>  		break;
> >>  	default:
> >>  		return -ENOPROTOOPT;
> >>
> > 
> > SO_IPRMDATA_MSG also seems to be only reading the value set via setsockopt,
> > so maybe it's ok to just cover the whole switch with lock/unlock? (will
> > mirror what setsockopt does)
> > 
> 
> I like that idea.

Ack, let me implement it and respin.

> Feel free to split this improvement out from your series, if that is more convenient for you.

That is what I was planning to do, but, it will cause some merge conflict
later, so, I decided to get both of them on the same series, so, it apply
cleanly. Let's try it once more in the same series, if this patch doesn't set
easily, I will split it in v4.

Thanks for the review,
--breno