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

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

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.