netdev - Re: [PATCH v1 net] sctp: Fix null-ptr-deref in reuseport_add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240730170646.62951-1-kuniyu@amazon.com>
Date: Tue, 30 Jul 2024 10:06:46 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <dmantipov@...dex.ru>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<kuni1840@...il.com>, <kuniyu@...zon.com>, <linux-sctp@...r.kernel.org>,
	<lucien.xin@...il.com>, <marcelo.leitner@...il.com>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<syzbot+e6979a5d2f10ecb700e4@...kaller.appspotmail.com>
Subject: Re: [PATCH v1 net] sctp: Fix null-ptr-deref in reuseport_add_sock().

From: Dmitry Antipov <dmantipov@...dex.ru>
Date: Tue, 30 Jul 2024 16:46:05 +0300
> Hm. Note both 'reuseport_add_sock()' and 'reuseport_detach_sock()' uses
> global 'reuseport_lock' internally. So what about using read lock to protect
> 'sctp_for_each_entry()' loop and upgrade to write lock only if hash bucket
> should be actually updated? E.g.:
> 
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 17fcaa9b0df9..4fbff388b1b4 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -735,6 +735,7 @@ static int __sctp_hash_endpoint(struct sctp_endpoint *ep)
>  	struct sock *sk = ep->base.sk;
>  	struct net *net = sock_net(sk);
>  	struct sctp_hashbucket *head;
> +	int err = 0;
>  
>  	ep->hashent = sctp_ep_hashfn(net, ep->base.bind_addr.port);
>  	head = &sctp_ep_hashtable[ep->hashent];
> @@ -743,11 +744,14 @@ static int __sctp_hash_endpoint(struct sctp_endpoint *ep)
>  		bool any = sctp_is_ep_boundall(sk);
>  		struct sctp_endpoint *ep2;
>  		struct list_head *list;
> -		int cnt = 0, err = 1;
> +		int cnt = 0;
> +
> +		err = 1;
>  
>  		list_for_each(list, &ep->base.bind_addr.address_list)
>  			cnt++;
>  
> +		read_lock(&head->lock);
>  		sctp_for_each_hentry(ep2, &head->chain) {
>  			struct sock *sk2 = ep2->base.sk;
>  
> @@ -760,25 +764,30 @@ static int __sctp_hash_endpoint(struct sctp_endpoint *ep)
>  						    sctp_sk(sk), cnt);
>  			if (!err) {
>  				err = reuseport_add_sock(sk, sk2, any);
> -				if (err)
> -					return err;
> +				if (err) {
> +					read_unlock(&head->lock);
> +					goto out;
> +				}
>  				break;
>  			} else if (err < 0) {
> -				return err;
> +				read_unlock(&head->lock);
> +				goto out;
>  			}
>  		}
> +		read_unlock(&head->lock);
>  
>  		if (err) {
>  			err = reuseport_alloc(sk, any);

What happens if two sockets matching each other reach here ?

There would be no error but the first hashed socket will be silently
dead as lookup stops at the 2nd sk ?


>  			if (err)
> -				return err;
> +				goto out;
>  		}
>  	}
>  
>  	write_lock(&head->lock);
>  	hlist_add_head(&ep->node, &head->chain);
>  	write_unlock(&head->lock);
> -	return 0;
> +out:
> +	return err;
>  }
>  
>  /* Add an endpoint to the hash. Local BH-safe. */
> 
> Dmitry