On 24/03/2016 17:01, Casey Schaufler wrote:
> On 3/23/2016 6:46 PM, Mickaël Salaün wrote:
>> diff --git a/security/seccomp/lsm.c b/security/seccomp/lsm.c
>> new file mode 100644
>> index 000000000000..93c881724341
>> --- /dev/null
>> +++ b/security/seccomp/lsm.c
>> @@ -0,0 +1,87 @@
>> +/*
>> + * Seccomp Linux Security Module
>> + *
>> + * Copyright (C) 2016 Mickaël Salaün <mic@xxxxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2, as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <asm/syscall.h> /* sys_call_table */
>> +#include <linux/compat.h>
>> +#include <linux/slab.h> /* kcalloc() */
>> +#include <linux/syscalls.h> /* syscall_argdesc */
>> +
>> +#include "lsm.h"
>> +
>> +/* TODO: Remove the need for CONFIG_SYSFS dependency */
>> +
>> +struct syscall_argdesc (*seccomp_syscalls_argdesc)[] = NULL;
>> +#ifdef CONFIG_COMPAT
>> +struct syscall_argdesc (*compat_seccomp_syscalls_argdesc)[] = NULL;
>> +#endif /* CONFIG_COMPAT */
>> +
>> +static const struct syscall_argdesc *__init
>> +find_syscall_argdesc(const struct syscall_argdesc *start,
>> + const struct syscall_argdesc *stop, const void *addr)
>> +{
>> + if (unlikely(!addr || !start || !stop)) {
>> + WARN_ON(1);
>> + return NULL;
>> + }
>> +
>> + for (; start < stop; start++) {
>> + if (start->addr == addr)
>> + return start;
>> + }
>> + return NULL;
>> +}
>> +
>> +static inline void __init init_argdesc(void)
>> +{
>> + const struct syscall_argdesc *argdesc;
>> + const void *addr;
>> + int i;
>> +
>> + seccomp_syscalls_argdesc = kcalloc(NR_syscalls,
>> + sizeof((*seccomp_syscalls_argdesc)[0]), GFP_KERNEL);
>> + if (unlikely(!seccomp_syscalls_argdesc)) {
>> + WARN_ON(1);
>> + return;
>> + }
>> + for (i = 0; i < NR_syscalls; i++) {
>> + addr = sys_call_table[i];
>> + argdesc = find_syscall_argdesc(__start_syscalls_argdesc,
>> + __stop_syscalls_argdesc, addr);
>> + if (!argdesc)
>> + continue;
>> +
>> + (*seccomp_syscalls_argdesc)[i] = *argdesc;
>> + }
>> +
>> +#ifdef CONFIG_COMPAT
>> + compat_seccomp_syscalls_argdesc = kcalloc(IA32_NR_syscalls,
>> + sizeof((*compat_seccomp_syscalls_argdesc)[0]),
>> + GFP_KERNEL);
>> + if (unlikely(!compat_seccomp_syscalls_argdesc)) {
>> + WARN_ON(1);
>> + return;
>> + }
>> + for (i = 0; i < IA32_NR_syscalls; i++) {
>> + addr = ia32_sys_call_table[i];
>> + argdesc = find_syscall_argdesc(__start_compat_syscalls_argdesc,
>> + __stop_compat_syscalls_argdesc, addr);
>> + if (!argdesc)
>> + continue;
>> +
>> + (*compat_seccomp_syscalls_argdesc)[i] = *argdesc;
>> + }
>> +#endif /* CONFIG_COMPAT */
>> +}
>> +
>> +void __init seccomp_init(void)
>> +{
>> + pr_info("seccomp: Becoming ready for sandboxing\n");
>> + init_argdesc();
>> +}
>
> This isn't using the LSM infrastructure at all, is it?
> It looks like the only reason you're calling it a security
> module is to get the initialization code called in
> security_init().
>
> Let me amend my previous comment, which was to change
> the name of seccomp_init(). Leave it as is, but add a
> comment before it that explains why you've put the
> call in the midst of the security module initialization.
The patch "[RFC v1 16/17] security/seccomp: Protect against filesystem TOCTOU" add LSM hooks, so it make sense to follow your first comment and rename seccomp_init() to seccomp_add_hooks().
Mickaël
Attachment:
signature.asc
Description: OpenPGP digital signature
