Re: [RFC v1 05/17] security/seccomp: Add LSM and create arrays of syscall metadata

["Mickaël Salaün" <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3599 bytes --]


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-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
>> + *
>> + * 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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]