Re: [PATCH 05/10] CaitSith: Add LSM interface management file.

[Kees Cook <keescook@chromium.org>]

On Thu, Nov 03, 2022 at 02:10:20AM +0900, Tetsuo Handa wrote:
> This file is used for registering CaitSith module into the
> security_hook_heads list. Further patches will not be interesting for
> reviewers, for further patches are providing similar functions provided
> by TOMOYO (but too different to share the code).
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> [...]
> +#define cs_debug_trace(pos)						\
> +	do {								\
> +		static bool done;					\
> +		if (!done) {						\
> +			pr_info("CAITSITH: Debug trace: " pos " of 2\n"); \
> +			done = true;					\
> +		}							\
> +	} while (0)

This can be replaced by pr_info_once().

> [...]
> +#if defined(CONFIG_STRICT_KERNEL_RWX) && !defined(CONFIG_SECURITY_WRITABLE_HOOKS)
> +#include <linux/uaccess.h> /* copy_to_kernel_nofault() */
> +#define NEED_TO_CHECK_HOOKS_ARE_WRITABLE
> +
> +#if defined(CONFIG_X86)
> +#define MAX_RO_PAGES 1024
> +static struct page *ro_pages[MAX_RO_PAGES] __initdata;
> +static unsigned int ro_pages_len __initdata;
> +
> +static bool __init lsm_test_page_ro(void *addr)
> +{
> +	unsigned int i;
> +	int unused;
> +	struct page *page;
> +
> +	page = (struct page *) lookup_address((unsigned long) addr, &unused);
> +	if (!page)
> +		return false;
> +	if (test_bit(_PAGE_BIT_RW, &(page->flags)))
> +		return true;
> +	for (i = 0; i < ro_pages_len; i++)
> +		if (page == ro_pages[i])
> +			return true;
> +	if (ro_pages_len == MAX_RO_PAGES)
> +		return false;
> +	ro_pages[ro_pages_len++] = page;
> +	return true;
> +}
> +
> +static bool __init check_ro_pages(struct security_hook_heads *hooks)
> +{
> +	int i;
> +	struct hlist_head *list = &hooks->capable;
> +
> +	if (!copy_to_kernel_nofault(list, list, sizeof(void *)))
> +		return true;
> +	for (i = 0; i < ARRAY_SIZE(caitsith_hooks); i++) {
> +		struct hlist_head *head = caitsith_hooks[i].head;
> +		struct security_hook_list *shp;
> +
> +		if (!lsm_test_page_ro(&head->first))
> +			return false;
> +		hlist_for_each_entry(shp, head, list)
> +			if (!lsm_test_page_ro(&shp->list.next) ||
> +			    !lsm_test_page_ro(&shp->list.pprev))
> +				return false;
> +	}
> +	return true;
> +}
> +#else
> +static bool __init check_ro_pages(struct security_hook_heads *hooks)
> +{
> +	struct hlist_head *list = &hooks->capable;
> +
> +	return !copy_to_kernel_nofault(list, list, sizeof(void *));
> +}
> +#endif
> +#endif
> +
> +/**
> + * cs_init - Initialize this module.
> + *
> + * Returns 0 on success, negative value otherwise.
> + */
> +static int __init cs_init(void)
> +{
> +	int idx;
> +#if defined(NEED_TO_CHECK_HOOKS_ARE_WRITABLE)
> +	if (!check_ro_pages(&security_hook_heads)) {
> +		pr_info("Can't update security_hook_heads due to write protected. Retry with rodata=0 kernel command line option added.\n");
> +		return -EINVAL;
> +	}
> +#endif
> +	for (idx = 0; idx < CS_MAX_TASK_SECURITY_HASH; idx++)
> +		INIT_LIST_HEAD(&cs_task_security_list[idx]);
> +	cs_init_module();
> +#if defined(NEED_TO_CHECK_HOOKS_ARE_WRITABLE) && defined(CONFIG_X86)
> +	for (idx = 0; idx < ro_pages_len; idx++)
> +		set_bit(_PAGE_BIT_RW, &(ro_pages[idx]->flags));
> +#endif
> +	swap_hook(&caitsith_hooks[0], &original_task_free);
> +	swap_hook(&caitsith_hooks[1], &original_cred_prepare);
> +	swap_hook(&caitsith_hooks[2], &original_task_alloc);
> +	for (idx = 3; idx < ARRAY_SIZE(caitsith_hooks); idx++)
> +		add_hook(&caitsith_hooks[idx]);
> +#if defined(NEED_TO_CHECK_HOOKS_ARE_WRITABLE) && defined(CONFIG_X86)
> +	for (idx = 0; idx < ro_pages_len; idx++)
> +		clear_bit(_PAGE_BIT_RW, &(ro_pages[idx]->flags));
> +#endif
> +	return 0;
> +}

I'm sorry, but absolutely not. One of the most basic elements of the
LSM infrastructure is that it is read-only. Even __lsm_ro_after_init is
a grand-fathered behavior that is supposed to be removed once all the
old SELinux disable-at-runtime users are gone.

I don't see any _justification_ for why any of this is needed. Yes,
it is technically possible to make an LSM loadable, but there needs to
be a convincing rationale for _why_.

-Kees

-- 
Kees Cook