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

On 15.05.26 22:45, Stanislav Fomichev wrote:
> On 05/15, Breno Leitao wrote:
>> Mirror the locking used by the SO_MSGLIMIT case directly above: take
>> lock_sock() before reading iucv->hs_dev and dereferencing hs_dev->mtu,
>> and release it afterwards. This keeps the two adjacent getsockopt arms
>> consistent and matches the lock held by iucv_sock_close() when it
>> clears hs_dev.
>>
>> 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'?


>> The change is purely defensive: making the locking pattern uniform
>> across the function avoids surprising the next reader and removes a
>> foot-gun should the close path ever grow a new caller that does not
>> hold the fd reference.
>>


Thank you for this in-depth analysis and clear explanation.
I agree it makes the code more consistent; there should have
been at least a comment, why locking is not necessary here.


>> Note: For the reason above, it doesn't contain a "Fixes" tag, and is
>> aiming at net-next instead of net.
>>
>> Signed-off-by: Breno Leitao <[email protected]>
>> ---
>>  net/iucv/af_iucv.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> 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.


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