Re: [libseccomp-discuss] [PATCH v3 0/2] Add support for MIPS BE/LE and O32 ABI

[Markos Chandras <Markos.Chandras@imgtec.com>]

On 04/17/2014 05:20 PM, Andy Lutomirski wrote:
> [cc's added]
>
> On Thu, Apr 17, 2014 at 9:08 AM, Eric Paris <eparis@redhat.com> wrote:
>> On Thu, 2014-04-17 at 17:05 +0100, Markos Chandras wrote:
>>> On 04/17/2014 04:38 PM, Paul Moore wrote:
>>>>> Similarly, for MIPS, restricting open() on all 3 ABIs means 3 filters.
>>>>> 1) AUDIT_ARCH_MIPS(EL) syscall=4005
>>>>> 2) AUDIT_ARCH_MIPS64(EL) syscall=5005 (n64)
>>>>> 3) AUDIT_ARCH_MIPS64(EL) syscall=6005 (n32)
>>>>>
>>>>> Is this bad?
>>>>
>>>> In my seccomp-heavy opinion it isn't good, but we can work around it.  MIPS64
>>>> looks like x86_64/x32, which means we can't identify the ABI by the AUDIT_ARCH
>>>> token alone, we need to factor in the syscall number as well; this complicates
>>>> the filter generation as well as the filter itself.
>>>>
>>>> Take a look at the x86_64 BPF generated from the 01-sim-allow test.  You'll
>>>> notice that the test creates a seccomp filter without any rules, simply a
>>>> default action, yet if you look at the raw BPF below you will notice that we
>>>> are checking both the the architecture token ($data[4]) and the syscall
>>>> ($data[0]).  Granted, this is a contrived example (look at the more complex
>>>> multi-arch examples to understand why this is important) but it is a simple
>>>> demonstration.
>>>>
>>>>    line  OP   JT   JF   K
>>>> =================================
>>>>    0000: 0x20 0x00 0x00 0x00000004   ld  $data[4]
>>>>    0001: 0x15 0x00 0x03 0xc000003e   jeq 3221225534 true:0002 false:0005
>>>>    0002: 0x20 0x00 0x00 0x00000000   ld  $data[0]
>>>>    0003: 0x35 0x01 0x00 0x40000000   jge 1073741824 true:0005 false:0004
>>>>    0004: 0x06 0x00 0x00 0x7fff0000   ret ALLOW
>>>>    0005: 0x06 0x00 0x00 0x00000000   ret KILL
>>>
>>> I see what you mean. That was very helpful
>>>
>>>> [.....]
>>>>> Even if seccomp could identify the ABI, you still need filters to restrict
>>>>> the actual system calls.
>>>>
>>>> Let me twist the phrasing above a bit ... The libseccomp library must be able
>>>> to identify the ABI and apply the correct ABI specific filter rules.
>>>>
>>>>> I am sorry if my replies make no sense, but it's probably because I
>>>>> don't understand why multiple filters (1 filter per ABI syscall) is not
>>>>> an option.
>>>>
>>>> It is more than an option, it is a requirement. :)
>>>
>>> I understand the problem now. So yeah, it's not a problem, it's more
>>> like a desired optimization to simplify the logic in filters as well as
>>> making them less complex. And it's not libseccomp specific.
>>>
>>> So, a quick patch to solve this in the kernel would be something like
>>> the following one (completely untested). Given this code hasn't been
>>> part of a released kernel, I believe there is time to add this to 3.15.
>>> Would something like this make things simpler?
>>>
>>> diff --git a/arch/mips/include/asm/syscall.h
>>> b/arch/mips/include/asm/syscall.h
>>> index c6e9cd2..bd7543c 100644
>>> --- a/arch/mips/include/asm/syscall.h
>>> +++ b/arch/mips/include/asm/syscall.h
>>> @@ -133,6 +133,8 @@ static inline int syscall_get_arch(void)
>>>    #ifdef CONFIG_64BIT
>>>            if (!test_thread_flag(TIF_32BIT_REGS))
>>>                    arch |= __AUDIT_ARCH_64BIT;
>>> +        if (test_thread_flag(TIF_32BIT_ADDR))
>>> +                arch |= __AUDIT_ARCH_MIPS64_N32;
>>>    #endif
>>>    #if defined(__LITTLE_ENDIAN)
>>>            arch |=  __AUDIT_ARCH_LE;
>>> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
>>> index 11917f7..6bd9322 100644
>>> --- a/include/uapi/linux/audit.h
>>> +++ b/include/uapi/linux/audit.h
>>> @@ -334,6 +334,8 @@ enum {
>>>    /* distinguish syscall tables */
>>>    #define __AUDIT_ARCH_64BIT 0x80000000
>>>    #define __AUDIT_ARCH_LE    0x40000000
>>> +#define __AUDIT_ARCH_MIPS64_N32 0x20000000
>>> +
>>>    #define AUDIT_ARCH_ALPHA
>>> (EM_ALPHA|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE)
>>>    #define AUDIT_ARCH_ARM          (EM_ARM|__AUDIT_ARCH_LE)
>>>    #define AUDIT_ARCH_ARMEB        (EM_ARM)
>>> @@ -346,7 +348,11 @@ enum {
>>>    #define AUDIT_ARCH_MIPS         (EM_MIPS)
>>>    #define AUDIT_ARCH_MIPSEL       (EM_MIPS|__AUDIT_ARCH_LE)
>>>    #define AUDIT_ARCH_MIPS64       (EM_MIPS|__AUDIT_ARCH_64BIT)
>>> +#define AUDIT_ARCH_MIPS64N32    (EM_MIPS|__AUDIT_ARCH_64BIT|\
>>> +                                 __AUDIT_ARCH_MIPS64_N32)
>>>    #define AUDIT_ARCH_MIPSEL64
>>> (EM_MIPS|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE)
>>> +#define AUDIT_ARCH_MIPSEL64N32
>>> (EM_MIPS|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE\
>>> +                                 __AUDIT_ARCH_MIPS64_N32)
>>>    #define AUDIT_ARCH_OPENRISC     (EM_OPENRISC)
>>>    #define AUDIT_ARCH_PARISC       (EM_PARISC)
>>>    #define AUDIT_ARCH_PARISC64     (EM_PARISC|__AUDIT_ARCH_64BIT)
>>
>> I love it from both an audit and libseccomp PoV...
>>
>
> I know nothing about the MIPS entry code, but the concept is:
>
> Acked-by: Andy Lutomirski <luto@amacapital.net>
>
> That being said, here's a minor nit:
>
> #define __AUDIT_ARCH_MIPS64_N32 0x20000000
>
> in a cross-arch header doesn't seem like the best idea.  Might it be
> better to do:
>
> /* These bits disambiguate different calling conventions that share an
> ELF machine type, bitness, and endianness */
> #define __AUDIT_ARCH_CONVENTION_MASK 0x30000000
> #define __AUDIT_ARCH_CONVENTION_MIPS64_N32 0x20000000
>
> This will encourage reuse of the same bit the next time this happens.
>
> --Andy
>

Thanks. I will change the patch based on your proposal and send it to 
the kernel mailing lists for review.

-- 
markos