On 10/26/2023 3:12 PM, Paul Moore wrote:
On Thu, Oct 26, 2023 at 5:33 PM Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> wrote:
On 10/23/2023 8:52 PM, Paul Moore wrote:
On Oct 4, 2023 Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx>
...
---
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().
Thank you for the suggestion. After some testing, I discovered that
switching to RCU to pin the super block and using a security blob to
mark a pinned super block works. This approach do avoid many spinlock
operations. I'll incorporate these changes in the next version of the patch.
I probably wasn't as clear as I should have been, I was thinking of
doing away with the @pinned_sb global variable entirely, as well as
its associated lock problems and simply marking the initramfs/initrd
superblock when it was mounted. I will admit that I haven't fully
thought about all the implementation details, but I think you could
leverage the security_sb_mount() hook to set a flag in IPE's
superblock metadata when the initramfs was mounted.
--
paul-moore.com
I wasn't able to find a way to let LSM pin initramfs/initrd during mount
time. But I think we could replace the global variable with a flag
variable ipe_sb_state so we could use atomic operation to only mark one
drive as pinned without any lock. The code will be like:
static void pin_sb(const struct super_block *sb)
{
if (!sb)
return;
if (!test_and_set_bit_lock(IPE_SB_PINNED, &ipe_sb_state)) {
ipe_sb(sb)->pinned = true;
}
}
Would this sound better?
-Fan
