[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-id: <168548566376.23533.14778348024215909777@noble.neil.brown.name>
Date: Wed, 31 May 2023 08:27:43 +1000
From: "NeilBrown" <neilb@...e.de>
To: "Dan Carpenter" <dan.carpenter@...aro.org>
Cc: "Stanislav Kinsbursky" <skinsbursky@...allels.com>,
"Chuck Lever" <chuck.lever@...cle.com>, "Jeff Layton" <jlayton@...nel.org>,
"Trond Myklebust" <trond.myklebust@...merspace.com>,
"Anna Schumaker" <anna@...nel.org>, "J. Bruce Fields" <bfields@...hat.com>,
linux-nfs@...r.kernel.org, netdev@...r.kernel.org,
kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] nfsd: fix double fget() bug in __write_ports_addfd()
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@...aro.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
>
>
Powered by blists - more mailing lists