Re: Seccomp implications for glibc wrapper function changes

Hello Adhemerval

On 8 November 2017 at 02:18, Adhemerval Zanella
[off-list ref] wrote:

On 07/11/2017 19:47, Michael Kerrisk (man-pages) wrote:

quoted

Hi Adhemerval

On 7 November 2017 at 22:14, Adhemerval Zanella
[off-list ref] wrote:

quoted

On 07/11/2017 18:35, Michael Kerrisk (man-pages) wrote:

quoted

Hello,

I was recently testing some code I'd written a while back that makes
use of seccomp filters to control which system calls a process can
make, and I got a surpise when someone showed the code no longer
worked in on a system that had glibc 2.26.

The behavior change resulted from Adhemerval's glibc commit

     commit b41152d716ee9c5ba34495a54e64ea2b732139b5
     Author: Adhemerval Zanella [off-list ref]
     Date:   Fri Nov 11 15:00:03 2016 -0200

        Consolidate Linux open implementation
            [...]
            3. Use __NR_openat as default syscall for open{64}.

The commit in question changed the glibc open() wrapper to swtcch from
use the kernel's open() system call to using the kernel's openat()
system call.

This change broke my code that was doing seccomp filtering for the
open() system call number (__NR_open). The breakage in question is not
serious, since this was really just demonstration code. However, I
want to raise awareness that these sorts of changes have the potential
to possibly cause breakages for some code using seccomp, and note that
I think such changes should not be made lightly or gratuitously. (In
the above commit, it's not clear why the switch was made to using
openat(): there's no mention of the reasoning in the commit message,
nor is there anything that is obvious from reading through the code
change itself.)

Your code would 'break' if you run with on a new architecture that does
not implement __NR_open, which it is the default for new architecture
on Linux.

(Is it the default for new architectures? I was unaware of that.)

Yes, new architectures should use the kernel headers
include/uapi/asm-generic/unistd.h for syscall definition and the idea
is to avoid duplication of functionality like syscalls that supplant
each one (openat and open for instance).  AArch64, for instance, only
supports on compat more for 32 bits.

Thanks for the info.

quoted

quoted

In fact I hardly consider this is a 'break' since the user API we
export does not have any constraint which underlying syscall we use.
For instance, a user can seccomp gettimeofday syscall on a system
without vDSO just to found out it is 'broken' on a vDSO kernel.

I think we should not constraint for this specific usercase; if one
is doing syscall filtering it is expected system level knowledge to
handle all possible syscalls related.  For instance, I would expect
that if the idea is to filtering open() libc implementation the
program should also filter __NR_openat and __NR_openat2 since it
is semantically possible to implement open() with __NR_openat2 if
the syscall is available.

Perhaps my use of the word 'break' was a bit too loaded. (And if my
mail came across as somehow critical of your patch, that was not my
intention, and if you feel it as such, I do apologize for my poor
wording.)

In fact I did not take is a criticize, but rather I was worried users
would expect that kind of syscalls stability over glibc releases.  The
'break' word indeed raise a flag ;)

Okay -- I see your philosophical position. I guess my only thought is
that these sort of changes are likely to at least surprise some
people. I'll see whether I can work some hints about this into a
suitable man page.

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/