Message ID | 20220121142616.163592-4-cascardo@canonical.com |
---|---|
State | New |
Headers | show |
Series | [SRU,Impish,Hirsute] fget: check that the fd still exists after getting a ref to it | 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> > (cherry picked from commit 054aa8d439b9185d4f5eb9a90282d1ce74772969) > CVE-2021-4083 > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > --- Applied to focal:linux-hwe-5.11/hwe-5.11-next instead of hirsute:linux. Thanks. -Stefan > fs/file.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/file.c b/fs/file.c > index 3cc1867f289f..f18e4746523b 100644 > --- a/fs/file.c > +++ b/fs/file.c > @@ -843,6 +843,10 @@ static struct file *__fget_files(struct files_struct *files, unsigned int fd, > file = NULL; > else if (!get_file_rcu_many(file, refs)) > goto loop; > + else if (files_lookup_fd_raw(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> > (cherry picked from commit 054aa8d439b9185d4f5eb9a90282d1ce74772969) > CVE-2021-4083 > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > --- Applied to impish: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 3cc1867f289f..f18e4746523b 100644 > --- a/fs/file.c > +++ b/fs/file.c > @@ -843,6 +843,10 @@ static struct file *__fget_files(struct files_struct *files, unsigned int fd, > file = NULL; > else if (!get_file_rcu_many(file, refs)) > goto loop; > + else if (files_lookup_fd_raw(files, fd) != file) { > + fput_many(file, refs); > + goto loop; > + } > } > rcu_read_unlock(); >
diff --git a/fs/file.c b/fs/file.c index 3cc1867f289f..f18e4746523b 100644 --- a/fs/file.c +++ b/fs/file.c @@ -843,6 +843,10 @@ static struct file *__fget_files(struct files_struct *files, unsigned int fd, file = NULL; else if (!get_file_rcu_many(file, refs)) goto loop; + else if (files_lookup_fd_raw(files, fd) != file) { + fput_many(file, refs); + goto loop; + } } rcu_read_unlock();