[PATCH] nfsd: fix double fget() bug in __write_ports_addfd()

[PATCH] nfsd: fix double fget() bug in __write_ports_addfd()

[Dan Carpenter]

The bug here is that you cannot rely on getting the same socket
from multiple calls to fget() because userspace can influence
that.  This is a kind of double fetch bug.

The fix is to delete the svc_alien_sock() function and insted do
the checking inside the svc_addsock() function.

Fixes: 3064639423c4 ("nfsd: check passed socket's net matches NFSd superblock's one")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
Based on static analysis and untested.  This goes through the NFS tree. 
Inspired by CVE-2023-1838.

 include/linux/sunrpc/svcsock.h |  7 +++----
 fs/nfsd/nfsctl.c               |  7 +------
 net/sunrpc/svcsock.c           | 23 +++++------------------
 3 files changed, 9 insertions(+), 28 deletions(-)

diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
index d16ae621782c..a7116048a4d4 100644
--- a/include/linux/sunrpc/svcsock.h
+++ b/include/linux/sunrpc/svcsock.h
@@ -61,10 +61,9 @@ int		svc_recv(struct svc_rqst *, long);
 void		svc_send(struct svc_rqst *rqstp);
 void		svc_drop(struct svc_rqst *);
 void		svc_sock_update_bufs(struct svc_serv *serv);
-bool		svc_alien_sock(struct net *net, int fd);
-int		svc_addsock(struct svc_serv *serv, const int fd,
-					char *name_return, const size_t len,
-					const struct cred *cred);
+int		svc_addsock(struct svc_serv *serv, struct net *net,
+			    const int fd, char *name_return, const size_t len,
+			    const struct cred *cred);
 void		svc_init_xprt_sock(void);
 void		svc_cleanup_xprt_sock(void);
 struct svc_xprt *svc_sock_create(struct svc_serv *serv, int prot);
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index e0e98b40a6e5..1489e0b703b4 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -698,16 +698,11 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
 		return -EINVAL;
 	trace_nfsd_ctl_ports_addfd(net, fd);
 
-	if (svc_alien_sock(net, fd)) {
-		printk(KERN_ERR "%s: socket net is different to NFSd's one\n", __func__);
-		return -EINVAL;
-	}
-
 	err = nfsd_create_serv(net);
 	if (err != 0)
 		return err;
 
-	err = svc_addsock(nn->nfsd_serv, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
+	err = svc_addsock(nn->nfsd_serv, net, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
 
 	if (err >= 0 &&
 	    !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 46845cb6465d..e4184e40793c 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1474,22 +1474,6 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
 	return svsk;
 }
 
-bool svc_alien_sock(struct net *net, int fd)
-{
-	int err;
-	struct socket *sock = sockfd_lookup(fd, &err);
-	bool ret = false;
-
-	if (!sock)
-		goto out;
-	if (sock_net(sock->sk) != net)
-		ret = true;
-	sockfd_put(sock);
-out:
-	return ret;
-}
-EXPORT_SYMBOL_GPL(svc_alien_sock);
-
 /**
  * svc_addsock - add a listener socket to an RPC service
  * @serv: pointer to RPC service to which to add a new listener
@@ -1502,8 +1486,8 @@ EXPORT_SYMBOL_GPL(svc_alien_sock);
  * Name is terminated with '\n'.  On error, returns a negative errno
  * value.
  */
-int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
-		const size_t len, const struct cred *cred)
+int svc_addsock(struct svc_serv *serv, struct net *net, const int fd,
+		char *name_return, const size_t len, const struct cred *cred)
 {
 	int err = 0;
 	struct socket *so = sockfd_lookup(fd, &err);
@@ -1514,6 +1498,9 @@ int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
 
 	if (!so)
 		return err;
+	err = -EINVAL;
+	if (sock_net(so->sk) != net)
+		goto out;
 	err = -EAFNOSUPPORT;
 	if ((so->sk->sk_family != PF_INET) && (so->sk->sk_family != PF_INET6))
 		goto out;
-- 
2.39.2

Re: [PATCH] nfsd: fix double fget() bug in __write_ports_addfd()

[Jeff Layton]

On Mon, 2023-05-29 at 14:35 +0300, Dan Carpenter wrote:
> The bug here is that you cannot rely on getting the same socket
> from multiple calls to fget() because userspace can influence
> that.  This is a kind of double fetch bug.
> 

Nice catch.

> The fix is to delete the svc_alien_sock() function and insted do
> the checking inside the svc_addsock() function.
> 
> Fixes: 3064639423c4 ("nfsd: check passed socket's net matches NFSd superblock's one")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> Based on static analysis and untested.  This goes through the NFS tree. 
> Inspired by CVE-2023-1838.
> 
>  include/linux/sunrpc/svcsock.h |  7 +++----
>  fs/nfsd/nfsctl.c               |  7 +------
>  net/sunrpc/svcsock.c           | 23 +++++------------------
>  3 files changed, 9 insertions(+), 28 deletions(-)
> 
> diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
> index d16ae621782c..a7116048a4d4 100644
> --- a/include/linux/sunrpc/svcsock.h
> +++ b/include/linux/sunrpc/svcsock.h
> @@ -61,10 +61,9 @@ int		svc_recv(struct svc_rqst *, long);
>  void		svc_send(struct svc_rqst *rqstp);
>  void		svc_drop(struct svc_rqst *);
>  void		svc_sock_update_bufs(struct svc_serv *serv);
> -bool		svc_alien_sock(struct net *net, int fd);
> -int		svc_addsock(struct svc_serv *serv, const int fd,
> -					char *name_return, const size_t len,
> -					const struct cred *cred);
> +int		svc_addsock(struct svc_serv *serv, struct net *net,
> +			    const int fd, char *name_return, const size_t len,
> +			    const struct cred *cred);
>  void		svc_init_xprt_sock(void);
>  void		svc_cleanup_xprt_sock(void);
>  struct svc_xprt *svc_sock_create(struct svc_serv *serv, int prot);
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index e0e98b40a6e5..1489e0b703b4 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -698,16 +698,11 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
>  		return -EINVAL;
>  	trace_nfsd_ctl_ports_addfd(net, fd);
>  
> -	if (svc_alien_sock(net, fd)) {
> -		printk(KERN_ERR "%s: socket net is different to NFSd's one\n", __func__);
> -		return -EINVAL;
> -	}
> -
>  	err = nfsd_create_serv(net);
>  	if (err != 0)
>  		return err;
>  
> -	err = svc_addsock(nn->nfsd_serv, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
> +	err = svc_addsock(nn->nfsd_serv, net, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
>  
>  	if (err >= 0 &&
>  	    !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 46845cb6465d..e4184e40793c 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -1474,22 +1474,6 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
>  	return svsk;
>  }
>  
> -bool svc_alien_sock(struct net *net, int fd)
> -{
> -	int err;
> -	struct socket *sock = sockfd_lookup(fd, &err);
> -	bool ret = false;
> -
> -	if (!sock)
> -		goto out;
> -	if (sock_net(sock->sk) != net)
> -		ret = true;
> -	sockfd_put(sock);
> -out:
> -	return ret;
> -}
> -EXPORT_SYMBOL_GPL(svc_alien_sock);
> -
>  /**
>   * svc_addsock - add a listener socket to an RPC service
>   * @serv: pointer to RPC service to which to add a new listener
> @@ -1502,8 +1486,8 @@ EXPORT_SYMBOL_GPL(svc_alien_sock);
>   * Name is terminated with '\n'.  On error, returns a negative errno
>   * value.
>   */
> -int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
> -		const size_t len, const struct cred *cred)
> +int svc_addsock(struct svc_serv *serv, struct net *net, const int fd,
> +		char *name_return, const size_t len, const struct cred *cred)
>  {
>  	int err = 0;
>  	struct socket *so = sockfd_lookup(fd, &err);
> @@ -1514,6 +1498,9 @@ int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
>  
>  	if (!so)
>  		return err;
> +	err = -EINVAL;
> +	if (sock_net(so->sk) != net)
> +		goto out;
>  	err = -EAFNOSUPPORT;
>  	if ((so->sk->sk_family != PF_INET) && (so->sk->sk_family != PF_INET6))
>  		goto out;

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH] nfsd: fix double fget() bug in __write_ports_addfd()

[NeilBrown]

On Mon, 29 May 2023, Dan Carpenter wrote:
> The bug here is that you cannot rely on getting the same socket
> from multiple calls to fget() because userspace can influence
> that.  This is a kind of double fetch bug.
> 
> The fix is to delete the svc_alien_sock() function and insted do
> the checking inside the svc_addsock() function.

Hi,
 I definitely agree with the change to pass the 'net' into
 svc_addsock(), and check the the fd has the correct net.

 I'm not sure I agree with the removal of the svc_alien_sock() test.  It
 is best to perform sanity tests before allocation things, and
 nfsd_create_serv() can create a new 'serv' - though most often it just
 incs the refcount.

 Maybe instead svc_alien_sock() could return the struct socket (if
 successful), and it could be passed to svc_addsock()???

 I would probably then change the name of svc_alien_sock()

Thanks,
NeilBrown


> 
> Fixes: 3064639423c4 ("nfsd: check passed socket's net matches NFSd superblock's one")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> Based on static analysis and untested.  This goes through the NFS tree. 
> Inspired by CVE-2023-1838.
> 
>  include/linux/sunrpc/svcsock.h |  7 +++----
>  fs/nfsd/nfsctl.c               |  7 +------
>  net/sunrpc/svcsock.c           | 23 +++++------------------
>  3 files changed, 9 insertions(+), 28 deletions(-)
> 
> diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
> index d16ae621782c..a7116048a4d4 100644
> --- a/include/linux/sunrpc/svcsock.h
> +++ b/include/linux/sunrpc/svcsock.h
> @@ -61,10 +61,9 @@ int		svc_recv(struct svc_rqst *, long);
>  void		svc_send(struct svc_rqst *rqstp);
>  void		svc_drop(struct svc_rqst *);
>  void		svc_sock_update_bufs(struct svc_serv *serv);
> -bool		svc_alien_sock(struct net *net, int fd);
> -int		svc_addsock(struct svc_serv *serv, const int fd,
> -					char *name_return, const size_t len,
> -					const struct cred *cred);
> +int		svc_addsock(struct svc_serv *serv, struct net *net,
> +			    const int fd, char *name_return, const size_t len,
> +			    const struct cred *cred);
>  void		svc_init_xprt_sock(void);
>  void		svc_cleanup_xprt_sock(void);
>  struct svc_xprt *svc_sock_create(struct svc_serv *serv, int prot);
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index e0e98b40a6e5..1489e0b703b4 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -698,16 +698,11 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
>  		return -EINVAL;
>  	trace_nfsd_ctl_ports_addfd(net, fd);
>  
> -	if (svc_alien_sock(net, fd)) {
> -		printk(KERN_ERR "%s: socket net is different to NFSd's one\n", __func__);
> -		return -EINVAL;
> -	}
> -
>  	err = nfsd_create_serv(net);
>  	if (err != 0)
>  		return err;
>  
> -	err = svc_addsock(nn->nfsd_serv, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
> +	err = svc_addsock(nn->nfsd_serv, net, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
>  
>  	if (err >= 0 &&
>  	    !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 46845cb6465d..e4184e40793c 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -1474,22 +1474,6 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
>  	return svsk;
>  }
>  
> -bool svc_alien_sock(struct net *net, int fd)
> -{
> -	int err;
> -	struct socket *sock = sockfd_lookup(fd, &err);
> -	bool ret = false;
> -
> -	if (!sock)
> -		goto out;
> -	if (sock_net(sock->sk) != net)
> -		ret = true;
> -	sockfd_put(sock);
> -out:
> -	return ret;
> -}
> -EXPORT_SYMBOL_GPL(svc_alien_sock);
> -
>  /**
>   * svc_addsock - add a listener socket to an RPC service
>   * @serv: pointer to RPC service to which to add a new listener
> @@ -1502,8 +1486,8 @@ EXPORT_SYMBOL_GPL(svc_alien_sock);
>   * Name is terminated with '\n'.  On error, returns a negative errno
>   * value.
>   */
> -int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
> -		const size_t len, const struct cred *cred)
> +int svc_addsock(struct svc_serv *serv, struct net *net, const int fd,
> +		char *name_return, const size_t len, const struct cred *cred)
>  {
>  	int err = 0;
>  	struct socket *so = sockfd_lookup(fd, &err);
> @@ -1514,6 +1498,9 @@ int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
>  
>  	if (!so)
>  		return err;
> +	err = -EINVAL;
> +	if (sock_net(so->sk) != net)
> +		goto out;
>  	err = -EAFNOSUPPORT;
>  	if ((so->sk->sk_family != PF_INET) && (so->sk->sk_family != PF_INET6))
>  		goto out;
> -- 
> 2.39.2
> 
> 

Re: [PATCH] nfsd: fix double fget() bug in __write_ports_addfd()

[Dan Carpenter]

On Wed, May 31, 2023 at 08:27:43AM +1000, NeilBrown wrote:
> On Mon, 29 May 2023, Dan Carpenter wrote:
> > The bug here is that you cannot rely on getting the same socket
> > from multiple calls to fget() because userspace can influence
> > that.  This is a kind of double fetch bug.
> > 
> > The fix is to delete the svc_alien_sock() function and insted do
> > the checking inside the svc_addsock() function.
> 
> Hi,
>  I definitely agree with the change to pass the 'net' into
>  svc_addsock(), and check the the fd has the correct net.
> 
>  I'm not sure I agree with the removal of the svc_alien_sock() test.  It
>  is best to perform sanity tests before allocation things, and
>  nfsd_create_serv() can create a new 'serv' - though most often it just
>  incs the refcount.

That's true.  But the other philosophical rule is that we shouldn't
optimize for the failure path.  If someone gives us bad data they
deserve a slow down.

I also think leaving svc_alien_sock() is a trap for the unwary because
it will lead to more double fget() bugs.  The svc_alien_sock() function
is weird because it returns false on success and false on failure and
true for alien sock.

> 
>  Maybe instead svc_alien_sock() could return the struct socket (if
>  successful), and it could be passed to svc_addsock()???
> 
>  I would probably then change the name of svc_alien_sock()

Yeah, because we don't want alien sockets, we want Earth sockets.
Doing this is much more complicated...  The name svc_get_earth_sock()
is just a joke.  Tell me what name to use if we decide to go this
route.

To be honest, I would probably still go with my v1 patch.

regards,
dan carpenter

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index e0e98b40a6e5d..affcd44f03d6b 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -689,6 +689,7 @@ static ssize_t __write_ports_names(char *buf, struct net *net)
  */
 static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred *cred)
 {
+	struct socket *so;
 	char *mesg = buf;
 	int fd, err;
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
@@ -698,22 +699,30 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
 		return -EINVAL;
 	trace_nfsd_ctl_ports_addfd(net, fd);
 
-	if (svc_alien_sock(net, fd)) {
+	so = svc_get_earth_sock(net, fd);
+	if (!so) {
 		printk(KERN_ERR "%s: socket net is different to NFSd's one\n", __func__);
 		return -EINVAL;
 	}
 
 	err = nfsd_create_serv(net);
 	if (err != 0)
-		return err;
+		goto out_put_sock;
 
-	err = svc_addsock(nn->nfsd_serv, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
+	err = svc_addsock(nn->nfsd_serv, so, buf, SIMPLE_TRANSACTION_LIMIT, cred);
+	if (err)
+		goto out_put_net;
 
-	if (err >= 0 &&
-	    !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
+	if (!nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
 		svc_get(nn->nfsd_serv);
 
 	nfsd_put(net);
+	return 0;
+
+out_put_net:
+	nfsd_put(net);
+out_put_sock:
+	sockfd_put(so);
 	return err;
 }
 
diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
index d16ae621782c0..2422d260591bb 100644
--- a/include/linux/sunrpc/svcsock.h
+++ b/include/linux/sunrpc/svcsock.h
@@ -61,8 +61,8 @@ int		svc_recv(struct svc_rqst *, long);
 void		svc_send(struct svc_rqst *rqstp);
 void		svc_drop(struct svc_rqst *);
 void		svc_sock_update_bufs(struct svc_serv *serv);
-bool		svc_alien_sock(struct net *net, int fd);
-int		svc_addsock(struct svc_serv *serv, const int fd,
+struct socket	*svc_get_earth_sock(struct net *net, int fd);
+int		svc_addsock(struct svc_serv *serv, struct socket *so,
 					char *name_return, const size_t len,
 					const struct cred *cred);
 void		svc_init_xprt_sock(void);
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 46845cb6465d7..78f6ae9fa42d4 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1474,21 +1474,20 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
 	return svsk;
 }
 
-bool svc_alien_sock(struct net *net, int fd)
+struct socket *svc_get_earth_sock(struct net *net, int fd)
 {
 	int err;
 	struct socket *sock = sockfd_lookup(fd, &err);
-	bool ret = false;
 
 	if (!sock)
-		goto out;
-	if (sock_net(sock->sk) != net)
-		ret = true;
-	sockfd_put(sock);
-out:
-	return ret;
+		return NULL;
+	if (sock_net(sock->sk) != net) {
+		sockfd_put(sock);
+		return NULL;
+	}
+	return sock;
 }
-EXPORT_SYMBOL_GPL(svc_alien_sock);
+EXPORT_SYMBOL_GPL(svc_get_earth_sock);
 
 /**
  * svc_addsock - add a listener socket to an RPC service
@@ -1502,36 +1501,27 @@ EXPORT_SYMBOL_GPL(svc_alien_sock);
  * Name is terminated with '\n'.  On error, returns a negative errno
  * value.
  */
-int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
+int svc_addsock(struct svc_serv *serv, struct socket *so, char *name_return,
 		const size_t len, const struct cred *cred)
 {
-	int err = 0;
-	struct socket *so = sockfd_lookup(fd, &err);
 	struct svc_sock *svsk = NULL;
 	struct sockaddr_storage addr;
 	struct sockaddr *sin = (struct sockaddr *)&addr;
 	int salen;
 
-	if (!so)
-		return err;
-	err = -EAFNOSUPPORT;
 	if ((so->sk->sk_family != PF_INET) && (so->sk->sk_family != PF_INET6))
-		goto out;
-	err =  -EPROTONOSUPPORT;
+		return -EAFNOSUPPORT;
 	if (so->sk->sk_protocol != IPPROTO_TCP &&
 	    so->sk->sk_protocol != IPPROTO_UDP)
-		goto out;
-	err = -EISCONN;
+		return -EPROTONOSUPPORT;
 	if (so->state > SS_UNCONNECTED)
-		goto out;
-	err = -ENOENT;
+		return -EISCONN;
 	if (!try_module_get(THIS_MODULE))
-		goto out;
+		return -ENOENT;
 	svsk = svc_setup_socket(serv, so, SVC_SOCK_DEFAULTS);
 	if (IS_ERR(svsk)) {
 		module_put(THIS_MODULE);
-		err = PTR_ERR(svsk);
-		goto out;
+		return PTR_ERR(svsk);
 	}
 	salen = kernel_getsockname(svsk->sk_sock, sin);
 	if (salen >= 0)
@@ -1539,9 +1529,6 @@ int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
 	svsk->sk_xprt.xpt_cred = get_cred(cred);
 	svc_add_new_perm_xprt(serv, &svsk->sk_xprt);
 	return svc_one_sock_name(svsk, name_return, len);
-out:
-	sockfd_put(so);
-	return err;
 }
 EXPORT_SYMBOL_GPL(svc_addsock);
 

Re: [PATCH] nfsd: fix double fget() bug in __write_ports_addfd()

[Dan Carpenter]

On Wed, May 31, 2023 at 10:48:09AM +0300, Dan Carpenter wrote:
>  	err = nfsd_create_serv(net);
>  	if (err != 0)
> -		return err;
> +		goto out_put_sock;
>  
> -	err = svc_addsock(nn->nfsd_serv, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
> +	err = svc_addsock(nn->nfsd_serv, so, buf, SIMPLE_TRANSACTION_LIMIT, cred);
> +	if (err)
> +		goto out_put_net;

Oops.  This change is wrong.  svc_addsock() actually does return
positive values on success.

>  
> -	if (err >= 0 &&
> -	    !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
> +	if (!nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
>  		svc_get(nn->nfsd_serv);
>  
>  	nfsd_put(net);
> +	return 0;

Also wrong (same bug).

> +
> +out_put_net:
> +	nfsd_put(net);
> +out_put_sock:
> +	sockfd_put(so);
>  	return err;
>  }

regards,
dan carpenter

Re: [PATCH] nfsd: fix double fget() bug in __write_ports_addfd()

[Jeff Layton]

On Wed, 2023-05-31 at 10:48 +0300, Dan Carpenter wrote:
> On Wed, May 31, 2023 at 08:27:43AM +1000, NeilBrown wrote:
> > On Mon, 29 May 2023, Dan Carpenter wrote:
> > > The bug here is that you cannot rely on getting the same socket
> > > from multiple calls to fget() because userspace can influence
> > > that.  This is a kind of double fetch bug.
> > > 
> > > The fix is to delete the svc_alien_sock() function and insted do
> > > the checking inside the svc_addsock() function.
> > 
> > Hi,
> >  I definitely agree with the change to pass the 'net' into
> >  svc_addsock(), and check the the fd has the correct net.
> > 
> >  I'm not sure I agree with the removal of the svc_alien_sock() test.  It
> >  is best to perform sanity tests before allocation things, and
> >  nfsd_create_serv() can create a new 'serv' - though most often it just
> >  incs the refcount.
> 
> That's true.  But the other philosophical rule is that we shouldn't
> optimize for the failure path.  If someone gives us bad data they
> deserve a slow down.

> I also think leaving svc_alien_sock() is a trap for the unwary because
> it will lead to more double fget() bugs.  The svc_alien_sock() function
> is weird because it returns false on success and false on failure and
> true for alien sock.
> 
> > 
> >  Maybe instead svc_alien_sock() could return the struct socket (if
> >  successful), and it could be passed to svc_addsock()???
> > 
> >  I would probably then change the name of svc_alien_sock()
> 
> Yeah, because we don't want alien sockets, we want Earth sockets.
> Doing this is much more complicated...  The name svc_get_earth_sock()
> is just a joke.  Tell me what name to use if we decide to go this
> route.
> 
> To be honest, I would probably still go with my v1 patch.
> 

+1.  I don't see a need to do this check twice. Let's optimize for the
success case and if someone sends down bogus data, then they just go
slower.

I too suggest we just go with Dan's original patch.



> regards,
> dan carpenter
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index e0e98b40a6e5d..affcd44f03d6b 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -689,6 +689,7 @@ static ssize_t __write_ports_names(char *buf, struct net *net)
>   */
>  static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred *cred)
>  {
> +	struct socket *so;
>  	char *mesg = buf;
>  	int fd, err;
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> @@ -698,22 +699,30 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
>  		return -EINVAL;
>  	trace_nfsd_ctl_ports_addfd(net, fd);
>  
> -	if (svc_alien_sock(net, fd)) {
> +	so = svc_get_earth_sock(net, fd);
> +	if (!so) {
>  		printk(KERN_ERR "%s: socket net is different to NFSd's one\n", __func__);
>  		return -EINVAL;
>  	}
>  
>  	err = nfsd_create_serv(net);
>  	if (err != 0)
> -		return err;
> +		goto out_put_sock;
>  
> -	err = svc_addsock(nn->nfsd_serv, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
> +	err = svc_addsock(nn->nfsd_serv, so, buf, SIMPLE_TRANSACTION_LIMIT, cred);
> +	if (err)
> +		goto out_put_net;
>  
> -	if (err >= 0 &&
> -	    !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
> +	if (!nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
>  		svc_get(nn->nfsd_serv);
>  
>  	nfsd_put(net);
> +	return 0;
> +
> +out_put_net:
> +	nfsd_put(net);
> +out_put_sock:
> +	sockfd_put(so);
>  	return err;
>  }
>  
> diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
> index d16ae621782c0..2422d260591bb 100644
> --- a/include/linux/sunrpc/svcsock.h
> +++ b/include/linux/sunrpc/svcsock.h
> @@ -61,8 +61,8 @@ int		svc_recv(struct svc_rqst *, long);
>  void		svc_send(struct svc_rqst *rqstp);
>  void		svc_drop(struct svc_rqst *);
>  void		svc_sock_update_bufs(struct svc_serv *serv);
> -bool		svc_alien_sock(struct net *net, int fd);
> -int		svc_addsock(struct svc_serv *serv, const int fd,
> +struct socket	*svc_get_earth_sock(struct net *net, int fd);
> +int		svc_addsock(struct svc_serv *serv, struct socket *so,
>  					char *name_return, const size_t len,
>  					const struct cred *cred);
>  void		svc_init_xprt_sock(void);
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 46845cb6465d7..78f6ae9fa42d4 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -1474,21 +1474,20 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
>  	return svsk;
>  }
>  
> -bool svc_alien_sock(struct net *net, int fd)
> +struct socket *svc_get_earth_sock(struct net *net, int fd)
>  {
>  	int err;
>  	struct socket *sock = sockfd_lookup(fd, &err);
> -	bool ret = false;
>  
>  	if (!sock)
> -		goto out;
> -	if (sock_net(sock->sk) != net)
> -		ret = true;
> -	sockfd_put(sock);
> -out:
> -	return ret;
> +		return NULL;
> +	if (sock_net(sock->sk) != net) {
> +		sockfd_put(sock);
> +		return NULL;
> +	}
> +	return sock;
>  }
> -EXPORT_SYMBOL_GPL(svc_alien_sock);
> +EXPORT_SYMBOL_GPL(svc_get_earth_sock);
>  
>  /**
>   * svc_addsock - add a listener socket to an RPC service
> @@ -1502,36 +1501,27 @@ EXPORT_SYMBOL_GPL(svc_alien_sock);
>   * Name is terminated with '\n'.  On error, returns a negative errno
>   * value.
>   */
> -int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
> +int svc_addsock(struct svc_serv *serv, struct socket *so, char *name_return,
>  		const size_t len, const struct cred *cred)
>  {
> -	int err = 0;
> -	struct socket *so = sockfd_lookup(fd, &err);
>  	struct svc_sock *svsk = NULL;
>  	struct sockaddr_storage addr;
>  	struct sockaddr *sin = (struct sockaddr *)&addr;
>  	int salen;
>  
> -	if (!so)
> -		return err;
> -	err = -EAFNOSUPPORT;
>  	if ((so->sk->sk_family != PF_INET) && (so->sk->sk_family != PF_INET6))
> -		goto out;
> -	err =  -EPROTONOSUPPORT;
> +		return -EAFNOSUPPORT;
>  	if (so->sk->sk_protocol != IPPROTO_TCP &&
>  	    so->sk->sk_protocol != IPPROTO_UDP)
> -		goto out;
> -	err = -EISCONN;
> +		return -EPROTONOSUPPORT;
>  	if (so->state > SS_UNCONNECTED)
> -		goto out;
> -	err = -ENOENT;
> +		return -EISCONN;
>  	if (!try_module_get(THIS_MODULE))
> -		goto out;
> +		return -ENOENT;
>  	svsk = svc_setup_socket(serv, so, SVC_SOCK_DEFAULTS);
>  	if (IS_ERR(svsk)) {
>  		module_put(THIS_MODULE);
> -		err = PTR_ERR(svsk);
> -		goto out;
> +		return PTR_ERR(svsk);
>  	}
>  	salen = kernel_getsockname(svsk->sk_sock, sin);
>  	if (salen >= 0)
> @@ -1539,9 +1529,6 @@ int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
>  	svsk->sk_xprt.xpt_cred = get_cred(cred);
>  	svc_add_new_perm_xprt(serv, &svsk->sk_xprt);
>  	return svc_one_sock_name(svsk, name_return, len);
> -out:
> -	sockfd_put(so);
> -	return err;
>  }
>  EXPORT_SYMBOL_GPL(svc_addsock);
>  
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] nfsd: fix double fget() bug in __write_ports_addfd()

[NeilBrown]

On Wed, 31 May 2023, Dan Carpenter wrote:
> On Wed, May 31, 2023 at 08:27:43AM +1000, NeilBrown wrote:
> > On Mon, 29 May 2023, Dan Carpenter wrote:
> > > The bug here is that you cannot rely on getting the same socket
> > > from multiple calls to fget() because userspace can influence
> > > that.  This is a kind of double fetch bug.
> > > 
> > > The fix is to delete the svc_alien_sock() function and insted do
> > > the checking inside the svc_addsock() function.
> > 
> > Hi,
> >  I definitely agree with the change to pass the 'net' into
> >  svc_addsock(), and check the the fd has the correct net.
> > 
> >  I'm not sure I agree with the removal of the svc_alien_sock() test.  It
> >  is best to perform sanity tests before allocation things, and
> >  nfsd_create_serv() can create a new 'serv' - though most often it just
> >  incs the refcount.
> 
> That's true.  But the other philosophical rule is that we shouldn't
> optimize for the failure path.  If someone gives us bad data they
> deserve a slow down.
> 
> I also think leaving svc_alien_sock() is a trap for the unwary because
> it will lead to more double fget() bugs.  The svc_alien_sock() function
> is weird because it returns false on success and false on failure and
> true for alien sock.

That's alien logic for you!

> 
> > 
> >  Maybe instead svc_alien_sock() could return the struct socket (if
> >  successful), and it could be passed to svc_addsock()???
> > 
> >  I would probably then change the name of svc_alien_sock()
> 
> Yeah, because we don't want alien sockets, we want Earth sockets.
> Doing this is much more complicated...  The name svc_get_earth_sock()
> is just a joke.  Tell me what name to use if we decide to go this
> route.
> 
> To be honest, I would probably still go with my v1 patch.

Thanks for trying it out.  Maybe it's not such a good idea after all.
I'm happy to accept your original.
  Revewied-by: NeilBrown <neilb@suse.com>

Thanks,
NeilBrown

> 
> regards,
> dan carpenter
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index e0e98b40a6e5d..affcd44f03d6b 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -689,6 +689,7 @@ static ssize_t __write_ports_names(char *buf, struct net *net)
>   */
>  static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred *cred)
>  {
> +	struct socket *so;
>  	char *mesg = buf;
>  	int fd, err;
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> @@ -698,22 +699,30 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
>  		return -EINVAL;
>  	trace_nfsd_ctl_ports_addfd(net, fd);
>  
> -	if (svc_alien_sock(net, fd)) {
> +	so = svc_get_earth_sock(net, fd);
> +	if (!so) {
>  		printk(KERN_ERR "%s: socket net is different to NFSd's one\n", __func__);
>  		return -EINVAL;
>  	}
>  
>  	err = nfsd_create_serv(net);
>  	if (err != 0)
> -		return err;
> +		goto out_put_sock;
>  
> -	err = svc_addsock(nn->nfsd_serv, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
> +	err = svc_addsock(nn->nfsd_serv, so, buf, SIMPLE_TRANSACTION_LIMIT, cred);
> +	if (err)
> +		goto out_put_net;
>  
> -	if (err >= 0 &&
> -	    !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
> +	if (!nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
>  		svc_get(nn->nfsd_serv);
>  
>  	nfsd_put(net);
> +	return 0;
> +
> +out_put_net:
> +	nfsd_put(net);
> +out_put_sock:
> +	sockfd_put(so);
>  	return err;
>  }
>  
> diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
> index d16ae621782c0..2422d260591bb 100644
> --- a/include/linux/sunrpc/svcsock.h
> +++ b/include/linux/sunrpc/svcsock.h
> @@ -61,8 +61,8 @@ int		svc_recv(struct svc_rqst *, long);
>  void		svc_send(struct svc_rqst *rqstp);
>  void		svc_drop(struct svc_rqst *);
>  void		svc_sock_update_bufs(struct svc_serv *serv);
> -bool		svc_alien_sock(struct net *net, int fd);
> -int		svc_addsock(struct svc_serv *serv, const int fd,
> +struct socket	*svc_get_earth_sock(struct net *net, int fd);
> +int		svc_addsock(struct svc_serv *serv, struct socket *so,
>  					char *name_return, const size_t len,
>  					const struct cred *cred);
>  void		svc_init_xprt_sock(void);
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 46845cb6465d7..78f6ae9fa42d4 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -1474,21 +1474,20 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
>  	return svsk;
>  }
>  
> -bool svc_alien_sock(struct net *net, int fd)
> +struct socket *svc_get_earth_sock(struct net *net, int fd)
>  {
>  	int err;
>  	struct socket *sock = sockfd_lookup(fd, &err);
> -	bool ret = false;
>  
>  	if (!sock)
> -		goto out;
> -	if (sock_net(sock->sk) != net)
> -		ret = true;
> -	sockfd_put(sock);
> -out:
> -	return ret;
> +		return NULL;
> +	if (sock_net(sock->sk) != net) {
> +		sockfd_put(sock);
> +		return NULL;
> +	}
> +	return sock;
>  }
> -EXPORT_SYMBOL_GPL(svc_alien_sock);
> +EXPORT_SYMBOL_GPL(svc_get_earth_sock);
>  
>  /**
>   * svc_addsock - add a listener socket to an RPC service
> @@ -1502,36 +1501,27 @@ EXPORT_SYMBOL_GPL(svc_alien_sock);
>   * Name is terminated with '\n'.  On error, returns a negative errno
>   * value.
>   */
> -int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
> +int svc_addsock(struct svc_serv *serv, struct socket *so, char *name_return,
>  		const size_t len, const struct cred *cred)
>  {
> -	int err = 0;
> -	struct socket *so = sockfd_lookup(fd, &err);
>  	struct svc_sock *svsk = NULL;
>  	struct sockaddr_storage addr;
>  	struct sockaddr *sin = (struct sockaddr *)&addr;
>  	int salen;
>  
> -	if (!so)
> -		return err;
> -	err = -EAFNOSUPPORT;
>  	if ((so->sk->sk_family != PF_INET) && (so->sk->sk_family != PF_INET6))
> -		goto out;
> -	err =  -EPROTONOSUPPORT;
> +		return -EAFNOSUPPORT;
>  	if (so->sk->sk_protocol != IPPROTO_TCP &&
>  	    so->sk->sk_protocol != IPPROTO_UDP)
> -		goto out;
> -	err = -EISCONN;
> +		return -EPROTONOSUPPORT;
>  	if (so->state > SS_UNCONNECTED)
> -		goto out;
> -	err = -ENOENT;
> +		return -EISCONN;
>  	if (!try_module_get(THIS_MODULE))
> -		goto out;
> +		return -ENOENT;
>  	svsk = svc_setup_socket(serv, so, SVC_SOCK_DEFAULTS);
>  	if (IS_ERR(svsk)) {
>  		module_put(THIS_MODULE);
> -		err = PTR_ERR(svsk);
> -		goto out;
> +		return PTR_ERR(svsk);
>  	}
>  	salen = kernel_getsockname(svsk->sk_sock, sin);
>  	if (salen >= 0)
> @@ -1539,9 +1529,6 @@ int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
>  	svsk->sk_xprt.xpt_cred = get_cred(cred);
>  	svc_add_new_perm_xprt(serv, &svsk->sk_xprt);
>  	return svc_one_sock_name(svsk, name_return, len);
> -out:
> -	sockfd_put(so);
> -	return err;
>  }
>  EXPORT_SYMBOL_GPL(svc_addsock);
>  
> 
> 
> 
>