Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Mon, 23 Oct 2023 23:52:25 -0400
Message-ID: <c53599e9d278fc55be30e3bac9411328.paul@paul-moore.com>
From:   Paul Moore <paul@paul-moore.com>
To:     Fan Wu <wufan@linux.microsoft.com>, corbet@lwn.net,
        zohar@linux.ibm.com, jmorris@namei.org, serge@hallyn.com,
        tytso@mit.edu, ebiggers@kernel.org, axboe@kernel.dk,
        agk@redhat.com, snitzer@kernel.org, eparis@redhat.com
Cc:     linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-fscrypt@vger.kernel.org, linux-block@vger.kernel.org,
        dm-devel@redhat.com, audit@vger.kernel.org,
        roberto.sassu@huawei.com, linux-kernel@vger.kernel.org,
        Deven Bowers <deven.desai@linux.microsoft.com>,
        Fan Wu <wufan@linux.microsoft.com>
Subject: Re: [PATCH RFC v11 5/19] ipe: introduce 'boot_verified' as a trust  provider
References: <1696457386-3010-6-git-send-email-wufan@linux.microsoft.com>
In-Reply-To: <1696457386-3010-6-git-send-email-wufan@linux.microsoft.com>
Precedence: bulk

On Oct  4, 2023 Fan Wu <wufan@linux.microsoft.com> wrote:
> 
> IPE is designed to provide system level trust guarantees, this usually
> implies that trust starts from bootup with a hardware root of trust,
> which validates the bootloader. After this, the bootloader verifies the
> kernel and the initramfs.
> 
> As there's no currently supported integrity method for initramfs, and
> it's typically already verified by the bootloader, introduce a property
> that causes the first superblock to have an execution to be "pinned",
> which is typically initramfs.
> 
> When the "pinned" device is unmounted, it will be "unpinned" and
> `boot_verified` property will always evaluate to false afterward.
> 
> We use a pointer with a spin_lock to "pin" the device instead of rcu
> because rcu synchronization may sleep, which is not allowed when
> unmounting a device.
>
> Signed-off-by: Deven Bowers <deven.desai@linux.microsoft.com>
> Signed-off-by: Fan Wu <wufan@linux.microsoft.com>
> ---
> v2:
>   +No Changes
> 
> v3:
>   + Remove useless caching system
>   + Move ipe_load_properties to this match
>   + Minor changes from checkpatch --strict warnings
> 
> v4:
>   + Remove comments from headers that was missed previously.
>   + Grammatical corrections.
> 
> v5:
>   + No significant changes
> 
> v6:
>   + No changes
> 
> v7:
>   + Reword and refactor patch 04/12 to [09/16], based on changes in the underlying system.
>   + Add common audit function for boolean values
>   + Use common audit function as implementation.
> 
> v8:
>   + No changes
> 
> v9:
>   + No changes
> 
> v10:
>   + Replace struct file with struct super_block
> 
> v11:
>   + Fix code style issues
> ---
>  security/ipe/eval.c          | 72 +++++++++++++++++++++++++++++++++++-
>  security/ipe/eval.h          |  2 +
>  security/ipe/hooks.c         | 12 ++++++
>  security/ipe/hooks.h         |  2 +
>  security/ipe/ipe.c           |  1 +
>  security/ipe/policy.h        |  2 +
>  security/ipe/policy_parser.c | 35 +++++++++++++++++-
>  7 files changed, 124 insertions(+), 2 deletions(-)
> 
> diff --git a/security/ipe/eval.c b/security/ipe/eval.c
> index 8a8bcc5c7d7f..bdac4abc0ddb 100644
> --- a/security/ipe/eval.c
> +++ b/security/ipe/eval.c
> @@ -9,6 +9,7 @@
>  #include <linux/file.h>
>  #include <linux/sched.h>
>  #include <linux/rcupdate.h>
> +#include <linux/spinlock.h>
>  
>  #include "ipe.h"
>  #include "eval.h"
> @@ -16,6 +17,44 @@
>  
>  struct ipe_policy __rcu *ipe_active_policy;
>  
> +static const struct super_block *pinned_sb;
> +static DEFINE_SPINLOCK(pin_lock);
> +#define FILE_SUPERBLOCK(f) ((f)->f_path.mnt->mnt_sb)
> +
> +/**
> + * pin_sb - Pin the underlying superblock of @f, marking it as trusted.
> + * @sb: Supplies a super_block structure to be pinned.
> + */
> +static void pin_sb(const struct super_block *sb)
> +{
> +	if (!sb)
> +		return;
> +	spin_lock(&pin_lock);
> +	if (!pinned_sb)
> +		pinned_sb = sb;
> +	spin_unlock(&pin_lock);
> +}
> +
> +/**
> + * from_pinned - Determine whether @sb is the pinned super_block.
> + * @sb: Supplies a super_block to check against the pinned super_block.
> + *
> + * Return:
> + * * true	- @sb is the pinned super_block
> + * * false	- @sb is not the pinned super_block
> + */
> +static bool from_pinned(const struct super_block *sb)
> +{
> +	bool rv;
> +
> +	if (!sb)
> +		return false;
> +	spin_lock(&pin_lock);
> +	rv = !IS_ERR_OR_NULL(pinned_sb) && pinned_sb == sb;
> +	spin_unlock(&pin_lock);

It's okay for an initial version, but I still think you need to get
away from this spinlock in from_pinned() as quickly as possible.
Maybe I'm wrong, but this looks like a major source of lock contention.

I understand the issue around RCU and the potential for matching on
a reused buffer/address, but if you modified IPE to have its own LSM
security blob in super_block::security you could mark the superblock
when it was mounted and do a lockless lookup here in from_pinned().

> +	return rv;
> +}

--
paul-moore.com