Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
descriptor table from the read() implementation of uffd, which may have
security implications for unprivileged use of the userfaultfd.
Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
CAP_SYS_PTRACE.
Signed-off-by: Mike Rapoport <[email protected]>
---
fs/userfaultfd.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index f9fd18670e22..d99d166fd892 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1834,13 +1834,12 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
if (copy_from_user(&uffdio_api, buf, sizeof(uffdio_api)))
goto out;
features = uffdio_api.features;
- if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES)) {
- memset(&uffdio_api, 0, sizeof(uffdio_api));
- if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
- goto out;
- ret = -EINVAL;
- goto out;
- }
+ ret = -EINVAL;
+ if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES))
+ goto err_out;
+ ret = -EPERM;
+ if ((features & UFFD_FEATURE_EVENT_FORK) && !capable(CAP_SYS_PTRACE))
+ goto err_out;
/* report all available features and ioctls to userland */
uffdio_api.features = UFFD_API_FEATURES;
uffdio_api.ioctls = UFFD_API_IOCTLS;
@@ -1853,6 +1852,11 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
ret = 0;
out:
return ret;
+err_out:
+ memset(&uffdio_api, 0, sizeof(uffdio_api));
+ if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
+ ret = -EFAULT;
+ goto out;
}
static long userfaultfd_ioctl(struct file *file, unsigned cmd,
--
2.7.4
Hello Mike,
On Tue, Nov 05, 2019 at 05:29:37PM +0200, Mike Rapoport wrote:
> Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
> descriptor table from the read() implementation of uffd, which may have
> security implications for unprivileged use of the userfaultfd.
>
> Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
> CAP_SYS_PTRACE.
>
> Signed-off-by: Mike Rapoport <[email protected]>
> ---
> fs/userfaultfd.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index f9fd18670e22..d99d166fd892 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1834,13 +1834,12 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
> if (copy_from_user(&uffdio_api, buf, sizeof(uffdio_api)))
> goto out;
> features = uffdio_api.features;
> - if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES)) {
> - memset(&uffdio_api, 0, sizeof(uffdio_api));
> - if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
> - goto out;
> - ret = -EINVAL;
> - goto out;
> - }
> + ret = -EINVAL;
> + if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES))
> + goto err_out;
> + ret = -EPERM;
> + if ((features & UFFD_FEATURE_EVENT_FORK) && !capable(CAP_SYS_PTRACE))
> + goto err_out;
> /* report all available features and ioctls to userland */
> uffdio_api.features = UFFD_API_FEATURES;
> uffdio_api.ioctls = UFFD_API_IOCTLS;
> @@ -1853,6 +1852,11 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
> ret = 0;
> out:
> return ret;
> +err_out:
> + memset(&uffdio_api, 0, sizeof(uffdio_api));
> + if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
> + ret = -EFAULT;
> + goto out;
> }
>
> static long userfaultfd_ioctl(struct file *file, unsigned cmd,
Reviewed-by: Andrea Arcangeli <[email protected]>
Thanks,
Andrea
On Tue, Nov 5, 2019 at 7:29 AM Mike Rapoport <[email protected]> wrote:
>
> Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
> descriptor table from the read() implementation of uffd, which may have
> security implications for unprivileged use of the userfaultfd.
>
> Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
> CAP_SYS_PTRACE.
Thanks. But shouldn't we be doing the capability check at
userfaultfd(2) time (when we do the other permission checks), not
later, in the API ioctl?
On 2019-11-05, Mike Rapoport <[email protected]> wrote:
> Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
> descriptor table from the read() implementation of uffd, which may have
> security implications for unprivileged use of the userfaultfd.
>
> Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
> CAP_SYS_PTRACE.
>
> Signed-off-by: Mike Rapoport <[email protected]>
> ---
> fs/userfaultfd.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index f9fd18670e22..d99d166fd892 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1834,13 +1834,12 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
> if (copy_from_user(&uffdio_api, buf, sizeof(uffdio_api)))
> goto out;
> features = uffdio_api.features;
> - if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES)) {
> - memset(&uffdio_api, 0, sizeof(uffdio_api));
> - if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
> - goto out;
> - ret = -EINVAL;
> - goto out;
> - }
> + ret = -EINVAL;
> + if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES))
> + goto err_out;
> + ret = -EPERM;
> + if ((features & UFFD_FEATURE_EVENT_FORK) && !capable(CAP_SYS_PTRACE))
> + goto err_out;
> /* report all available features and ioctls to userland */
> uffdio_api.features = UFFD_API_FEATURES;
> uffdio_api.ioctls = UFFD_API_IOCTLS;
> @@ -1853,6 +1852,11 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
> ret = 0;
> out:
> return ret;
> +err_out:
> + memset(&uffdio_api, 0, sizeof(uffdio_api));
> + if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
Wouldn't it be simpler to do clear_user()?
> + ret = -EFAULT;
> + goto out;
> }
>
> static long userfaultfd_ioctl(struct file *file, unsigned cmd,
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
On Tue, Nov 5, 2019 at 7:55 AM Daniel Colascione <[email protected]> wrote:
>
> On Tue, Nov 5, 2019 at 7:29 AM Mike Rapoport <[email protected]> wrote:
> >
> > Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
> > descriptor table from the read() implementation of uffd, which may have
> > security implications for unprivileged use of the userfaultfd.
> >
> > Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
> > CAP_SYS_PTRACE.
>
> Thanks. But shouldn't we be doing the capability check at
> userfaultfd(2) time (when we do the other permission checks), not
> later, in the API ioctl?
The ioctl seems reasonable to me. In particular, if there is anyone
who creates a userfaultfd as root and then drop permissions, a later
ioctl could unexpectedly enable FORK.
This assumes that the code in question is only reachable through
ioctl() and not write().
On Tue, Nov 5, 2019 at 8:00 AM Andy Lutomirski <[email protected]> wrote:
>
> On Tue, Nov 5, 2019 at 7:55 AM Daniel Colascione <[email protected]> wrote:
> >
> > On Tue, Nov 5, 2019 at 7:29 AM Mike Rapoport <[email protected]> wrote:
> > >
> > > Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
> > > descriptor table from the read() implementation of uffd, which may have
> > > security implications for unprivileged use of the userfaultfd.
> > >
> > > Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
> > > CAP_SYS_PTRACE.
> >
> > Thanks. But shouldn't we be doing the capability check at
> > userfaultfd(2) time (when we do the other permission checks), not
> > later, in the API ioctl?
>
> The ioctl seems reasonable to me. In particular, if there is anyone
> who creates a userfaultfd as root and then drop permissions, a later
> ioctl could unexpectedly enable FORK.
Sure, but the same argument applies to all the other permission checks
that we do at open time, not at ioctl time. For better or for worse,
the DAC-ish model used in most places is that access checks happen at
file object creation time and anyone who has the FD can perform those
operations later. Confusing the model by doing *some* permission
checks at open time and *some* permission checks at usage time makes
the system harder to understand.
On Tue, Nov 05, 2019 at 08:00:26AM -0800, Andy Lutomirski wrote:
> On Tue, Nov 5, 2019 at 7:55 AM Daniel Colascione <[email protected]> wrote:
> >
> > On Tue, Nov 5, 2019 at 7:29 AM Mike Rapoport <[email protected]> wrote:
> > >
> > > Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
> > > descriptor table from the read() implementation of uffd, which may have
> > > security implications for unprivileged use of the userfaultfd.
> > >
> > > Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
> > > CAP_SYS_PTRACE.
> >
> > Thanks. But shouldn't we be doing the capability check at
> > userfaultfd(2) time (when we do the other permission checks), not
> > later, in the API ioctl?
>
> The ioctl seems reasonable to me. In particular, if there is anyone
> who creates a userfaultfd as root and then drop permissions, a later
> ioctl could unexpectedly enable FORK.
>
> This assumes that the code in question is only reachable through
> ioctl() and not write().
write isn't implemented. Until UFFDIO_API runs, all other implemented
syscalls are disabled (i.e. all other ioctls, poll and read). You can
quickly verify all the 3 blocks by searching for UFFD_STATE_WAIT_API,
UFFDIO_API is the place where the handshake with userland
happens. userland asks for certain features and the kernel
implementation of userlands answers yes or no.
Normally we would only ever return -EINVAL on a request of a feature
that isn't available in the running kernel (equivalent to -ENOSYS if
the syscall is entirely missing on an even older kernel), -EPERM is
more informative as it tells userland the feature is actually in the
kernel just it requires more permissions.
We could have returned -EINVAL too, but it wouldn't have made a
difference to non-privileged CRIU and we're not aware of other users
that could benefit from -EINVAL instead of -EPERM. This the relevant
CRIU userland:
if (ioctl(uffd, UFFDIO_API, &uffdio_api)) {
pr_perror("Failed to get uffd API");
goto err;
}
Unfortunately this is an ABI break, preferred than the clean removal
of the feature, because it's at least not going to break CRIU
deployments running with the PTRACE privilege. The clean removal while
non-ABI breaking, would have prevented all CRIU users to keep running
after a kernel upgrade.
The long term plan is to introduce UFFD_FEATURE_EVENT_FORK2 feature
flag that uses the ioctl to receive the child uffd, it'll consume more
CPU, but it wouldn't require the PTRACE privilege anymore.
Overall any suid or SCM_RIGHTS fd-receiving app, that isn't checking
the retval of open/socket or whatever fd "installing" syscall, is non
robust and is prone to break over time as more people edit the code or
as any library call internally change behavior, so if there's any
practical issue caused by this, it should be fixed in userland
too for higher robustness.
If you stick your userland to std::fs and std::net robustness against
issues like this is enforced by the language.
Thanks,
Andrea
On Tue, Nov 05, 2019 at 08:06:49AM -0800, Daniel Colascione wrote:
> Sure, but the same argument applies to all the other permission checks
> that we do at open time, not at ioctl time. For better or for worse,
> the DAC-ish model used in most places is that access checks happen at
> file object creation time and anyone who has the FD can perform those
> operations later. Confusing the model by doing *some* permission
> checks at open time and *some* permission checks at usage time makes
> the system harder to understand.
The only case that requires change is if userland requested the
UFFD_FEATURE_EVENT_FORK feature (which AFIK only CRIU does) and that
request is done in the UFFDIO_API call not during the syscall.
Doing the check in the syscall would then break all non privileged
users like if we'd set /proc/sys/vm/unprivileged_userfaultfd to
zero. Qemu for example rightfully never runs with privilege (with a
few exceptions like Kata which should be fixed in fact) and it never
asks for the UFFD_FEATURE_EVENT_FORK feature either.
On Tue, Nov 5, 2019 at 8:33 AM Andrea Arcangeli <[email protected]> wrote:
>
> On Tue, Nov 05, 2019 at 08:06:49AM -0800, Daniel Colascione wrote:
> > Sure, but the same argument applies to all the other permission checks
> > that we do at open time, not at ioctl time. For better or for worse,
> > the DAC-ish model used in most places is that access checks happen at
> > file object creation time and anyone who has the FD can perform those
> > operations later. Confusing the model by doing *some* permission
> > checks at open time and *some* permission checks at usage time makes
> > the system harder to understand.
>
> The only case that requires change is if userland requested the
> UFFD_FEATURE_EVENT_FORK feature (which AFIK only CRIU does) and that
> request is done in the UFFDIO_API call not during the syscall.
>
> Doing the check in the syscall would then break all non privileged
> users like if we'd set /proc/sys/vm/unprivileged_userfaultfd to
> zero.
I'm not suggesting that we fail userfaultfd(2) without CAP_SYS_PTRACE.
That would, as you point out, break things. I'm talking about
recording *whether* we had CAP_SYS_PTRACE in an internal flag in the
uffd context when we create the thing --- and then, at ioctl time,
checking that flag, not the caller's CAP_SYS_PTRACE, to see whether
UFFD_FEATURE_EVENT_FORK should be made available. This way, the
security check hinges on whether the caller *at create time* was
privileged.
On Tue, Nov 5, 2019 at 8:24 AM Andrea Arcangeli <[email protected]> wrote:
> The long term plan is to introduce UFFD_FEATURE_EVENT_FORK2 feature
> flag that uses the ioctl to receive the child uffd, it'll consume more
> CPU, but it wouldn't require the PTRACE privilege anymore.
Why not just have callers retrieve FDs using recvmsg? This way, you
retrieve the message packet and the file descriptor at the same time
and you don't need any appreciable extra CPU use.
On Tue, Nov 05, 2019 at 08:39:26AM -0800, Daniel Colascione wrote:
> I'm not suggesting that we fail userfaultfd(2) without CAP_SYS_PTRACE.
> That would, as you point out, break things. I'm talking about
> recording *whether* we had CAP_SYS_PTRACE in an internal flag in the
> uffd context when we create the thing --- and then, at ioctl time,
> checking that flag, not the caller's CAP_SYS_PTRACE, to see whether
> UFFD_FEATURE_EVENT_FORK should be made available. This way, the
> security check hinges on whether the caller *at create time* was
> privileged.
Until now it wasn't clear to me you still wanted to do the permission
check in UFFDIO_API time, and you only intended to move the
"measurement" of the capability to the syscall.
So you're suggesting to add more kernel complexity to code pending for
removal to achieve a theoretically more pure solution in the band-aid
required to defer the removal of the posix-breaking read
implementation of the uffd fork feature?
On Tue, Nov 5, 2019 at 8:56 AM Andrea Arcangeli <[email protected]> wrote:
>
> On Tue, Nov 05, 2019 at 08:39:26AM -0800, Daniel Colascione wrote:
> > I'm not suggesting that we fail userfaultfd(2) without CAP_SYS_PTRACE.
> > That would, as you point out, break things. I'm talking about
> > recording *whether* we had CAP_SYS_PTRACE in an internal flag in the
> > uffd context when we create the thing --- and then, at ioctl time,
> > checking that flag, not the caller's CAP_SYS_PTRACE, to see whether
> > UFFD_FEATURE_EVENT_FORK should be made available. This way, the
> > security check hinges on whether the caller *at create time* was
> > privileged.
>
> Until now it wasn't clear to me you still wanted to do the permission
> check in UFFDIO_API time, and you only intended to move the
> "measurement" of the capability to the syscall.
>
> So you're suggesting to add more kernel complexity to code pending for
> removal to achieve a theoretically more pure solution in the band-aid
> required to defer the removal of the posix-breaking read
> implementation of the uffd fork feature?
And you're suggesting making a security check work weirdly unlike most
other security checks because you hope it'll get removed one day?
Temporary solutions aren't, and if something goes into the kernel at
all, it's worth getting right. The general rule is that access checks
happen at open time. The kernel has already been bitten by UFFD
exempting itself from the normal rules (e.g., the
read(2)-makes-a-file-descriptor thing) in the name of expediency.
There shouldn't be any more exceptions.
On Tue, Nov 05, 2019 at 09:02:09AM -0800, Daniel Colascione wrote:
> And you're suggesting making a security check work weirdly unlike most
> other security checks because you hope it'll get removed one day?
I didn't actually suggest that, I was only asking clarifications that
I understood correctly because up until that point you didn't seem to
say that the "permission check" needs to remain in UFFDIO_API.
> Temporary solutions aren't, and if something goes into the kernel at
> all, it's worth getting right. The general rule is that access checks
> happen at open time. The kernel has already been bitten by UFFD
> exempting itself from the normal rules (e.g., the
> read(2)-makes-a-file-descriptor thing) in the name of expediency.
> There shouldn't be any more exceptions.
It didn't occur to me that not doing the measurement in the syscall
that opens an fd is weird.
The posted patch doesn't work any different than fscrypt_ioctl_add_key
in FS_IOC_ADD_ENCRYPTION_KEY of ext4 and others, or
btrfs_ioctl_fssetxattr or a ton of other examples where permissions
are checked directly the in ioctl of the files and the measurement is
also done in the ioctl and not in the open() as you suggest as the
only non-weird solution that should exist in the kernel.
I can surely provide a lot more examples of the exact same paradigm
where the measurement of the capability is done in the ioctl, those
are the first two examples that show up so it's unlikely they're the
only ones.
So overall I didn't think this was something wrong to do, or weird or
something particularly new and I didn't look like we were bitting
anything with it. And more than in the name of expediency this simply
looks preferable to keep the complexity of the kernel low which in
turns it means it's going to be more secure and simpler to
maintain. Especially considering this code is likely to be modified
later.
Said that I've nothing contrary to do the more complex solution if
that's the correct thing to do despite more complex and despite the
code is pending for removal anyway, just I don't see any difference
between the current simple patch to what ext4_ioctl does in
FS_IOC_ADD_ENCRYPTION_KEY + FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR.
> On Nov 5, 2019, at 9:02 AM, Daniel Colascione <[email protected]> wrote:
>
> On Tue, Nov 5, 2019 at 8:56 AM Andrea Arcangeli <[email protected]> wrote:
>>
>>> On Tue, Nov 05, 2019 at 08:39:26AM -0800, Daniel Colascione wrote:
>>> I'm not suggesting that we fail userfaultfd(2) without CAP_SYS_PTRACE.
>>> That would, as you point out, break things. I'm talking about
>>> recording *whether* we had CAP_SYS_PTRACE in an internal flag in the
>>> uffd context when we create the thing --- and then, at ioctl time,
>>> checking that flag, not the caller's CAP_SYS_PTRACE, to see whether
>>> UFFD_FEATURE_EVENT_FORK should be made available. This way, the
>>> security check hinges on whether the caller *at create time* was
>>> privileged.
>>
>> Until now it wasn't clear to me you still wanted to do the permission
>> check in UFFDIO_API time, and you only intended to move the
>> "measurement" of the capability to the syscall.
>>
>> So you're suggesting to add more kernel complexity to code pending for
>> removal to achieve a theoretically more pure solution in the band-aid
>> required to defer the removal of the posix-breaking read
>> implementation of the uffd fork feature?
>
> And you're suggesting making a security check work weirdly unlike most
> other security checks because you hope it'll get removed one day?
> Temporary solutions aren't, and if something goes into the kernel at
> all, it's worth getting right. The general rule is that access checks
> happen at open time. The kernel has already been bitten by UFFD
> exempting itself from the normal rules (e.g., the
> read(2)-makes-a-file-descriptor thing) in the name of expediency.
> There shouldn't be any more exceptions.
I don’t think ioctl() checking permission is particularly unusual. In principle, it’s better than open for a retrofit — open didn’t capture this permission in the past, so adding it makes an existing capability stronger than it was, which isn’t fantastic.
On Tue, Nov 5, 2019 at 2:01 PM Andy Lutomirski <[email protected]> wrote:
> > On Nov 5, 2019, at 9:02 AM, Daniel Colascione <[email protected]> wrote:
> >
> > On Tue, Nov 5, 2019 at 8:56 AM Andrea Arcangeli <[email protected]> wrote:
> >>
> >>> On Tue, Nov 05, 2019 at 08:39:26AM -0800, Daniel Colascione wrote:
> >>> I'm not suggesting that we fail userfaultfd(2) without CAP_SYS_PTRACE.
> >>> That would, as you point out, break things. I'm talking about
> >>> recording *whether* we had CAP_SYS_PTRACE in an internal flag in the
> >>> uffd context when we create the thing --- and then, at ioctl time,
> >>> checking that flag, not the caller's CAP_SYS_PTRACE, to see whether
> >>> UFFD_FEATURE_EVENT_FORK should be made available. This way, the
> >>> security check hinges on whether the caller *at create time* was
> >>> privileged.
> >>
> >> Until now it wasn't clear to me you still wanted to do the permission
> >> check in UFFDIO_API time, and you only intended to move the
> >> "measurement" of the capability to the syscall.
> >>
> >> So you're suggesting to add more kernel complexity to code pending for
> >> removal to achieve a theoretically more pure solution in the band-aid
> >> required to defer the removal of the posix-breaking read
> >> implementation of the uffd fork feature?
> >
> > And you're suggesting making a security check work weirdly unlike most
> > other security checks because you hope it'll get removed one day?
> > Temporary solutions aren't, and if something goes into the kernel at
> > all, it's worth getting right. The general rule is that access checks
> > happen at open time. The kernel has already been bitten by UFFD
> > exempting itself from the normal rules (e.g., the
> > read(2)-makes-a-file-descriptor thing) in the name of expediency.
> > There shouldn't be any more exceptions.
>
> I don’t think ioctl() checking permission is particularly unusual. In principle, it’s better than open for a retrofit — open didn’t capture this permission in the past, so adding it makes an existing capability stronger than it was, which isn’t fantastic.
All right, let's do it the way the OP's patch does it then.
Hi Daniel,
On Tue, Nov 05, 2019 at 08:41:18AM -0800, Daniel Colascione wrote:
> On Tue, Nov 5, 2019 at 8:24 AM Andrea Arcangeli <[email protected]> wrote:
> > The long term plan is to introduce UFFD_FEATURE_EVENT_FORK2 feature
> > flag that uses the ioctl to receive the child uffd, it'll consume more
> > CPU, but it wouldn't require the PTRACE privilege anymore.
>
> Why not just have callers retrieve FDs using recvmsg? This way, you
> retrieve the message packet and the file descriptor at the same time
> and you don't need any appreciable extra CPU use.
I don't follow you here. Can you elaborate on how recvmsg would be used in
this case?
--
Sincerely yours,
Mike.
On Thu, Nov 7, 2019 at 12:39 AM Mike Rapoport <[email protected]> wrote:
> On Tue, Nov 05, 2019 at 08:41:18AM -0800, Daniel Colascione wrote:
> > On Tue, Nov 5, 2019 at 8:24 AM Andrea Arcangeli <[email protected]> wrote:
> > > The long term plan is to introduce UFFD_FEATURE_EVENT_FORK2 feature
> > > flag that uses the ioctl to receive the child uffd, it'll consume more
> > > CPU, but it wouldn't require the PTRACE privilege anymore.
> >
> > Why not just have callers retrieve FDs using recvmsg? This way, you
> > retrieve the message packet and the file descriptor at the same time
> > and you don't need any appreciable extra CPU use.
>
> I don't follow you here. Can you elaborate on how recvmsg would be used in
> this case?
Imagine an AF_UNIX SOCK_DGRAM socket. You call recvmsg(). You get a
blob of regular data along with some ancillary data. The ancillary
data may include some file descriptors or it may not. Isn't the UFFD
message model the same thing? You'd call recvmsg() on a UFFD and get
back a uffd_msg data structure. If that uffd_msg came with file
descriptors, these descriptors would be in ancillary data. If you
didn't reserve enough space for the message or enough space for its
ancillary data, the recvmsg() call would fail cleanly with MSG_TRUNC
or MSG_CTRUNC.
The nice thing about using recvmsg() for this purpose is that there's
tons of existing code for dealing with recvmsg()'s calling convention
and its ancillary data. You can, for example, use recvmsg out of the
box in a Python script. You could make an ioctl that also returned a
data blob plus some optional file descriptors, but if recvmsg already
does exactly that job and it's well-understood, why not just reuse the
recvmsg interface?
How practical is it to actually support recvmsg without being a
socket? How hard would it be to just become a socket? I don't know. My
point is only that *from a userspace API* point of view, recvmsg()
seems ideal.
Hello,
On Thu, Nov 07, 2019 at 12:54:59AM -0800, Daniel Colascione wrote:
> On Thu, Nov 7, 2019 at 12:39 AM Mike Rapoport <[email protected]> wrote:
> > On Tue, Nov 05, 2019 at 08:41:18AM -0800, Daniel Colascione wrote:
> > > On Tue, Nov 5, 2019 at 8:24 AM Andrea Arcangeli <[email protected]> wrote:
> > > > The long term plan is to introduce UFFD_FEATURE_EVENT_FORK2 feature
> > > > flag that uses the ioctl to receive the child uffd, it'll consume more
> > > > CPU, but it wouldn't require the PTRACE privilege anymore.
> > >
> > > Why not just have callers retrieve FDs using recvmsg? This way, you
> > > retrieve the message packet and the file descriptor at the same time
> > > and you don't need any appreciable extra CPU use.
> >
> > I don't follow you here. Can you elaborate on how recvmsg would be used in
> > this case?
>
> Imagine an AF_UNIX SOCK_DGRAM socket. You call recvmsg(). You get a
> blob of regular data along with some ancillary data. The ancillary
> data may include some file descriptors or it may not. Isn't the UFFD
> message model the same thing? You'd call recvmsg() on a UFFD and get
> back a uffd_msg data structure. If that uffd_msg came with file
> descriptors, these descriptors would be in ancillary data. If you
> didn't reserve enough space for the message or enough space for its
> ancillary data, the recvmsg() call would fail cleanly with MSG_TRUNC
> or MSG_CTRUNC.
Having to check for truncation is just a slowdown doesn't sound a
feature here but just a complication and unnecessary branches. You can
already read as much as you want in multiples of the uffd size.
> The nice thing about using recvmsg() for this purpose is that there's
> tons of existing code for dealing with recvmsg()'s calling convention
> and its ancillary data. You can, for example, use recvmsg out of the
> box in a Python script. You could make an ioctl that also returned a
> data blob plus some optional file descriptors, but if recvmsg already
> does exactly that job and it's well-understood, why not just reuse the
> recvmsg interface?
uffd can't become an plain AF_UNIX because on the other end there's no
other process but the kernel. Even if it could the fact it'd
facilitate a pure python backend isn't relevant because handling page
faults is a performance critical system activity, and rust can do the
ioctl like it can do poll/epoll without mio/tokyo by just calling
glibc. We can't write kernel code in python either for the same
reason.
> How practical is it to actually support recvmsg without being a
> socket? How hard would it be to just become a socket? I don't know. My
AF_UINIX has more features than we need (credentials) and dealing with
skbs and truncation would slow down the protocol. The objective is to
get the highest performance possible out of the uffd API so that it
performs as close as possible to running page faults in the kernel.
So even if we could avoid a syscall in CRIU, but we'd be slowing down
QEMU and all other normal cooperative usages if we made uffd a
socket. So overall it would be a net loss.
> point is only that *from a userspace API* point of view, recvmsg()
> seems ideal.
Now thinking about this, the semantics of the ancillary data seems to
be per socket family. So what does prevent you to create an AF_UNIX
socket, send it to a SCM_RIGHTS receiving daemon unaware that it is
getting an AF_UNIX socket. The daemon is calling recvmsg on the fd it
receives from SCM_RIGHTS in order to receive ancillary data from
another non-AF_UNIX family instead (it is irrelevant what the
semantics of the ancillary data are but they're not AF_UNIX). So the
daemon calls recvmsg and it will not understand that the fd in the
ancillary data represents an installed "fd" in the fd space and in
turn still gets the fd involuntary installed with the exact same side
effects of what we're fixing in the uffd fork event read?
I guess there shall be something somewhere that prevents recvmsg to
run on anything but an AF_UNIX if msg_control isn't NULL and
msg_controllen > 0? Otherwise even if we implemented the uffd fork
event with recvmsg, we would be back to square one.
As a corollary this could also imply we don't need the ptrace check
after all if the same thing can happen already to SCM_RIGHTS receiving
daemon expecting to receive ancillary data from AF_SOMETHING but
getting an AF_UNIX instead through SCM_RIGHTS (just like the uffd
example was expecting to call read() on a normal fd and instead it got
an uffd).
I'm sure there's something stopping SCM_RIGHTS to have the same
pitfalls of uffd event fork and that makes recvmsg safe unlike read()
but then it's not immediately clear what it is.
Thanks,
Andrea
On Thu, Nov 7, 2019 at 7:38 AM Andrea Arcangeli <[email protected]> wrote:
> On Thu, Nov 07, 2019 at 12:54:59AM -0800, Daniel Colascione wrote:
> > On Thu, Nov 7, 2019 at 12:39 AM Mike Rapoport <[email protected]> wrote:
> > > On Tue, Nov 05, 2019 at 08:41:18AM -0800, Daniel Colascione wrote:
> > > > On Tue, Nov 5, 2019 at 8:24 AM Andrea Arcangeli <[email protected]> wrote:
> > > > > The long term plan is to introduce UFFD_FEATURE_EVENT_FORK2 feature
> > > > > flag that uses the ioctl to receive the child uffd, it'll consume more
> > > > > CPU, but it wouldn't require the PTRACE privilege anymore.
> > > >
> > > > Why not just have callers retrieve FDs using recvmsg? This way, you
> > > > retrieve the message packet and the file descriptor at the same time
> > > > and you don't need any appreciable extra CPU use.
> > >
> > > I don't follow you here. Can you elaborate on how recvmsg would be used in
> > > this case?
> >
> > Imagine an AF_UNIX SOCK_DGRAM socket. You call recvmsg(). You get a
> > blob of regular data along with some ancillary data. The ancillary
> > data may include some file descriptors or it may not. Isn't the UFFD
> > message model the same thing? You'd call recvmsg() on a UFFD and get
> > back a uffd_msg data structure. If that uffd_msg came with file
> > descriptors, these descriptors would be in ancillary data. If you
> > didn't reserve enough space for the message or enough space for its
> > ancillary data, the recvmsg() call would fail cleanly with MSG_TRUNC
> > or MSG_CTRUNC.
>
> Having to check for truncation is just a slowdown doesn't sound a
> feature here but just a complication and unnecessary branches. You can
> already read as much as you want in multiples of the uffd size.
You're already paying for bounds checking. Receiving a message via a
datagram socket is basically the same thing as what UFFD's read is
doing anyway.
> > The nice thing about using recvmsg() for this purpose is that there's
> > tons of existing code for dealing with recvmsg()'s calling convention
> > and its ancillary data. You can, for example, use recvmsg out of the
> > box in a Python script. You could make an ioctl that also returned a
> > data blob plus some optional file descriptors, but if recvmsg already
> > does exactly that job and it's well-understood, why not just reuse the
> > recvmsg interface?
>
> uffd can't become an plain AF_UNIX because on the other end there's no
> other process but the kernel. Even if it could the fact it'd
> facilitate a pure python backend isn't relevant because handling page
> faults is a performance critical system activity, and rust can do the
> ioctl like it can do poll/epoll without mio/tokyo by just calling
> glibc. We can't write kernel code in python either for the same
> reason.
My point isn't "hey, you should write this in Python". (Although for
prototyping, why not?) My point is that where there's an existing
kernel interface for exactly the functionality you want, you should
use it instead of inventing some new thing, because when we use the
same interface for things have the same shape and purpose, we not only
get to reuse code, but also the knowledge in people's heads.
> > point is only that *from a userspace API* point of view, recvmsg()
> > seems ideal.
>
> Now thinking about this, the semantics of the ancillary data seems to
> be per socket family. So what does prevent you to create an AF_UNIX
> socket, send it to a SCM_RIGHTS receiving daemon unaware that it is
> getting an AF_UNIX socket. The daemon is calling recvmsg on the fd it
> receives from SCM_RIGHTS in order to receive ancillary data from
> another non-AF_UNIX family instead (it is irrelevant what the
> semantics of the ancillary data are but they're not AF_UNIX). So the
> daemon calls recvmsg and it will not understand that the fd in the
> ancillary data represents an installed "fd" in the fd space and in
> turn still gets the fd involuntary installed with the exact same side
> effects of what we're fixing in the uffd fork event read?
SCM_RIGHTS (AFAIK) is the only bit of ancillary data which indicates
that the kernel has created a file descriptor in the process doing the
recvmsg.
> I guess there shall be something somewhere that prevents recvmsg to
> run on anything but an AF_UNIX if msg_control isn't NULL and
> msg_controllen > 0? Otherwise even if we implemented the uffd fork
> event with recvmsg, we would be back to square one.
Why would we limit recvmsg to AF_UNIX? We can receive ancillary data
on other sockets, e.g., netlink. SCM_RIGHTS works only with AF_UNIX
right now, but this limitation isn't written in stone.
> As a corollary this could also imply we don't need the ptrace check
> after all if the same thing can happen already to SCM_RIGHTS receiving
> daemon expecting to receive ancillary data from AF_SOMETHING but
> getting an AF_UNIX instead through SCM_RIGHTS (just like the uffd
> example was expecting to call read() on a normal fd and instead it got
> an uffd).
Programs generally don't go calling recvmsg() on random FDs they get
from the outside world. They do call read() on those FDs, which is why
read() having unexpected side effects is terrible.
> I'm sure there's something stopping SCM_RIGHTS to have the same
> pitfalls of uffd event fork and that makes recvmsg safe unlike read()
> but then it's not immediately clear what it is.
If you call it with a non-empty ancillary data buffer, you know to
react to what you get. You're *opting into* the possibility of getting
file descriptors. Sure, it's theoretically possible that a program
calls recvmsg on random FDs it gets from unknown sources, sees
SCM_RIGHTS unexpectedly, and just the SCM_RIGHTS message and its FD
payload, but that's an outright bug, while calling read() on stdin is
no bug.
Anyway, IMHO, UFFD should be a netlink-like SOCK_DGRAM socket that
sends FDs with SCM_RIGHTS. This interface is already very efficient --
people have been optimizing the hell out of AF_UNIX for decades ---
and this interface provides exactly the right interface semantics for
what UFFD needs to do.
On Thu, Nov 07, 2019 at 08:15:53AM -0800, Daniel Colascione wrote:
> You're already paying for bounds checking. Receiving a message via a
> datagram socket is basically the same thing as what UFFD's read is
> doing anyway.
Except it's synchronous and there are no dynamic allocations required
in uffd, while af_netlink and af_unix both all deal with queue of
events in skbs dynamically allocated.
Ultimately if we strip away the skbs for performance reasons, there
wouldn't be much code to share, so if the only benefit would be to
call recvmsg which would still be as insecure as read for the "worse"
case than suid, so I don't see the point.
And should then eventfd also become a netlink then? I mean uffd was
supposed to work like eventfd except it requires specialized events.
> Programs generally don't go calling recvmsg() on random FDs they get
> from the outside world. They do call read() on those FDs, which is why
That programs generally don't do something only means the attack is
less probable.
Programs generally aren't suid. Programs generally don't use
SCM_RIGHTS. Programs generally don't ignore the retval of
open/socket/uffd syscalls. Programs generally don't make assumptions
on the fd ID after one of those syscalls that install fds.
If all programs generally do the right thing (where the most important
is to not make assumptions on the fd IDs and to check all syscall
retvals), there was never an issue to begin with even in uffd API.
> read() having unexpected side effects is terrible.
If having unexpected side effects in read is "terrible" (i.e. I
personally prefer to use terms like terrible when there's at least
something that can be exploited in practice, not for theoretical
issues) for an SCM_RIGHTS receiving daemon, I just don't see how the
exact same unexpected (still theoretical) side effects in recvmsg with
an unexpected nested cmsg->cmsg_type == SCM_RIGHTS message being
returned, isn't terrible too.
> If you call it with a non-empty ancillary data buffer, you know to
> react to what you get. You're *opting into* the possibility of getting
> file descriptors. Sure, it's theoretically possible that a program
> calls recvmsg on random FDs it gets from unknown sources, sees
> SCM_RIGHTS unexpectedly, and just the SCM_RIGHTS message and its FD
> payload, but that's an outright bug, while calling read() on stdin is
> no bug.
I'm not talking about stdin and suid. recvmsg might mitigate the
concern for suid (not certain, depends on the suid, if it's generally
doing what you expect most suid to be doing or not), I was talking
about the SCM_RIGHTS receiving daemon instead, the "worse" more
concerning case than the suid.
I quote below Andy's relevant email:
======
It's worse if SCM_RIGHTS is involved.
======
Not all software will do this after calling recvmsg:
if (cmsg->cmsg_type == SCM_RIGHTS) {
/* oops we got attacked and an fd was involountarily installed
because we received another AF_UNIX from a malicious attacker
in control of the other end of the SCM_RIGHTS-receiving
AF_UNIX connection instead of our expected socket family
which doesn't even support SCM_RIGHTS so we would never have
noticed an fd was installed after recvmsg */
}
On Thu, Nov 7, 2019 at 10:23 AM Andrea Arcangeli <[email protected]> wrote:
>
> On Thu, Nov 07, 2019 at 08:15:53AM -0800, Daniel Colascione wrote:
> > You're already paying for bounds checking. Receiving a message via a
> > datagram socket is basically the same thing as what UFFD's read is
> > doing anyway.
>
> Except it's synchronous and there are no dynamic allocations required
> in uffd, while af_netlink and af_unix both all deal with queue of
> events in skbs dynamically allocated.
Do you have any evidence that skb allocation is a significant cost
compared to a page fault and schedule? Regardless: if you don't want
to use skbs, don't. My point is that recvmsg is the ideal interface
for UFFD and i'm agnostic on the implementation of this interface.
> And should then eventfd also become a netlink then? I mean uffd was
> supposed to work like eventfd except it requires specialized events.
You've raised eventfd as a model for UFFD on several occasions. I
don't think eventfd is a good reference point. An eventfd is a single
object with 64 bits of state. It can notify interested parties in
changes to that state. Eventfd does not provide a queue. UFFD,
however, *is* a queue. It provides an arbitrary number of state change
notifications to a reader. In this way, UFFD is much more like a
socket than it's like eventfd. That is, eventfd is about level-change
notifications, but UFFD is about sending messages.
> > Programs generally don't go calling recvmsg() on random FDs they get
> > from the outside world. They do call read() on those FDs, which is why
>
> That programs generally don't do something only means the attack is
> less probable.
>
> Programs generally aren't suid. Programs generally don't use
> SCM_RIGHTS. Programs generally don't ignore the retval of
> open/socket/uffd syscalls. Programs generally don't make assumptions
> on the fd ID after one of those syscalls that install fds.
>
> If all programs generally do the right thing (where the most important
> is to not make assumptions on the fd IDs and to check all syscall
> retvals), there was never an issue to begin with even in uffd API.
"The right thing" is a matter of contracts. If a program calls read()
and behaves as if read() has only the effects read() is documented to
have, that means that from the kernel's point of view, the program is
doing the right thing. That you think certain practices are more
prudent than others is irrelevant here. UFFD is a violation of
read()'s *contract* and so if programs break after calling read(),
it's the *kernel*'s fault.
> > read() having unexpected side effects is terrible.
>
> If having unexpected side effects in read is "terrible" (i.e. I
> personally prefer to use terms like terrible when there's at least
> something that can be exploited in practice, not for theoretical
> issues) for an SCM_RIGHTS receiving daemon, I just don't see how the
> exact same unexpected (still theoretical) side effects in recvmsg with
> an unexpected nested cmsg->cmsg_type == SCM_RIGHTS message being
> returned, isn't terrible too.
If a program calls recvmsg on an FD of unknown provenance, it *must*
be prepared to receive file descriptors via SCM_RIGHTS. If it doesn't,
it's a bug. The contract the kernel makes with userspace for recvmsg()
includes the possibility of creating file descriptors. The contract
the kernel makes with userspace for read() does not ordinarily involve
creating file descriptors, so if the kernel does in fact do that, it's
the kernel's problem.
> > If you call it with a non-empty ancillary data buffer, you know to
> > react to what you get. You're *opting into* the possibility of getting
> > file descriptors. Sure, it's theoretically possible that a program
> > calls recvmsg on random FDs it gets from unknown sources, sees
> > SCM_RIGHTS unexpectedly, and just the SCM_RIGHTS message and its FD
> > payload, but that's an outright bug, while calling read() on stdin is
> > no bug.
>
> I'm not talking about stdin and suid. recvmsg might mitigate the
> concern for suid (not certain, depends on the suid, if it's generally
> doing what you expect most suid to be doing or not), I was talking
> about the SCM_RIGHTS receiving daemon instead, the "worse" more
> concerning case than the suid.
>
> I quote below Andy's relevant email:
>
> ======
> It's worse if SCM_RIGHTS is involved.
> ======
>
> Not all software will do this after calling recvmsg:
>
> if (cmsg->cmsg_type == SCM_RIGHTS) {
> /* oops we got attacked and an fd was involountarily installed
> because we received another AF_UNIX from a malicious attacker
> in control of the other end of the SCM_RIGHTS-receiving
> AF_UNIX connection instead of our expected socket family
> which doesn't even support SCM_RIGHTS so we would never have
> noticed an fd was installed after recvmsg */
> }
If a program omits this code after calling recvmsg on a file
descriptor of unknown provenance and the program breaks, it's the
program's fault. It's reasonable to epect that recvmsg might create
file descriptors if you call it on an unknown FD. It's unreasonable to
expect a program to consider the possibility of read() creating file
descriptors because read isn't documented to do that.
>
On Thu, Nov 07, 2019 at 10:50:26AM -0800, Daniel Colascione wrote:
> On Thu, Nov 7, 2019 at 10:23 AM Andrea Arcangeli <[email protected]> wrote:
> > Not all software will do this after calling recvmsg:
> >
> > if (cmsg->cmsg_type == SCM_RIGHTS) {
> > /* oops we got attacked and an fd was involountarily installed
> > because we received another AF_UNIX from a malicious attacker
> > in control of the other end of the SCM_RIGHTS-receiving
> > AF_UNIX connection instead of our expected socket family
> > which doesn't even support SCM_RIGHTS so we would never have
> > noticed an fd was installed after recvmsg */
> > }
>
> If a program omits this code after calling recvmsg on a file
> descriptor of unknown provenance and the program breaks, it's the
> program's fault. [..]
Hmm, ok, would it be possible to do a research effort and check how
much software that receives an fd through SCM_RIGHTS and then calls
recvmsg on it, always remembers to follow the recvsmg with a if
(cmsg->cmsg_type == SCM_RIGHTS) path that closes the involuntarily
opened fd?
Frankly until today, I didn't realize that not adding a specialized
"cmsg->cmsg_type == SCM_RIGHTS" check after every recvmsg executed on
any fd received with SCM_RIGHTS, was a program's fault and it made the
program vulnerable (non robust) from an attack from the other end of
the AF_UNIX pipe just like if the program issued a read() with uffd
event fork being sent through SCM_RIGHTS (except in that case it
wasn't program's fault because read had not to install the fd while
recvmsg can always install fd if cmsg_type == SCM_RIGHTS is returned).
The main argument in favor of recvmsg would be that even if we use it
for uffd too, it won't make recvmsg on an fd received from SCM_RIGHTS
any less secure than it already is, but it's not exactly an example of
robustness in terms of API if it's a program's fault if it doesn't
follow every recvmsg with the above quoted check.
The ioctl is much more robust because there's no chance that somebody
can be attacked by forgetting a specialized non intuitive check after
calling the specialized ioctl that installs the fd.
Thanks,
Andrea
On Thu, Nov 7, 2019 at 10:23 AM Andrea Arcangeli <[email protected]> wrote:
>
> On Thu, Nov 07, 2019 at 08:15:53AM -0800, Daniel Colascione wrote:
> > You're already paying for bounds checking. Receiving a message via a
> > datagram socket is basically the same thing as what UFFD's read is
> > doing anyway.
>
> Except it's synchronous and there are no dynamic allocations required
> in uffd, while af_netlink and af_unix both all deal with queue of
> events in skbs dynamically allocated.
>
> Ultimately if we strip away the skbs for performance reasons, there
> wouldn't be much code to share, so if the only benefit would be to
> call recvmsg which would still be as insecure as read for the "worse"
> case than suid, so I don't see the point.
Not sure what you mean.
>
> And should then eventfd also become a netlink then? I mean uffd was
> supposed to work like eventfd except it requires specialized events.
No. None of this even means that these objects should be sockets per
se. The point is that anyone who calls recvmsg() and passes a control
buf *must* handle SCM_RIGHTS because even very old Unixes can
materialize file descriptors. The only exception is if the program
knows a priori that the fd refers to a socket that can't use
SCM_RIGHTS.
In other words, failing to handle file descriptors returned by
recvmsg() is an application bug. Failing to handle file descriptors
returned by read() is not an application bug -- it's a kernel bug.
> > If you call it with a non-empty ancillary data buffer, you know to
> > react to what you get. You're *opting into* the possibility of getting
> > file descriptors. Sure, it's theoretically possible that a program
> > calls recvmsg on random FDs it gets from unknown sources, sees
> > SCM_RIGHTS unexpectedly, and just the SCM_RIGHTS message and its FD
> > payload, but that's an outright bug, while calling read() on stdin is
> > no bug.
>
> I'm not talking about stdin and suid. recvmsg might mitigate the
> concern for suid (not certain, depends on the suid, if it's generally
> doing what you expect most suid to be doing or not), I was talking
> about the SCM_RIGHTS receiving daemon instead, the "worse" more
> concerning case than the suid.
>
> I quote below Andy's relevant email:
>
> ======
> It's worse if SCM_RIGHTS is involved.
> ======
>
> Not all software will do this after calling recvmsg:
>
> if (cmsg->cmsg_type == SCM_RIGHTS) {
> /* oops we got attacked and an fd was involountarily installed
> because we received another AF_UNIX from a malicious attacker
> in control of the other end of the SCM_RIGHTS-receiving
> AF_UNIX connection instead of our expected socket family
> which doesn't even support SCM_RIGHTS so we would never have
> noticed an fd was installed after recvmsg */
> }
>
You've misunderstood what you're quoting me as saying. I'm saying
that the issue is worse if you pass the userfaultfd via SCM_RIGHTS to
an unsuspecting program. It is perfectly valid to receive a file
descriptor via SCM_RIGHTS and then call read(), at least so long as
you are okay with potentially blocking.
If you receive a fd to a socket using SCM_RIGHTS and then you fail to
check cmsg_type as above, then you have a bug regardless of
userfaultfd.
--Andy