Message ID | 20180705040415.7962-2-khalid.elmously@canonical.com |
---|---|
State | New |
Headers | show |
Series | [SRU,A/B] PATCH 1/1] socket: close race condition between sock_close() and sockfs_setattr() | expand |
On 05.07.2018 06:04, Khalid Elmously wrote: > From: Cong Wang <xiyou.wangcong@gmail.com> > > CVE-2018-12232 > > fchownat() doesn't even hold refcnt of fd until it figures out > fd is really needed (otherwise is ignored) and releases it after > it resolves the path. This means sock_close() could race with > sockfs_setattr(), which leads to a NULL pointer dereference > since typically we set sock->sk to NULL in ->release(). > > As pointed out by Al, this is unique to sockfs. So we can fix this > in socket layer by acquiring inode_lock in sock_close() and > checking against NULL in sockfs_setattr(). > > sock_release() is called in many places, only the sock_close() > path matters here. And fortunately, this should not affect normal > sock_close() as it is only called when the last fd refcnt is gone. > It only affects sock_close() with a parallel sockfs_setattr() in > progress, which is not common. > > Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.") > Reported-by: shankarapailoor <shankarapailoor@gmail.com> > Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> > Cc: Lorenzo Colitti <lorenzo@google.com> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry-picked from 6d8c50dcb029872b298eea68cc6209c866fd3e14) > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- Only medium priority, so no longer caring for Artful. About Xenial and Trusty, I think you are correct and this is a problem with the tools. I think I remember that one should only use full length sha1 and this one has a shortened as the breaks (pointing to 4.10). -Stefan > net/socket.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/net/socket.c b/net/socket.c > index 6f05d5c4bf30..f2957aa205e6 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -544,7 +544,10 @@ static int sockfs_setattr(struct dentry *dentry, struct iattr *iattr) > if (!err && (iattr->ia_valid & ATTR_UID)) { > struct socket *sock = SOCKET_I(d_inode(dentry)); > > - sock->sk->sk_uid = iattr->ia_uid; > + if (sock->sk) > + sock->sk->sk_uid = iattr->ia_uid; > + else > + err = -ENOENT; > } > > return err; > @@ -594,12 +597,16 @@ EXPORT_SYMBOL(sock_alloc); > * an inode not a file. > */ > > -void sock_release(struct socket *sock) > +static void __sock_release(struct socket *sock, struct inode *inode) > { > if (sock->ops) { > struct module *owner = sock->ops->owner; > > + if (inode) > + inode_lock(inode); > sock->ops->release(sock); > + if (inode) > + inode_unlock(inode); > sock->ops = NULL; > module_put(owner); > } > @@ -614,6 +621,11 @@ void sock_release(struct socket *sock) > } > sock->file = NULL; > } > + > +void sock_release(struct socket *sock) > +{ > + __sock_release(sock, NULL); > +} > EXPORT_SYMBOL(sock_release); > > void __sock_tx_timestamp(__u16 tsflags, __u8 *tx_flags) > @@ -1128,7 +1140,7 @@ static int sock_mmap(struct file *file, struct vm_area_struct *vma) > > static int sock_close(struct inode *inode, struct file *filp) > { > - sock_release(SOCKET_I(inode)); > + __sock_release(SOCKET_I(inode), inode); > return 0; > } > >
On 07/05/18 06:04, Khalid Elmously wrote: > From: Cong Wang <xiyou.wangcong@gmail.com> > > CVE-2018-12232 > > fchownat() doesn't even hold refcnt of fd until it figures out > fd is really needed (otherwise is ignored) and releases it after > it resolves the path. This means sock_close() could race with > sockfs_setattr(), which leads to a NULL pointer dereference > since typically we set sock->sk to NULL in ->release(). > > As pointed out by Al, this is unique to sockfs. So we can fix this > in socket layer by acquiring inode_lock in sock_close() and > checking against NULL in sockfs_setattr(). > > sock_release() is called in many places, only the sock_close() > path matters here. And fortunately, this should not affect normal > sock_close() as it is only called when the last fd refcnt is gone. > It only affects sock_close() with a parallel sockfs_setattr() in > progress, which is not common. > > Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.") > Reported-by: shankarapailoor <shankarapailoor@gmail.com> > Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> > Cc: Lorenzo Colitti <lorenzo@google.com> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry-picked from 6d8c50dcb029872b298eea68cc6209c866fd3e14) > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> Artful is EOL, for Bionic: Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> PS: the CVE tracker was updated and showing only Bionic as affected. thanks! > --- > net/socket.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/net/socket.c b/net/socket.c > index 6f05d5c4bf30..f2957aa205e6 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -544,7 +544,10 @@ static int sockfs_setattr(struct dentry *dentry, struct iattr *iattr) > if (!err && (iattr->ia_valid & ATTR_UID)) { > struct socket *sock = SOCKET_I(d_inode(dentry)); > > - sock->sk->sk_uid = iattr->ia_uid; > + if (sock->sk) > + sock->sk->sk_uid = iattr->ia_uid; > + else > + err = -ENOENT; > } > > return err; > @@ -594,12 +597,16 @@ EXPORT_SYMBOL(sock_alloc); > * an inode not a file. > */ > > -void sock_release(struct socket *sock) > +static void __sock_release(struct socket *sock, struct inode *inode) > { > if (sock->ops) { > struct module *owner = sock->ops->owner; > > + if (inode) > + inode_lock(inode); > sock->ops->release(sock); > + if (inode) > + inode_unlock(inode); > sock->ops = NULL; > module_put(owner); > } > @@ -614,6 +621,11 @@ void sock_release(struct socket *sock) > } > sock->file = NULL; > } > + > +void sock_release(struct socket *sock) > +{ > + __sock_release(sock, NULL); > +} > EXPORT_SYMBOL(sock_release); > > void __sock_tx_timestamp(__u16 tsflags, __u8 *tx_flags) > @@ -1128,7 +1140,7 @@ static int sock_mmap(struct file *file, struct vm_area_struct *vma) > > static int sock_close(struct inode *inode, struct file *filp) > { > - sock_release(SOCKET_I(inode)); > + __sock_release(SOCKET_I(inode), inode); > return 0; > } > >
On 07/05/18 06:04, Khalid Elmously wrote: > From: Cong Wang <xiyou.wangcong@gmail.com> > > CVE-2018-12232 > > fchownat() doesn't even hold refcnt of fd until it figures out > fd is really needed (otherwise is ignored) and releases it after > it resolves the path. This means sock_close() could race with > sockfs_setattr(), which leads to a NULL pointer dereference > since typically we set sock->sk to NULL in ->release(). > > As pointed out by Al, this is unique to sockfs. So we can fix this > in socket layer by acquiring inode_lock in sock_close() and > checking against NULL in sockfs_setattr(). > > sock_release() is called in many places, only the sock_close() > path matters here. And fortunately, this should not affect normal > sock_close() as it is only called when the last fd refcnt is gone. > It only affects sock_close() with a parallel sockfs_setattr() in > progress, which is not common. > > Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.") > Reported-by: shankarapailoor <shankarapailoor@gmail.com> > Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> > Cc: Lorenzo Colitti <lorenzo@google.com> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry-picked from 6d8c50dcb029872b298eea68cc6209c866fd3e14) > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> > --- > net/socket.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/net/socket.c b/net/socket.c > index 6f05d5c4bf30..f2957aa205e6 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -544,7 +544,10 @@ static int sockfs_setattr(struct dentry *dentry, struct iattr *iattr) > if (!err && (iattr->ia_valid & ATTR_UID)) { > struct socket *sock = SOCKET_I(d_inode(dentry)); > > - sock->sk->sk_uid = iattr->ia_uid; > + if (sock->sk) > + sock->sk->sk_uid = iattr->ia_uid; > + else > + err = -ENOENT; > } > > return err; > @@ -594,12 +597,16 @@ EXPORT_SYMBOL(sock_alloc); > * an inode not a file. > */ > > -void sock_release(struct socket *sock) > +static void __sock_release(struct socket *sock, struct inode *inode) > { > if (sock->ops) { > struct module *owner = sock->ops->owner; > > + if (inode) > + inode_lock(inode); > sock->ops->release(sock); > + if (inode) > + inode_unlock(inode); > sock->ops = NULL; > module_put(owner); > } > @@ -614,6 +621,11 @@ void sock_release(struct socket *sock) > } > sock->file = NULL; > } > + > +void sock_release(struct socket *sock) > +{ > + __sock_release(sock, NULL); > +} > EXPORT_SYMBOL(sock_release); > > void __sock_tx_timestamp(__u16 tsflags, __u8 *tx_flags) > @@ -1128,7 +1140,7 @@ static int sock_mmap(struct file *file, struct vm_area_struct *vma) > > static int sock_close(struct inode *inode, struct file *filp) > { > - sock_release(SOCKET_I(inode)); > + __sock_release(SOCKET_I(inode), inode); > return 0; > } > > Applied to bionic/master-next branch. Thanks, Kleber
diff --git a/net/socket.c b/net/socket.c index 6f05d5c4bf30..f2957aa205e6 100644 --- a/net/socket.c +++ b/net/socket.c @@ -544,7 +544,10 @@ static int sockfs_setattr(struct dentry *dentry, struct iattr *iattr) if (!err && (iattr->ia_valid & ATTR_UID)) { struct socket *sock = SOCKET_I(d_inode(dentry)); - sock->sk->sk_uid = iattr->ia_uid; + if (sock->sk) + sock->sk->sk_uid = iattr->ia_uid; + else + err = -ENOENT; } return err; @@ -594,12 +597,16 @@ EXPORT_SYMBOL(sock_alloc); * an inode not a file. */ -void sock_release(struct socket *sock) +static void __sock_release(struct socket *sock, struct inode *inode) { if (sock->ops) { struct module *owner = sock->ops->owner; + if (inode) + inode_lock(inode); sock->ops->release(sock); + if (inode) + inode_unlock(inode); sock->ops = NULL; module_put(owner); } @@ -614,6 +621,11 @@ void sock_release(struct socket *sock) } sock->file = NULL; } + +void sock_release(struct socket *sock) +{ + __sock_release(sock, NULL); +} EXPORT_SYMBOL(sock_release); void __sock_tx_timestamp(__u16 tsflags, __u8 *tx_flags) @@ -1128,7 +1140,7 @@ static int sock_mmap(struct file *file, struct vm_area_struct *vma) static int sock_close(struct inode *inode, struct file *filp) { - sock_release(SOCKET_I(inode)); + __sock_release(SOCKET_I(inode), inode); return 0; }