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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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
>
>
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);
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
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 <[email protected]>
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 <[email protected]>
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);
>
>
>
>
>