Messages in this thread | | | Date | Thu, 10 Aug 2023 08:03:43 -0700 | Subject | Re: [PATCH 04/13] Add TSEM master header file. | From | Casey Schaufler <> |
| |
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@enjellic.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@enjellic.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
| |