| Message ID | 20210624152515.1844133-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] userfaultfd: open userfaultfds with O_RDONLY | expand |
On Thu, Jun 24, 2021 at 5:25 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > Since userfaultfd doesn't implement a write operation, it is more > appropriate to open it read-only. > > When userfaultfds are opened read-write like it is now, and such fd is > passed from one process to another, SELinux will check both read and > write permissions for the target process, even though it can't actually > do any write operation on the fd later. > > Inspired by the following bug report, which has hit the SELinux scenario > described above: > https://bugzilla.redhat.com/show_bug.cgi?id=1974559 > > Reported-by: Robert O'Callahan <roc@ocallahan.org> > Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization") > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > > I marked this as RFC, because I'm not sure if this has any unwanted side > effects. I only ran this patch through selinux-testsuite, which has a > simple userfaultfd subtest, and a reproducer from the Bugzilla report. > > Please tell me whether this makes sense and/or if it passes any > userfaultfd tests you guys might have. > > fs/userfaultfd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c > index 14f92285d04f..24e14c36068f 100644 > --- a/fs/userfaultfd.c > +++ b/fs/userfaultfd.c > @@ -986,7 +986,7 @@ static int resolve_userfault_fork(struct userfaultfd_ctx *new, > int fd; > > fd = anon_inode_getfd_secure("[userfaultfd]", &userfaultfd_fops, new, > - O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS), inode); > + O_RDONLY | (new->flags & UFFD_SHARED_FCNTL_FLAGS), inode); > if (fd < 0) > return fd; > > @@ -2088,7 +2088,7 @@ SYSCALL_DEFINE1(userfaultfd, int, flags) > mmgrab(ctx->mm); > > fd = anon_inode_getfd_secure("[userfaultfd]", &userfaultfd_fops, ctx, > - O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS), NULL); > + O_RDONLY | (flags & UFFD_SHARED_FCNTL_FLAGS), NULL); > if (fd < 0) { > mmdrop(ctx->mm); > kmem_cache_free(userfaultfd_ctx_cachep, ctx); > -- > 2.31.1 Ping? Any comments on this patch?
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index 14f92285d04f..24e14c36068f 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -986,7 +986,7 @@ static int resolve_userfault_fork(struct userfaultfd_ctx *new, int fd; fd = anon_inode_getfd_secure("[userfaultfd]", &userfaultfd_fops, new, - O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS), inode); + O_RDONLY | (new->flags & UFFD_SHARED_FCNTL_FLAGS), inode); if (fd < 0) return fd; @@ -2088,7 +2088,7 @@ SYSCALL_DEFINE1(userfaultfd, int, flags) mmgrab(ctx->mm); fd = anon_inode_getfd_secure("[userfaultfd]", &userfaultfd_fops, ctx, - O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS), NULL); + O_RDONLY | (flags & UFFD_SHARED_FCNTL_FLAGS), NULL); if (fd < 0) { mmdrop(ctx->mm); kmem_cache_free(userfaultfd_ctx_cachep, ctx);
Since userfaultfd doesn't implement a write operation, it is more appropriate to open it read-only. When userfaultfds are opened read-write like it is now, and such fd is passed from one process to another, SELinux will check both read and write permissions for the target process, even though it can't actually do any write operation on the fd later. Inspired by the following bug report, which has hit the SELinux scenario described above: https://bugzilla.redhat.com/show_bug.cgi?id=1974559 Reported-by: Robert O'Callahan <roc@ocallahan.org> Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization") Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- I marked this as RFC, because I'm not sure if this has any unwanted side effects. I only ran this patch through selinux-testsuite, which has a simple userfaultfd subtest, and a reproducer from the Bugzilla report. Please tell me whether this makes sense and/or if it passes any userfaultfd tests you guys might have. fs/userfaultfd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)