Message-ID: <fbfd9d4d-d62e-8535-3f3a-c37a3840544b@schaufler-ca.com>
Date:   Thu, 10 Aug 2023 08:03:43 -0700
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     "Dr. Greg" <greg@...ellic.com>
Cc:     linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, corbet@....net,
        Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH 04/13] Add TSEM master header file.

On 8/9/2023 7:57 PM, Dr. Greg wrote:
> On Mon, Aug 07, 2023 at 01:39:22PM -0700, Casey Schaufler wrote:
>
> Hi Casey, thank you for the review comments.
>
>> On 7/10/2023 3:23 AM, Dr. Greg wrote:
>>> TSEM is designed, from a functional perspective, to be contained
>>> entirely in its own directory.
>>>
>>> The tsem.h header file defines the enumeration types, structure
>>> definitions and externally visiable functions that are referenced
>>> by all of the compilation units of the TSEM LSM implementation in
>>> that directory.
>> Extensive documentation notwithstanding, it's impossible to review
>> the data structures and constants without the code that goes along
>> with them.
> ...
>
> We may have missed it but I don't see anything in the kernel
> developers guide with respect best practices for introducing blocks of
> entire sub-system code.
>
> Recommendations welcome.

You've broken the code into progressive units. Include the data structure
definitions as required in those progressive units. Include the .h's as
they are needed by the .c's.

>
>>> The structure and enumeration types are extensively documented
>>> and are the recommended starting point for understanding TSEM
>>> implementation and functionality.
>>>
>>> Signed-off-by: Greg Wettstein <greg@...ellic.com>
>>> ---
>>>  security/tsem/tsem.h | 1516 ++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 1516 insertions(+)
>>>  create mode 100644 security/tsem/tsem.h
>>>
>>> diff --git a/security/tsem/tsem.h b/security/tsem/tsem.h
>>> new file mode 100644
>>> index 000000000000..03915f47529b
>>> --- /dev/null
>>> +++ b/security/tsem/tsem.h
>>> @@ -0,0 +1,1516 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +/*
>>> + * Copyright (C) 2023 Enjellic Systems Development, LLC
>>> + * Author: Dr. Greg Wettstein <greg@...ellic.com>
>>> + *
>>> + * This is the single include file that documents all of the externally
>>> + * visible types and functions that are used by TSEM.  This file is
>>> + * currently organized into four major sections in the following order;
>>> + *
>>> + * includes used by all compilation units
>>> + * CPP definitions
>>> + * enumeration types
>>> + * structure definitions
>>> + * function declarations
>>> + * inline encapsulation functions.
>>> + *
>>> + * Include files that are referenced by more than a single compilation
>>> + * should be included in this file.  Includes that are needed to
>>> + * satisfy compilation requirements for only a single file should be
>>> + * included in the file needing that include.
>>> + *
>>> + * Understanding the overall implementation and architecture of TSEM
>>> + * will be facilitated by reviewing the documentation in this file.
>>> + */
>>> +
>>> +#include <uapi/linux/in.h>
>>> +#include <uapi/linux/in6.h>
>>> +#include <linux/wait.h>
>>> +#include <linux/kref.h>
>>> +#include <linux/lsm_hooks.h>
>>> +#include <linux/capability.h>
>>> +#include <crypto/hash.h>
>>> +#include <crypto/hash_info.h>
>>> +#include <net/af_unix.h>
>>> +
>>> +/* The capability needed to manage TSEM. */
>>> +#define TSEM_CONTROL_CAPABILITY CAP_ML
>> Why would you do this? You gave the capability a name that even you
>> don't want to use.
> A simple placeholder that allows the capability used to control
> security modeling to be set in one place, and one place only, until
> the debate over the capability issue is settled.
>
> We never envisioned this define lasting beyond the initial reviews.

Intentional obfuscation is never going to make the review process
easier. I have enough trouble following clear and obvious code. I'm
going to search for CAP_ during a review. You just made that harder. 

> ...
>
> +
> +/**
> + * enum tsem_task_trust - Ordinal value describing task trust status.
> + * @TSEM_TASK_TRUSTED: This ordinal value indicates that the task has
> + *		       not executed a security event that has resulted
> + *		       in a security behavior not described by the
> + *		       security model the task is being governed by.
> + * @TSEM_TASK_UNTRUSTED: This ordinal value indicates that the task
> + *		          has requested the execution of a security event
> + *		          that resulted in a security behavior not
> + *		          permitted by the security model the task is
> + *		          being governed by.
> + * @TSEM_TASK_TRUST_PENDING: This ordinal value indicates that the setting
> + *			     of the task trust status is pending a response
> + *		             from an external TMA.
> + *
> + * This enumeration type is used to specify the three different trust
> + * states that a task can be in.  The trust status of a task is
> + * regulated by the trust_status member of struct tsem_task.  A task
> + * carrying the status of TSEM_TASK_TRUSTED means that it has
> + * not requested the execution of any security events that are
> + * inconsistent with the security model that the task is running in.
> + *
> + * If a task requests execution of a security event that is
> + * inconsistent with the security model it is operating in, and the
> + * domain is running in 'sealed' mode, the task trust status is set to
> + * TSEM_TASK_UNTRUSTED.  This value is 'sticky' in that it will be
> + * propagated to any child tasks that are spawned from an untrusted
> + * task.
> + *
> + * In the case of an externally modeled security domain/namespace, the
> + * task trust status cannot be determined until the modeling of the
> + * security event has been completed.  The tsem_export_event()
> + * function sets the trust status TSEM_TASK_TRUST_PENDING and then
> + * places the task into an interruptible sleep state.
> + *
> + * Only two events will cause the task to be removed from sleep state.
> + * Either the task is killed or a control message is written to the
> + * TSEM control file that specifies the trust status of the task.  See
> + * the description of the TSEM_CONTROL_TRUSTED and
> + * TSEM_CONTROL_UNTRUSTED enumeration types.
> + */
> +enum tsem_task_trust {
> +	TSEM_TASK_TRUSTED = 1,
> +	TSEM_TASK_UNTRUSTED = 2,
> +	TSEM_TASK_TRUST_PENDING = 4
>> What happened to 3?
> The enumerations represent bit values used in the 'trust_status'
> member of the tsem_task structure.

Someone can correct me if I'm wrong, but I think that's a misuse
of enum. 

>
> If a task thinks that it has a trust status value of three it would
> indicate the task needs massive doses of chlorpromazine.

If you're not using logical operations (&, |) why do you care if
they're "bit values"?

> ...
>
> + *
> + * Each task that is created in a non-root security modeling namespace
> + * increments the reference count maintained in the kref member of
> + * this structure in the tsem_task_alloc() function.  The
> + * tsem_task_free() function decrements this reference count.  When
> + * the reference count expires, ie. when the last task using the
> + * modeling namespace exits, an asynchronous workqueue request is
> + * dispatched to dispose of the context.  The work member of this
> + * structure is used to reference that workqueue request.
> + */
> +struct tsem_context {
> +	struct kref kref;
> +	struct work_struct work;
> +
> +	u64 id;
> +	bool sealed;
> +	bool use_current_ns;
> +
> +	enum tsem_action_type actions[TSEM_EVENT_CNT];
> +
> +	char *digestname;
> +	u8 zero_digest[HASH_MAX_DIGESTSIZE];
> +	struct crypto_shash *tfm;
> +
> +	unsigned int magazine_size;
> +	spinlock_t magazine_lock;
> +	unsigned long *magazine_index;
> +	struct tsem_work *ws;
> +	struct tsem_event **magazine;
> +
> +	struct tsem_model *model;
> +	struct tsem_external *external;
> +};
>> Odd use of whitespace in the structure definition.
> It is a carry over from our userspace coding style.  As a team we have
> found it helpful to visually segregate members of a structure that
> have a common and distinct purpose.
>
> FWIW, it would seem that include/linux/sched.h seems to embrace
> a similar concept for 'struct task_struct'.

I'm not formally objecting to it, but there's no obvious reason for it.

>
> [ ... rest of tsem.h trimmed ... ]
>
> As always,
> Dr. Greg
>
> The Quixote Project - Flailing at the Travails of Cybersecurity

Powered by blists - more mailing lists