Re: Seccomp implications for glibc wrapper function changes

Hi Adhemerval

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

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.)

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.)

My "broken" code was just some demo code, and I got surprised by the
behavior change. And I agree with pretty much everything you say,
especially the point about needing good system level knowledge when
you are doing system call filtering (and of course a real-world
application that cared about filtering open() should also be filtering
for openat()). This is one of the reasons that Linux man-pages have
steadily been acquiring subsections called "C library/kernel
differences". But I did just want to raise some awareness about that
these sorts of changes could be a possible issue in some cases.

(By the way, unless I am missing something, there is no __NR_openat2?)

Now for the reasoning of using __NR_openat is since we have support
for it on all architecture it meant less logic to handle possible
architecture differences.

Fair enough. Maybe this could be stated more explicitly in the commit
message though?

With best regards,

Michael

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