Generic vDSO breaks seccomp-enabled userspace on i386

The generic vDSO implementation, starting with commit

7ac870747988 ("x86/vdso: Switch to generic vDSO implementation")

breaks seccomp-enabled userspace on 32-bit x86 (i386) kernels. Prior to the generic implementation, the x86 vDSO used identical code for both x86_64 and i386 kernels, which worked because it did all calcuations using structs with naturally sized variables, i.e. didn't use __kernel_timespec.

The generic vDSO does its internal calculations using __kernel_timespec, which in turn requires the i386 fallback syscall to use the 64-bit variation, __NR_clock_gettime64.

Quoted 11 lines · Sean Christopherson

The generic vDSO implementation, starting with commit

7ac870747988 ("x86/vdso: Switch to generic vDSO implementation")

breaks seccomp-enabled userspace on 32-bit x86 (i386) kernels. Prior to the generic implementation, the x86 vDSO used identical code for both x86_64 and i386 kernels, which worked because it did all calcuations using structs with naturally sized variables, i.e. didn't use __kernel_timespec.

Quoted 3 lines

The generic vDSO does its internal calculations using __kernel_timespec, which in turn requires the i386 fallback syscall to use the 64-bit variation, __NR_clock_gettime64.

This is basically doomed to break eventually, right?

I’ve occasionally considered adding a concept of “seccomp aliases”. The idea is that, if a filter returns anything other than ALLOW, we re-run it with a different nr that we dig out it a small list of such cases. This would be limited to cases where the new syscall does the same thing with the same arguments.

I want this for restart_syscall: I want to renumber it.

Quoted 11 lines · Andy Lutomirski

Quoted 11 lines · Sean Christopherson

The generic vDSO implementation, starting with commit

7ac870747988 ("x86/vdso: Switch to generic vDSO implementation")

breaks seccomp-enabled userspace on 32-bit x86 (i386) kernels. Prior to the generic implementation, the x86 vDSO used identical code for both x86_64 and i386 kernels, which worked because it did all calcuations using structs with naturally sized variables, i.e. didn't use __kernel_timespec.

Quoted 5 lines

Quoted 3 lines

The generic vDSO does its internal calculations using __kernel_timespec, which in turn requires the i386 fallback syscall to use the 64-bit variation, __NR_clock_gettime64.

This is basically doomed to break eventually, right?

Just so I'm understanding: the vDSO change introduced code to make an actual syscall on i386, which for most seccomp filters would be rejected?

Quoted 2 lines · Kees Cook

Just so I'm understanding: the vDSO change introduced code to make an actual syscall on i386, which for most seccomp filters would be rejected?

No. The old x86 specific VDSO implementation had a fallback syscall as well, i.e. clock_gettime(). On 32bit clock_gettime() uses the y2038 endangered timespec.

So when the VDSO was made generic we changed the internal data structures to be 2038 safe right away. As a consequence the fallback syscall is not clock_gettime(), it's clock_gettime64(). which seems to surprise seccomp.

Quoted 11 lines · Thomas Gleixner

Quoted 2 lines · Kees Cook

Just so I'm understanding: the vDSO change introduced code to make an actual syscall on i386, which for most seccomp filters would be rejected?

No. The old x86 specific VDSO implementation had a fallback syscall as well, i.e. clock_gettime(). On 32bit clock_gettime() uses the y2038 endangered timespec.

So when the VDSO was made generic we changed the internal data structures to be 2038 safe right away. As a consequence the fallback syscall is not clock_gettime(), it's clock_gettime64(). which seems to surprise seccomp.

Okay, it's didn't add a syscall, it just changed it. Results are the
same: conservative filters suddenly start breaking due to the different call. (And now I see why Andy's alias suggestion would help...)

I'm not sure which direction to do with this. It seems like an alias list is a large hammer for this case, and a "seccomp-bypass when calling from vDSO" solution seems too fragile?

Quoted 14 lines · Kees Cook

Quoted 11 lines · Thomas Gleixner

Quoted 2 lines · Kees Cook

Just so I'm understanding: the vDSO change introduced code to make an actual syscall on i386, which for most seccomp filters would be rejected?

No. The old x86 specific VDSO implementation had a fallback syscall as well, i.e. clock_gettime(). On 32bit clock_gettime() uses the y2038 endangered timespec.

So when the VDSO was made generic we changed the internal data structures to be 2038 safe right away. As a consequence the fallback syscall is not clock_gettime(), it's clock_gettime64(). which seems to surprise seccomp.

Quoted 8 lines

Okay, it's didn't add a syscall, it just changed it. Results are the
same: conservative filters suddenly start breaking due to the different call. (And now I see why Andy's alias suggestion would help...)

I'm not sure which direction to do with this. It seems like an alias list is a large hammer for this case, and a "seccomp-bypass when calling from vDSO" solution seems too fragile?

I don't like the seccomp bypass at all. If someone uses seccomp to disallow all clock_gettime() variants, there shouldn't be a back door to learn the time.

Here's the restart_syscall() logic that makes me want aliases: we have different syscall numbers for restart_syscall() on 32-bit and 64-bit. The logic to decide which one to use is dubious at best. I'd like to introduce a restart_syscall2() that is identical to restart_syscall() except that it has the same number on both variants.

Quoted 28 lines · Andy Lutomirski

Quoted 14 lines · Kees Cook

Quoted 11 lines · Thomas Gleixner

Quoted 2 lines · Kees Cook

Just so I'm understanding: the vDSO change introduced code to make an actual syscall on i386, which for most seccomp filters would be rejected?

No. The old x86 specific VDSO implementation had a fallback syscall as well, i.e. clock_gettime(). On 32bit clock_gettime() uses the y2038 endangered timespec.

So when the VDSO was made generic we changed the internal data structures to be 2038 safe right away. As a consequence the fallback syscall is not clock_gettime(), it's clock_gettime64(). which seems to surprise seccomp.

Quoted 8 lines

Okay, it's didn't add a syscall, it just changed it. Results are the
same: conservative filters suddenly start breaking due to the different call. (And now I see why Andy's alias suggestion would help...)

I'm not sure which direction to do with this. It seems like an alias list is a large hammer for this case, and a "seccomp-bypass when calling from vDSO" solution seems too fragile?

I don't like the seccomp bypass at all. If someone uses seccomp to disallow all clock_gettime() variants, there shouldn't be a back door to learn the time.

Quoted 5 lines

Here's the restart_syscall() logic that makes me want aliases: we have different syscall numbers for restart_syscall() on 32-bit and 64-bit. The logic to decide which one to use is dubious at best. I'd like to introduce a restart_syscall2() that is identical to restart_syscall() except that it has the same number on both variants.

I've built a straw-man for this idea... but I have to say I don't like it. This can lead to really unexpected behaviors if someone were to have differing filters for the two syscalls. For example, let's say someone was doing a paranoid audit of 2038-unsafe clock usage and marked clock_gettime() with RET_KILL and marked clock_gettime64() with RET_LOG. This aliasing would make clock_gettime64() trigger with RET_KILL...

kernel/seccomp.c +7 -0

+       /* Handle syscall aliases when result is not SECCOMP_RET_ALLOW. */
+       if (unlikely(action != SECCOMP_RET_ALLOW)) {
+               int alias;
+
+               alias = seccomp_syscall_alias(sd->arch, sd->nr);
+               if (unlikely(alias != -1)) {
+                       /* Use sd_local for an aliased syscall. */

I don't love this whole concept, but I also don't have a better idea.

Quoted 4 lines · Andy Lutomirski

I don't love this whole concept, but I also don't have a better idea.

Are we really talking about changing the kernel because BPF is expecting things? That is, did we just elevate everything BPF can observe to ABI?

Quoted 6 lines · Peter Zijlstra

Quoted 3 lines · Andy Lutomirski

Quoted 5 lines

Quoted 1 line

I don't love this whole concept, but I also don't have a better idea.

Are we really talking about changing the kernel because BPF is expecting things? That is, did we just elevate everything BPF can observe to ABI?

No, this isn’t about internals in the kernel mode sense.

It’s about the smallish number of cases where the kernel causes user code to do a specific syscall and the user has a policy that doesn’t allow that syscall. This is visible to user code via seccomp and ptrace.

Yes, it’s obnoxious. Do you have any suggestions?

Quoted 7 lines · Kees Cook

I've built a straw-man for this idea... but I have to say I don't like it. This can lead to really unexpected behaviors if someone were to have differing filters for the two syscalls. For example, let's say someone was doing a paranoid audit of 2038-unsafe clock usage and marked clock_gettime() with RET_KILL and marked clock_gettime64() with RET_LOG. This aliasing would make clock_gettime64() trigger with RET_KILL...

This particular issue is solvable:

Quoted 7 lines

kernel/seccomp.c +7 -0

+       /* Handle syscall aliases when result is not SECCOMP_RET_ALLOW. */
+       if (unlikely(action != SECCOMP_RET_ALLOW)) {
+               int alias;
+
+               alias = seccomp_syscall_alias(sd->arch, sd->nr);
+               if (unlikely(alias != -1)) {
+                       /* Use sd_local for an aliased syscall. */

kernel/seccomp.c static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd, · line 826

Quoted 10 lines

kernel/seccomp.c +10 -0

+                       if (sd != &sd_local) {
+                               sd_local = *sd;
+                               sd = &sd_local;
+                       }
+                       sd_local.nr = alias;
+
+                       /* Run again, with the alias, accepting the results. */
+                       filter_ret = seccomp_run_filters(sd, &match);
+                       data = filter_ret & SECCOMP_RET_DATA;
+                       action = filter_ret & SECCOMP_RET_ACTION_FULL;

How about:

new_data = ...;
new_action = ...;
if (new_action == SECCOMP_RET_ALLOW) {
data = new_data;
action = new_action;
}

It might also be nice to allow a filter to say "hey, I want to set this result and I do *not* want compatibility aliases applied", but I'm not quite sure how to express that.

I don't love this whole concept, but I also don't have a better idea.

Quoted 31 lines · Andy Lutomirski

Quoted 7 lines · Kees Cook

I've built a straw-man for this idea... but I have to say I don't like it. This can lead to really unexpected behaviors if someone were to have differing filters for the two syscalls. For example, let's say someone was doing a paranoid audit of 2038-unsafe clock usage and marked clock_gettime() with RET_KILL and marked clock_gettime64() with RET_LOG. This aliasing would make clock_gettime64() trigger with RET_KILL...

This particular issue is solvable:

Quoted 7 lines

kernel/seccomp.c +7 -0

+       /* Handle syscall aliases when result is not SECCOMP_RET_ALLOW. */
+       if (unlikely(action != SECCOMP_RET_ALLOW)) {
+               int alias;
+
+               alias = seccomp_syscall_alias(sd->arch, sd->nr);
+               if (unlikely(alias != -1)) {
+                       /* Use sd_local for an aliased syscall. */

kernel/seccomp.c static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd, · line 826

Quoted 10 lines

kernel/seccomp.c +10 -0

+                       if (sd != &sd_local) {
+                               sd_local = *sd;
+                               sd = &sd_local;
+                       }
+                       sd_local.nr = alias;
+
+                       /* Run again, with the alias, accepting the results. */
+                       filter_ret = seccomp_run_filters(sd, &match);
+                       data = filter_ret & SECCOMP_RET_DATA;
+                       action = filter_ret & SECCOMP_RET_ACTION_FULL;

How about:

Quoted 6 lines

new_data = ...;
new_action = ...;
if (new_action == SECCOMP_RET_ALLOW) {
data = new_data;
action = new_action;
}

Spelling it out for myself: this means that if both syscalls have non-RET_ALLOW results, the original result is kept. But if the alias is allowed, allow it. That solves my particular example, but I don't think it's enough. (And it might be just as bad.) What if someone wants to RET_TRACE clock_gettime64 and other syscalls are RET_ALLOWed. Now clock_gettime64 cannot be traced since clock_gettime gets RET_ALLOW and replaces the results.

Quoted 4 lines

It might also be nice to allow a filter to say "hey, I want to set this result and I do *not* want compatibility aliases applied", but I'm not quite sure how to express that.

Especially since we're working on "old" filters.

Quoted 1 line

I don't love this whole concept, but I also don't have a better idea.

How about we revert the vDSO change? :P

I keep coming back to using the vDSO return address as an indicator. Most vDSO calls don't make syscalls, yes? So they're normally unfilterable by seccomp.

What was the prior vDSO behavior?

Quoted 1 line · Andy Lutomirski

I don't love this whole concept, but I also don't have a better idea.

How about we revert the vDSO change? :P

I keep coming back to using the vDSO return address as an indicator. Most vDSO calls don't make syscalls, yes? So they're normally unfilterable by seccomp.

What was the prior vDSO behavior?

And as we have sys_clock_gettime64() exposed for 32bit anyway you need to deal with that in seccomp independently of the VDSO. It does not make sense to treat sys_clock_gettime() differently than sys_clock_gettime64(). They both expose the same information, but the latter is y2038 safe.

Quoted 4 lines · Thomas Gleixner

And as we have sys_clock_gettime64() exposed for 32bit anyway you need to deal with that in seccomp independently of the VDSO. It does not make sense to treat sys_clock_gettime() differently than sys_clock_gettime64(). They both expose the same information, but the latter is y2038 safe.

Okay, so combining Andy's ideas on aliasing and "more seccomp flags", we could declare that clock_gettime64() is not filterable on 32-bit at all without the magic SECCOMP_IGNORE_ALIASES flag or something. Then we would alias clock_gettime64 to clock_gettime _before_ the first evaluation (unless SECCOMP_IGNORE_ALIASES is set)?

(When was clock_gettime64() introduced? Is it too long ago to do this "you can't filter it without a special flag" change?)

Quoted 12 lines · Kees Cook

Quoted 4 lines · Thomas Gleixner

And as we have sys_clock_gettime64() exposed for 32bit anyway you need to deal with that in seccomp independently of the VDSO. It does not make sense to treat sys_clock_gettime() differently than sys_clock_gettime64(). They both expose the same information, but the latter is y2038 safe.

Okay, so combining Andy's ideas on aliasing and "more seccomp flags", we could declare that clock_gettime64() is not filterable on 32-bit at all without the magic SECCOMP_IGNORE_ALIASES flag or something. Then we would alias clock_gettime64 to clock_gettime _before_ the first evaluation (unless SECCOMP_IGNORE_ALIASES is set)?

Quoted 2 lines

(When was clock_gettime64() introduced? Is it too long ago to do this "you can't filter it without a special flag" change?)

clock_gettime64() and the other sys_*time64() syscalls which address the y2038 issue were added in 5.1

+cc Paul

Quoted 14 lines · Thomas Gleixner

Quoted 12 lines · Kees Cook

Quoted 4 lines · Thomas Gleixner

And as we have sys_clock_gettime64() exposed for 32bit anyway you need to deal with that in seccomp independently of the VDSO. It does not make sense to treat sys_clock_gettime() differently than sys_clock_gettime64(). They both expose the same information, but the latter is y2038 safe.

Okay, so combining Andy's ideas on aliasing and "more seccomp flags", we could declare that clock_gettime64() is not filterable on 32-bit at all without the magic SECCOMP_IGNORE_ALIASES flag or something. Then we would alias clock_gettime64 to clock_gettime _before_ the first evaluation (unless SECCOMP_IGNORE_ALIASES is set)?

Quoted 5 lines

Quoted 2 lines

(When was clock_gettime64() introduced? Is it too long ago to do this "you can't filter it without a special flag" change?)

clock_gettime64() and the other sys_*time64() syscalls which address the y2038 issue were added in 5.1

Paul Bolle pointed out that this regression showed up in v5.3-rc1, not v5.2. In Paul's case, systemd-journal is failing.

Quoted 24 lines · Sean Christopherson

+cc Paul

Quoted 19 lines · Thomas Gleixner

Quoted 6 lines · Kees Cook

Quoted 4 lines · Thomas Gleixner

And as we have sys_clock_gettime64() exposed for 32bit anyway you need to deal with that in seccomp independently of the VDSO. It does not make sense to treat sys_clock_gettime() differently than sys_clock_gettime64(). They both expose the same information, but the latter is y2038 safe.

Quoted 8 lines

Okay, so combining Andy's ideas on aliasing and "more seccomp flags", we could declare that clock_gettime64() is not filterable on 32-bit at all without the magic SECCOMP_IGNORE_ALIASES flag or something. Then we would alias clock_gettime64 to clock_gettime _before_ the first evaluation (unless SECCOMP_IGNORE_ALIASES is set)?

(When was clock_gettime64() introduced? Is it too long ago to do this "you can't filter it without a special flag" change?)

clock_gettime64() and the other sys_*time64() syscalls which address the y2038 issue were added in 5.1

Quoted 2 lines

Paul Bolle pointed out that this regression showed up in v5.3-rc1, not v5.2. In Paul's case, systemd-journal is failing.

I think it's getting quite late to start inventing new seccomp features to fix this. I think the right solution for 5.3 is to change the 32-bit vdso fallback to use the old clock_gettime, i.e. clock_gettime32. This is obviously not an acceptable long-term solution.

Quoted 6 lines · Andy Lutomirski

I think it's getting quite late to start inventing new seccomp features to fix this. I think the right solution for 5.3 is to change the 32-bit vdso fallback to use the old clock_gettime, i.e. clock_gettime32. This is obviously not an acceptable long-term solution.

Sigh. I'll have a look....

Quoted 8 lines · Thomas Gleixner

Quoted 6 lines · Andy Lutomirski

I think it's getting quite late to start inventing new seccomp features to fix this. I think the right solution for 5.3 is to change the 32-bit vdso fallback to use the old clock_gettime, i.e. clock_gettime32. This is obviously not an acceptable long-term solution.

Quoted 1 line

Sigh. I'll have a look....

Completely untested patch below.

For the record: I have to say that I hate it.

Quoted 22 lines · Sean Christopherson

Quoted 19 lines · Thomas Gleixner

Quoted 6 lines · Kees Cook

Quoted 4 lines · Thomas Gleixner

And as we have sys_clock_gettime64() exposed for 32bit anyway you need to deal with that in seccomp independently of the VDSO. It does not make sense to treat sys_clock_gettime() differently than sys_clock_gettime64(). They both expose the same information, but the latter is y2038 safe.

Quoted 8 lines

Okay, so combining Andy's ideas on aliasing and "more seccomp flags", we could declare that clock_gettime64() is not filterable on 32-bit at all without the magic SECCOMP_IGNORE_ALIASES flag or something. Then we would alias clock_gettime64 to clock_gettime _before_ the first evaluation (unless SECCOMP_IGNORE_ALIASES is set)?

(When was clock_gettime64() introduced? Is it too long ago to do this "you can't filter it without a special flag" change?)

clock_gettime64() and the other sys_*time64() syscalls which address the y2038 issue were added in 5.1

Quoted 2 lines

Paul Bolle pointed out that this regression showed up in v5.3-rc1, not v5.2. In Paul's case, systemd-journal is failing.

I think it's getting quite late to start inventing new seccomp features to fix this. I think the right solution for 5.3 is to change the 32-bit vdso fallback to use the old clock_gettime, i.e. clock_gettime32. This is obviously not an acceptable long-term solution.

On Sat, Jul 27, 2019 at 7:53 PM Andy Lutomirski <[email protected]> wrote:
lib/vdso/gettimeofday.c:
static __maybe_unused int
__cvdso_clock_gettime32(clockid_t clock, struct old_timespec32 *res)

{
        struct __kernel_timespec ts;
        int ret;

        if (res == NULL)
                goto fallback;

        ret = __cvdso_clock_gettime(clock, &ts);

        if (ret == 0) {
                res->tv_sec = ts.tv_sec;
                res->tv_nsec = ts.tv_nsec;
        }

        return ret;

fallback:

        return clock_gettime_fallback(clock, (struct __kernel_timespec *)res);
}

So we get an 'old_timespec32' pointer from user space, and cast it to __kernel_timespec in order to pass it to the low-level function that actually fills in the 64-bit structure.

On a little-endian machine, the first four bytes are actually correct here, but this is followed by tv_nsec=0 and 8 more bytes that overwrite whatever comes after the user space 'timespec'. [I missed the typecast as an indication of a bug during my review, sorry about that].

We also need to decide what to do about __cvdso_clock_gettime32() once we add a compile-time option to make all time32 syscalls to return an error. Returning -ENOSYS from the clock_gettime32() fallback is probably a good idea, but for consistency the __vdso_clock_gettime() call should either always return the same in that configuration, or be left out from the vdso build endirely.

Quoted 1 line · Arnd Bergmann

On Sat, Jul 27, 2019 at 7:53 PM Andy Lutomirski <[email protected]> wrote:

Which is totally irrelevant because res is NULL and that NULL pointer check should simply return -EFAULT, which is what the syscall fallback returns because the pointer is NULL.

But that NULL pointer check is inconsistent anyway:

- 64 bit does not have it and never had

- the vdso is not capable of handling faults properly anyway. If the pointer is not valid, then it will segfault. So just preventing the segfault for NULL is silly.

I'm going to just remove it.

Quoted 15 lines · Thomas Gleixner

Quoted 1 line · Arnd Bergmann

On Sat, Jul 27, 2019 at 7:53 PM Andy Lutomirski <[email protected]> wrote:

Which is totally irrelevant because res is NULL and that NULL pointer check should simply return -EFAULT, which is what the syscall fallback returns because the pointer is NULL.

But that NULL pointer check is inconsistent anyway:

- 64 bit does not have it and never had

- the vdso is not capable of handling faults properly anyway. If the pointer is not valid, then it will segfault. So just preventing the segfault for NULL is silly.

Quoted 1 line

I'm going to just remove it.

Ah, you are right, I misread.

Anyway, if we want to keep the traditional behavior and get fewer surprises for users of seccomp and anything else that might observe clock_gettime behavior, below is how I'd do it. (not even build tested, x86-only. I'll send a proper patch if this is where we want to take it).

Quoted 18 lines · Arnd Bergmann

Quoted 16 lines · Thomas Gleixner

Quoted 1 line · Arnd Bergmann

On Sat, Jul 27, 2019 at 7:53 PM Andy Lutomirski <[email protected]> wrote:

Which is totally irrelevant because res is NULL and that NULL pointer check should simply return -EFAULT, which is what the syscall fallback returns because the pointer is NULL.

But that NULL pointer check is inconsistent anyway:

- 64 bit does not have it and never had

- the vdso is not capable of handling faults properly anyway. If the pointer is not valid, then it will segfault. So just preventing the segfault for NULL is silly.

I'm going to just remove it.

Quoted 6 lines

Ah, you are right, I misread.

Anyway, if we want to keep the traditional behavior and get fewer surprises for users of seccomp and anything else that might observe clock_gettime behavior, below is how I'd do it. (not even build tested, x86-only. I'll send a proper patch if this is where we want to take it).

I posted a series which fixes up the mess 2 hours before you sent this mail :)