[cc's added]
On Thu, Apr 17, 2014 at 9:08 AM, Eric Paris <eparis@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
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
