Re: [PATCH 3/7] Add a UFFD_SECURE flag to the userfaultfd API.

On Wed, Oct 23, 2019 at 5:44 AM Mike Rapoport [off-list ref] wrote:

On Wed, Oct 23, 2019 at 10:29:20AM +0300, Cyrill Gorcunov wrote:

quoted

On Tue, Oct 22, 2019 at 09:11:04PM -0700, Andy Lutomirski wrote:

quoted

Trying again.  It looks like I used the wrong address for Pavel.

Thanks for CC Andy! I must confess I didn't dive into userfaultfd engine
personally but let me CC more people involved from criu side. (overquoting
left untouched for their sake).

Thanks for CC Cyrill!

quoted

quoted

On Sat, Oct 12, 2019 at 6:14 PM Andy Lutomirski [off-list ref] wrote:

quoted

[adding more people because this is going to be an ABI break, sigh]

On Sat, Oct 12, 2019 at 5:52 PM Daniel Colascione [off-list ref] wrote:

quoted

On Sat, Oct 12, 2019 at 4:10 PM Andy Lutomirski [off-list ref] wrote:

quoted

On Sat, Oct 12, 2019 at 12:16 PM Daniel Colascione [off-list ref] wrote:

quoted

The new secure flag makes userfaultfd use a new "secure" anonymous
file object instead of the default one, letting security modules
supervise userfaultfd use.

Requiring that users pass a new flag lets us avoid changing the
semantics for existing callers.

Is there any good reason not to make this be the default?


The only downside I can see is that it would increase the memory usage
of userfaultfd(), but that doesn't seem like such a big deal.  A
lighter-weight alternative would be to have a single inode shared by
all userfaultfd instances, which would require a somewhat different
internal anon_inode API.

I'd also prefer to just make SELinux use mandatory, but there's a
nasty interaction with UFFD_EVENT_FORK. Adding a new UFFD_SECURE mode
which blocks UFFD_EVENT_FORK sidesteps this problem. Maybe you know a
better way to deal with it.

Right now, when a process with a UFFD-managed VMA using
UFFD_EVENT_FORK forks, we make a new userfaultfd_ctx out of thin air
and enqueue it on the message queue for the parent process. When we
dequeue that context, we get to resolve_userfault_fork, which makes up
a new UFFD file object out of thin air in the context of the reading
process. Following normal SELinux rules, the SID attached to that new
file object would be the task SID of the process *reading* the fork
event, not the SID of the new fork child. That seems wrong, because
the label we give to the UFFD should correspond to the label of the
process that UFFD controls.

I must admit I have no idea about how SELinux works, but what's wrong with
making the new UFFD object to inherit the properties of the "original" one?

The new file object is created in the context of the same task that owns
the initial userfault file descriptor and it is used by the same task. So
if you have a process that registers some of its VMAs with userfaultfd
and enables UFFD_EVENT_FORK, the same process controls UFFD of itself and
its children.

I'm not actually convinced this is a problem.

What *is* a problem is touching the file descriptor table at all from
read(2).  That's a big no-no.

--Andy