Re: Seccomp implications for glibc wrapper function changes

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 07/11/2017 19:47, Michael Kerrisk (man-pages) wrote:
> Hi Adhemerval
> 
> On 7 November 2017 at 22:14, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>> On 07/11/2017 18:35, Michael Kerrisk (man-pages) wrote:
>>> 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 <adhemerval.zanella@linaro.org>
>>>      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.

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

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

I would expect a more high level library such libseccomp to provide
family like filter (to get all variant of open, rename, etc.).

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

There is not indeed, I confused with rename which has 3 variants
(__NR_rename, __NR_renameat, and __NR_renameat2).

> 
>> 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?

I assumed it was implicit by the idea of consolidation. But I agree
a more thoughtfully explanation on commit won't hurt.