| Message ID | 9c90e813-c7fb-4c90-b52b-131481640a78@kili.mountain (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | nfsd: fix double fget() bug in __write_ports_addfd() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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>
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 > >
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); > > > >
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); > > > > >
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;
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(-)