From c1427a04dfba87bd2ebce767086b5351c725db25 Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Tue, 25 Oct 2022 21:42:58 -0700 Subject: [PATCH] Disable fasync for signalfd descriptors In Linux, signalfd doesn't support fasync events. Reported-by: syzbot+eeb463868529314bd733@syzkaller.appspotmail.com PiperOrigin-RevId: 483861398 --- pkg/sentry/fsimpl/signalfd/signalfd.go | 11 ++++++++++ pkg/sentry/vfs/file_description.go | 9 +++++--- pkg/sentry/vfs/file_description_impl_util.go | 22 ++++++++++++++++++++ test/syscalls/linux/fcntl.cc | 14 +++++++++++++ test/syscalls/linux/signalfd.cc | 10 --------- test/util/BUILD | 1 + test/util/signal_util.cc | 11 ++++++++++ test/util/signal_util.h | 4 ++++ 8 files changed, 69 insertions(+), 13 deletions(-) diff --git a/pkg/sentry/fsimpl/signalfd/signalfd.go b/pkg/sentry/fsimpl/signalfd/signalfd.go index 0954d2e47..38f56d6f8 100644 --- a/pkg/sentry/fsimpl/signalfd/signalfd.go +++ b/pkg/sentry/fsimpl/signalfd/signalfd.go @@ -34,6 +34,7 @@ type SignalFileDescription struct { vfs.FileDescriptionDefaultImpl vfs.DentryMetadataFileDescriptionImpl vfs.NoLockFD + vfs.NoAsyncEventFD // target is the original signal target task. // @@ -155,3 +156,13 @@ func (sfd *SignalFileDescription) Epollable() bool { func (sfd *SignalFileDescription) Release(context.Context) { sfd.target.SignalUnregister(&sfd.entry) } + +// RegisterFileAsyncHandler implements vfs.FileDescriptionImpl.RegisterFileAsyncHandler. +func (sfd *SignalFileDescription) RegisterFileAsyncHandler(fd *vfs.FileDescription) error { + return sfd.NoAsyncEventFD.RegisterFileAsyncHandler(fd) +} + +// UnregisterFileAsyncHandler implements vfs.FileDescriptionImpl.UnregisterFileAsyncHandler. +func (sfd *SignalFileDescription) UnregisterFileAsyncHandler(fd *vfs.FileDescription) { + sfd.NoAsyncEventFD.UnregisterFileAsyncHandler(fd) +} diff --git a/pkg/sentry/vfs/file_description.go b/pkg/sentry/vfs/file_description.go index a19773076..0c1d89056 100644 --- a/pkg/sentry/vfs/file_description.go +++ b/pkg/sentry/vfs/file_description.go @@ -196,7 +196,7 @@ func (fd *FileDescription) DecRef(ctx context.Context) { fd.vd.DecRef(ctx) fd.flagsMu.Lock() if fd.statusFlags.RacyLoad()&linux.O_ASYNC != 0 && fd.asyncHandler != nil { - fd.asyncHandler.Unregister(fd) + fd.impl.UnregisterFileAsyncHandler(fd) } fd.asyncHandler = nil fd.flagsMu.Unlock() @@ -280,11 +280,11 @@ func (fd *FileDescription) SetStatusFlags(ctx context.Context, creds *auth.Crede // Use fd.statusFlags instead of oldFlags, which may have become outdated, // to avoid double registering/unregistering. if fd.statusFlags.RacyLoad()&linux.O_ASYNC == 0 && flags&linux.O_ASYNC != 0 { - if err := fd.asyncHandler.Register(fd); err != nil { + if err := fd.impl.RegisterFileAsyncHandler(fd); err != nil { return err } } else if fd.statusFlags.RacyLoad()&linux.O_ASYNC != 0 && flags&linux.O_ASYNC == 0 { - fd.asyncHandler.Unregister(fd) + fd.impl.UnregisterFileAsyncHandler(fd) } } fd.statusFlags.Store((oldFlags &^ settableFlags) | (flags & settableFlags)) @@ -474,6 +474,9 @@ type FileDescriptionImpl interface { // TestPOSIX returns information about whether the specified lock can be held, in the style of the F_GETLK fcntl. TestPOSIX(ctx context.Context, uid lock.UniqueID, t lock.LockType, r lock.LockRange) (linux.Flock, error) + + RegisterFileAsyncHandler(fd *FileDescription) error + UnregisterFileAsyncHandler(fd *FileDescription) } // Dirent holds the information contained in struct linux_dirent64. diff --git a/pkg/sentry/vfs/file_description_impl_util.go b/pkg/sentry/vfs/file_description_impl_util.go index 19b5d9515..1cd3fd0f6 100644 --- a/pkg/sentry/vfs/file_description_impl_util.go +++ b/pkg/sentry/vfs/file_description_impl_util.go @@ -172,6 +172,16 @@ func (FileDescriptionDefaultImpl) RemoveXattr(ctx context.Context, name string) return linuxerr.ENOTSUP } +// RegisterFileAsyncHandler implements FileDescriptionImpl.RegisterFileAsyncHandler. +func (FileDescriptionDefaultImpl) RegisterFileAsyncHandler(fd *FileDescription) error { + return fd.asyncHandler.Register(fd) +} + +// UnregisterFileAsyncHandler implements FileDescriptionImpl.UnregisterFileAsyncHandler. +func (FileDescriptionDefaultImpl) UnregisterFileAsyncHandler(fd *FileDescription) { + fd.asyncHandler.Unregister(fd) +} + // DirectoryFileDescriptionDefaultImpl may be embedded by implementations of // FileDescriptionImpl that always represent directories to obtain // implementations of non-directory I/O methods that return EISDIR. @@ -466,6 +476,18 @@ func (fd *LockFD) TestPOSIX(ctx context.Context, uid fslock.UniqueID, t fslock.L return fd.locks.TestPOSIX(ctx, uid, t, r) } +// NoAsyncEventFD implements [Un]RegisterFileAsyncHandler of FileDescriptionImpl. +type NoAsyncEventFD struct{} + +// RegisterFileAsyncHandler implements FileDescriptionImpl.RegisterFileAsyncHandler. +func (NoAsyncEventFD) RegisterFileAsyncHandler(fd *FileDescription) error { + return nil +} + +// UnregisterFileAsyncHandler implements FileDescriptionImpl.UnregisterFileAsyncHandler. +func (NoAsyncEventFD) UnregisterFileAsyncHandler(fd *FileDescription) { +} + // NoLockFD implements Lock*/Unlock* portion of FileDescriptionImpl interface // returning ENOLCK. // diff --git a/test/syscalls/linux/fcntl.cc b/test/syscalls/linux/fcntl.cc index ac4dd4899..94696409e 100644 --- a/test/syscalls/linux/fcntl.cc +++ b/test/syscalls/linux/fcntl.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -1496,6 +1497,19 @@ TEST_F(FcntlSignalTest, SetSigDefault) { // siginfo contents is undefined in this case. } +TEST_F(FcntlSignalTest, SignalFD) { + // Create the signalfd. + sigset_t mask; + sigemptyset(&mask); + sigaddset(&mask, SIGIO); + FileDescriptor fd = ASSERT_NO_ERRNO_AND_VALUE(NewSignalFD(&mask, 0)); + const auto signal_cleanup = + ASSERT_NO_ERRNO_AND_VALUE(RegisterSignalHandler(SIGIO)); + RegisterFD(fd.get(), 0); + int tid = syscall(SYS_gettid); + syscall(SYS_tkill, tid, SIGIO); +} + TEST_F(FcntlSignalTest, SetSigCustom) { const auto signal_cleanup = ASSERT_NO_ERRNO_AND_VALUE(RegisterSignalHandler(SIGUSR1)); diff --git a/test/syscalls/linux/signalfd.cc b/test/syscalls/linux/signalfd.cc index c86cd2755..0af733ba2 100644 --- a/test/syscalls/linux/signalfd.cc +++ b/test/syscalls/linux/signalfd.cc @@ -42,16 +42,6 @@ constexpr int kSigno = SIGUSR1; constexpr int kSignoMax = 64; // SIGRTMAX constexpr int kSignoAlt = SIGUSR2; -// Returns a new signalfd. -inline PosixErrorOr NewSignalFD(sigset_t* mask, int flags = 0) { - int fd = signalfd(-1, mask, flags); - MaybeSave(); - if (fd < 0) { - return PosixError(errno, "signalfd"); - } - return FileDescriptor(fd); -} - class SignalfdTest : public ::testing::TestWithParam {}; TEST_P(SignalfdTest, Basic) { diff --git a/test/util/BUILD b/test/util/BUILD index 07b901ca5..e309b50e0 100644 --- a/test/util/BUILD +++ b/test/util/BUILD @@ -263,6 +263,7 @@ cc_library( hdrs = ["signal_util.h"], deps = [ ":cleanup", + ":file_descriptor", ":posix_error", ":test_util", gtest, diff --git a/test/util/signal_util.cc b/test/util/signal_util.cc index 5ee95ee80..41f71493f 100644 --- a/test/util/signal_util.cc +++ b/test/util/signal_util.cc @@ -15,6 +15,7 @@ #include "test/util/signal_util.h" #include +#include #include @@ -100,5 +101,15 @@ PosixErrorOr ScopedSignalMask(int how, sigset_t const& set) { }); } +// Returns a new signalfd. +PosixErrorOr NewSignalFD(sigset_t* mask, int flags) { + int fd = signalfd(-1, mask, flags); + MaybeSave(); + if (fd < 0) { + return PosixError(errno, "signalfd"); + } + return FileDescriptor(fd); +} + } // namespace testing } // namespace gvisor diff --git a/test/util/signal_util.h b/test/util/signal_util.h index 20eebd7e4..49730e4a8 100644 --- a/test/util/signal_util.h +++ b/test/util/signal_util.h @@ -23,6 +23,7 @@ #include "gmock/gmock.h" #include "test/util/cleanup.h" +#include "test/util/file_descriptor.h" #include "test/util/posix_error.h" // Format a sigset_t as a comma separated list of numeric ranges. @@ -101,6 +102,9 @@ inline void FixupFault(ucontext_t* ctx) { } #endif +// Wrapper around signalfd(2) that returns a FileDescriptor. +PosixErrorOr NewSignalFD(sigset_t* mask, int flags = 0); + } // namespace testing } // namespace gvisor