Re: [PATCH net-next V2] net/iucv: fix UAF in afiucv_netdev_event()

[Nagamani PV <nagamani@linux.ibm.com>]

On 11/05/26 2:41 PM, Alexandra Winter wrote:
> 
> 
> On 08.05.26 19:05, Nagamani PV wrote:
>> afiucv_netdev_event() traverses iucv_sk_list without holding
>> iucv_sk_list.lock.
> 
> I agree with the analysis and the patch.
> Good catch Hidayath and Nagamani!
> 
> vvv
> 
>> A concurrent socket teardown can unlink and free the socket via
>> iucv_sock_kill() while the notifier path is still iterating over
>> the list, leading to a possible use-after-free when dereferencing
>> the socket.
>>
>> Protect the traversal using the existing read-side lock, matching
>> the locking pattern already used by other iucv_sk_list traversal
>> paths in af_iucv.c.
>>
>> Use read_lock()/read_unlock() to remain consistent with existing
>> softirq/tasklet-side readers in the same file.
>>
> 
> ^^^these Paragraphs can be less verbose.
> iucv_sk_list.lock is a RW_lock, so it's rather clear that
> afiucv_netdev_event() needs to hold it for traversing the list.
> 
> 
> 
> Please add KASAN report to be part of commit message.
> 
> Just for my information:
> Was the KASAN finding triggered by CI-KASAN run? which testcase?
> Did you verify your patch with KASAN and the same CI testcase? Probably looping?
> 
> 
> 
>> Fixes: 9fbd87d41392 ("af_iucv: handle netdev events")
>> Cc: stable@vger.kernel.org
>> Reported-by: syzbotz+89435e7383b82238dd91@linux.ibm.com
>> Closes: https://lnxgwne1.boeblingen.de.ibm.com/linux-ci/syzbot/dashboard/bug?extid=89435e7383b82238dd91
> 
> This is an internal website, so we cannot report it upstream.
> I am not 100% sure how to handle this case.
> Note that Heiko said, it's ok to use Reported-by without Closes, even if checkpatch complains.
> (He was referring to Reported-by a person, though).
> I would add the KASAN report and remove both tags, if you ask me.
> 
> 
>> Suggested-by: Hidayath Khan <hidayath@linux.ibm.com>
>> Signed-off-by: Nagamani PV <nagamani@linux.ibm.com>
>>
>> ---
>> v2:
>> - Target net-next (missed in v1 subject)
>> ---
> 
> As this is a problem fix, it needs to go to net, not net-next.
> Don't forget to do BBPF backports once this is upstream!
> 
> 
> 
>>  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 72dfccd4e3d5..e8a0b55fc55d 100644
>> --- a/net/iucv/af_iucv.c
>> +++ b/net/iucv/af_iucv.c
>> @@ -2188,6 +2188,7 @@ static int afiucv_netdev_event(struct notifier_block *this,
>>  	switch (event) {
>>  	case NETDEV_REBOOT:
>>  	case NETDEV_GOING_DOWN:
>> +		read_lock(&iucv_sk_list.lock);
>>  		sk_for_each(sk, &iucv_sk_list.head) {
>>  			iucv = iucv_sk(sk);
>>  			if ((iucv->hs_dev == event_dev) &&
>> @@ -2198,6 +2199,7 @@ static int afiucv_netdev_event(struct notifier_block *this,
>>  				sk->sk_state_change(sk);
>>  			}
>>  		}
>> +		read_unlock(&iucv_sk_list.lock);
>>  		break;
>>  	case NETDEV_DOWN:
>>  	case NETDEV_UNREGISTER:
> 
> I agree with the analysis and the patch.
Hi Alexandra,
Thanks for the detailed review.
I’ll simplify the commit message to be less verbose, include a relevant excerpt of the syzbot KASAN report, and remove the internal dashboard link. I’ll keep the Reported-by: syzbot… tag and drop Closes: as suggested. The fix will be targeted to net, not net‑next.
Regarding KASAN: the issue was detected by a syzbot CI run with KASAN enabled. The report does not provide a standalone reproducer or named testcase. I did not rerun the original CI workload, as no reproducer is available; the fix is based on analysis of the reported race and the syzbot KASAN trace.
Following the discussion with Heiko and your later confirmation, I’ll use read_lock_bh() / read_unlock_bh() in the notifier path to keep the locking symmetric with existing write_lock_bh() users.
I’ll resend an updated v2 addressing the above.
Thanks,
Nagamani