2022-07-08 09:37:43

by Ondrej Mosnacek

[permalink] [raw]
Subject: [RFC PATCH RESEND] userfaultfd: open userfaultfds with O_RDONLY

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 <[email protected]>
Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
Signed-off-by: Ondrej Mosnacek <[email protected]>
---

Resending as the last submission was ignored for over a year...

https://lore.kernel.org/lkml/[email protected]/T/

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 e943370107d0..8ccf00be63e1 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -989,7 +989,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;

@@ -2090,7 +2090,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.36.1


2022-07-12 14:59:16

by Peter Xu

[permalink] [raw]
Subject: Re: [RFC PATCH RESEND] userfaultfd: open userfaultfds with O_RDONLY

On Fri, Jul 08, 2022 at 11:34:51AM +0200, Ondrej Mosnacek 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 <[email protected]>
> Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
> Signed-off-by: Ondrej Mosnacek <[email protected]>

Acked-by: Peter Xu <[email protected]>

--
Peter Xu

2022-07-12 21:24:07

by Paul Moore

[permalink] [raw]
Subject: Re: [RFC PATCH RESEND] userfaultfd: open userfaultfds with O_RDONLY

On Tue, Jul 12, 2022 at 10:22 AM Peter Xu <[email protected]> wrote:
> On Fri, Jul 08, 2022 at 11:34:51AM +0200, Ondrej Mosnacek 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 <[email protected]>
> > Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
> > Signed-off-by: Ondrej Mosnacek <[email protected]>
>
> Acked-by: Peter Xu <[email protected]>

Thanks Peter.

--
paul-moore.com

2022-08-16 22:51:31

by Paul Moore

[permalink] [raw]
Subject: Re: [RFC PATCH RESEND] userfaultfd: open userfaultfds with O_RDONLY

On Fri, Jul 8, 2022 at 5:35 AM Ondrej Mosnacek <[email protected]> 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 <[email protected]>
> Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
> Signed-off-by: Ondrej Mosnacek <[email protected]>
> ---
>
> Resending as the last submission was ignored for over a year...
>
> https://lore.kernel.org/lkml/[email protected]/T/
>
> 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(-)

VFS folks, any objection to this patch? It seems reasonable to me and
I'd really prefer this to go in via the vfs tree, but I'm not above
merging this via the lsm/next tree to get someone in vfs land to pay
attention to this ...

> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index e943370107d0..8ccf00be63e1 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -989,7 +989,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;
>
> @@ -2090,7 +2090,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.36.1

--
paul-moore.com

2022-08-19 18:56:35

by Paul Moore

[permalink] [raw]
Subject: Re: [RFC PATCH RESEND] userfaultfd: open userfaultfds with O_RDONLY

On Tue, Aug 16, 2022 at 6:12 PM Paul Moore <[email protected]> wrote:
> On Fri, Jul 8, 2022 at 5:35 AM Ondrej Mosnacek <[email protected]> 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 <[email protected]>
> > Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
> > Signed-off-by: Ondrej Mosnacek <[email protected]>
> > ---
> >
> > Resending as the last submission was ignored for over a year...
> >
> > https://lore.kernel.org/lkml/[email protected]/T/
> >
> > 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(-)
>
> VFS folks, any objection to this patch? It seems reasonable to me and
> I'd really prefer this to go in via the vfs tree, but I'm not above
> merging this via the lsm/next tree to get someone in vfs land to pay
> attention to this ...

Okay, final warning, if I don't see any objections to this when I make
my patch sweep next week I'm going to go ahead and merge this via the
LSM tree.

> > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > index e943370107d0..8ccf00be63e1 100644
> > --- a/fs/userfaultfd.c
> > +++ b/fs/userfaultfd.c
> > @@ -989,7 +989,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;
> >
> > @@ -2090,7 +2090,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.36.1
>
> --
> paul-moore.com



--
paul-moore.com

2022-08-26 09:08:10

by Christian Brauner

[permalink] [raw]
Subject: Re: [RFC PATCH RESEND] userfaultfd: open userfaultfds with O_RDONLY

On Fri, Aug 19, 2022 at 02:50:57PM -0400, Paul Moore wrote:
> On Tue, Aug 16, 2022 at 6:12 PM Paul Moore <[email protected]> wrote:
> > On Fri, Jul 8, 2022 at 5:35 AM Ondrej Mosnacek <[email protected]> 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 <[email protected]>
> > > Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
> > > Signed-off-by: Ondrej Mosnacek <[email protected]>
> > > ---
> > >
> > > Resending as the last submission was ignored for over a year...
> > >
> > > https://lore.kernel.org/lkml/[email protected]/T/
> > >
> > > 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(-)
> >
> > VFS folks, any objection to this patch? It seems reasonable to me and
> > I'd really prefer this to go in via the vfs tree, but I'm not above
> > merging this via the lsm/next tree to get someone in vfs land to pay
> > attention to this ...
>
> Okay, final warning, if I don't see any objections to this when I make
> my patch sweep next week I'm going to go ahead and merge this via the
> LSM tree.

Makes sense,
Acked-by: Christian Brauner (Microsoft) <[email protected]>

2022-08-30 20:39:11

by Paul Moore

[permalink] [raw]
Subject: Re: [RFC PATCH RESEND] userfaultfd: open userfaultfds with O_RDONLY

On Fri, Aug 26, 2022 at 4:44 AM Christian Brauner <[email protected]> wrote:
> On Fri, Aug 19, 2022 at 02:50:57PM -0400, Paul Moore wrote:
> > On Tue, Aug 16, 2022 at 6:12 PM Paul Moore <[email protected]> wrote:
> > > On Fri, Jul 8, 2022 at 5:35 AM Ondrej Mosnacek <[email protected]> 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 <[email protected]>
> > > > Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
> > > > Signed-off-by: Ondrej Mosnacek <[email protected]>
> > > > ---
> > > >
> > > > Resending as the last submission was ignored for over a year...
> > > >
> > > > https://lore.kernel.org/lkml/[email protected]/T/
> > > >
> > > > 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(-)
> > >
> > > VFS folks, any objection to this patch? It seems reasonable to me and
> > > I'd really prefer this to go in via the vfs tree, but I'm not above
> > > merging this via the lsm/next tree to get someone in vfs land to pay
> > > attention to this ...
> >
> > Okay, final warning, if I don't see any objections to this when I make
> > my patch sweep next week I'm going to go ahead and merge this via the
> > LSM tree.
>
> Makes sense,
> Acked-by: Christian Brauner (Microsoft) <[email protected]>

Merged into lsm/next, thanks all.

--
paul-moore.com