| Message ID | 20220121142616.163592-3-cascardo@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | CVE-2021-4083 | expand |
On 21.01.22 15:26, Thadeu Lima de Souza Cascardo wrote: > From: Linus Torvalds <torvalds@linux-foundation.org> > > Jann Horn points out that there is another possible race wrt Unix domain > socket garbage collection, somewhat reminiscent of the one fixed in > commit cbcf01128d0a ("af_unix: fix garbage collect vs MSG_PEEK"). > > See the extended comment about the garbage collection requirements added > to unix_peek_fds() by that commit for details. > > The race comes from how we can locklessly look up a file descriptor just > as it is in the process of being closed, and with the right artificial > timing (Jann added a few strategic 'mdelay(500)' calls to do that), the > Unix domain socket garbage collector could see the reference count > decrement of the close() happen before fget() took its reference to the > file and the file was attached onto a new file descriptor. > > This is all (intentionally) correct on the 'struct file *' side, with > RCU lookups and lockless reference counting very much part of the > design. Getting that reference count out of order isn't a problem per > se. > > But the garbage collector can get confused by seeing this situation of > having seen a file not having any remaining external references and then > seeing it being attached to an fd. > > In commit cbcf01128d0a ("af_unix: fix garbage collect vs MSG_PEEK") the > fix was to serialize the file descriptor install with the garbage > collector by taking and releasing the unix_gc_lock. > > That's not really an option here, but since this all happens when we are > in the process of looking up a file descriptor, we can instead simply > just re-check that the file hasn't been closed in the meantime, and just > re-do the lookup if we raced with a concurrent close() of the same file > descriptor. > > Reported-and-tested-by: Jann Horn <jannh@google.com> > Acked-by: Miklos Szeredi <mszeredi@redhat.com> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > (backported from commit 054aa8d439b9185d4f5eb9a90282d1ce74772969) > [cascardo: __fcheck_files was renamed to files_lookup_fd_raw at > bebf684bf330 ("file: Rename __fcheck_files to files_lookup_fd_raw")] > CVE-2021-4083 > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > --- Applied to focal:linux/master-next. Thanks. -Stefan > fs/file.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/file.c b/fs/file.c > index 3c4b9cf7ef17..5913a5db9c2a 100644 > --- a/fs/file.c > +++ b/fs/file.c > @@ -698,6 +698,10 @@ static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs) > file = NULL; > else if (!get_file_rcu_many(file, refs)) > goto loop; > + else if (__fcheck_files(files, fd) != file) { > + fput_many(file, refs); > + goto loop; > + } > } > rcu_read_unlock(); >
On 21.01.22 15:26, Thadeu Lima de Souza Cascardo wrote: > From: Linus Torvalds <torvalds@linux-foundation.org> > > Jann Horn points out that there is another possible race wrt Unix domain > socket garbage collection, somewhat reminiscent of the one fixed in > commit cbcf01128d0a ("af_unix: fix garbage collect vs MSG_PEEK"). > > See the extended comment about the garbage collection requirements added > to unix_peek_fds() by that commit for details. > > The race comes from how we can locklessly look up a file descriptor just > as it is in the process of being closed, and with the right artificial > timing (Jann added a few strategic 'mdelay(500)' calls to do that), the > Unix domain socket garbage collector could see the reference count > decrement of the close() happen before fget() took its reference to the > file and the file was attached onto a new file descriptor. > > This is all (intentionally) correct on the 'struct file *' side, with > RCU lookups and lockless reference counting very much part of the > design. Getting that reference count out of order isn't a problem per > se. > > But the garbage collector can get confused by seeing this situation of > having seen a file not having any remaining external references and then > seeing it being attached to an fd. > > In commit cbcf01128d0a ("af_unix: fix garbage collect vs MSG_PEEK") the > fix was to serialize the file descriptor install with the garbage > collector by taking and releasing the unix_gc_lock. > > That's not really an option here, but since this all happens when we are > in the process of looking up a file descriptor, we can instead simply > just re-check that the file hasn't been closed in the meantime, and just > re-do the lookup if we raced with a concurrent close() of the same file > descriptor. > > Reported-and-tested-by: Jann Horn <jannh@google.com> > Acked-by: Miklos Szeredi <mszeredi@redhat.com> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > (backported from commit 054aa8d439b9185d4f5eb9a90282d1ce74772969) > [cascardo: __fcheck_files was renamed to files_lookup_fd_raw at > bebf684bf330 ("file: Rename __fcheck_files to files_lookup_fd_raw")] > CVE-2021-4083 > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > --- Applied to bionic:linux/master-next (2/2). Thanks. -Stefan > fs/file.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/file.c b/fs/file.c > index 3c4b9cf7ef17..5913a5db9c2a 100644 > --- a/fs/file.c > +++ b/fs/file.c > @@ -698,6 +698,10 @@ static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs) > file = NULL; > else if (!get_file_rcu_many(file, refs)) > goto loop; > + else if (__fcheck_files(files, fd) != file) { > + fput_many(file, refs); > + goto loop; > + } > } > rcu_read_unlock(); >
diff --git a/fs/file.c b/fs/file.c index 3c4b9cf7ef17..5913a5db9c2a 100644 --- a/fs/file.c +++ b/fs/file.c @@ -698,6 +698,10 @@ static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs) file = NULL; else if (!get_file_rcu_many(file, refs)) goto loop; + else if (__fcheck_files(files, fd) != file) { + fput_many(file, refs); + goto loop; + } } rcu_read_unlock();