Re: [PATCH net] net/iucv: take a reference on the socket found in afiucv_hs_rcv()

[Alexandra Winter <wintera@linux.ibm.com>]

On 07.07.26 05:24, sashiko-bot@kernel.org wrote:
> [Severity: High]
> This is a pre-existing issue, but does modifying the socket state from a
> softirq context corrupt the socket state machine?
> 
> Since this handler runs in softirq context when network packets arrive, it
> acquires the socket spinlock and directly mutates sk->sk_state. 
> 
> If a user process is concurrently executing a system call and holds the socket
> mutex via lock_sock(), does bypassing the backlog mechanism by failing to check
> sock_owned_by_user() cause race conditions?
> 
> [ ... ]
> 
>> @@ -2089,6 +2089,8 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
>>  			}
>>  		}
>>  	}
>> +	if (sk)
>> +		sock_hold(sk);
>>  	read_unlock(&iucv_sk_list.lock);
>>  	if (!iucv)
>>  		sk = NULL;
>> @@ -2138,6 +2140,8 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
>>  		kfree_skb(skb);
>>  	}
>>  
>> +	if (sk)
>> +		sock_put(sk);
>>  	return err;
>>  }


I think sashiko is right, this patch does not completely close the gap.
See my commit message for
f558120cd709 ("net/iucv: fix use after free in iucv_sock_close()")
Iiuc the socket locking in AF_IUCV is incomplete in general.

Bryam, would you like to try to update the socket locking in AF_IUCV altogether?
Otherwise Hidayath or I can take that task.