On Sat, Dec 16, 2023 at 09:41:10AM -0800, Alexei Starovoitov wrote:
> On Sat, Dec 16, 2023 at 2:38 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> >
> > On Fri, Dec 15, 2023 at 10:08:08AM -0800, Alexei Starovoitov wrote:
> > > On Fri, Dec 15, 2023 at 6:15 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> > > >
> > > > On Fri, Dec 15, 2023 at 02:26:53PM +0100, Michael Weiß wrote:
> > > > > On 15.12.23 13:31, Christian Brauner wrote:
> > > > > > On Wed, Dec 13, 2023 at 03:38:13PM +0100, Michael Weiß wrote:
> > > > > >> devguard is a simple LSM to allow CAP_MKNOD in non-initial user
> > > > > >> namespace in cooperation of an attached cgroup device program. We
> > > > > >> just need to implement the security_inode_mknod() hook for this.
> > > > > >> In the hook, we check if the current task is guarded by a device
> > > > > >> cgroup using the lately introduced cgroup_bpf_current_enabled()
> > > > > >> helper. If so, we strip out SB_I_NODEV from the super block.
> > > > > >>
> > > > > >> Access decisions to those device nodes are then guarded by existing
> > > > > >> device cgroups mechanism.
> > > > > >>
> > > > > >> Signed-off-by: Michael Weiß <michael.weiss@xxxxxxxxxxxxxxxxxxx>
> > > > > >> ---
> > > > > >
> > > > > > I think you misunderstood me... My point was that I believe you don't
> > > > > > need an additional LSM at all and no additional LSM hook. But I might be
> > > > > > wrong. Only a POC would show.
> > > > >
> > > > > Yeah sorry, I got your point now.
> > > >
> > > > I think I might have had a misconception about how this works.
> > > > A bpf LSM program can't easily alter a kernel object such as struct
> > > > super_block I've been told.
> > >
> > > Right. bpf cannot change arbitrary kernel objects,
> > > but we can add a kfunc that will change a specific bit in a specific
> > > data structure.
> > > Adding a new lsm hook that does:
> > > rc = call_int_hook(sb_device_access, 0, sb);
> > > switch (rc) {
> > > case 0: do X
> > > case 1: do Y
> > >
> > > is the same thing, but uglier, since return code will be used
> > > to do this action.
> > > The 'do X' can be one kfunc
> > > and 'do Y' can be another.
> > > If later we find out that 'do X' is not a good idea we can remove
> > > that kfunc.
> >
> > The reason I moved the SB_I_MANAGED_DEVICES here is that I want a single
> > central place where that is done for any possible LSM that wants to
> > implement device management. So we don't have to go chasing where that
> > bit is set for each LSM. I also don't want to have LSMs raise bits in
> > sb->s_iflags directly as that's VFS property.
>
> a kfunc that sets a bit in sb->s_iflags will be the same central place.
For the BPF LSM. I'm talking the same place for al LSMs.
> It will be somewhere in the fs/ directory and vfs maintainers can do what they
> wish with it, including removal.
> For traditional LSM one would need to do an accurate code review to make
> sure that they don't mess with sb->s_iflags while for bpf_lsm it
> will be done automatically. That kfunc will be that only one central place.
I'm not generally opposed to kfuncs ofc but here it just seems a bit
pointless. What we want is to keep SB_I_{NODEV,MANAGED_DEVICES} confined
to alloc_super(). The only central place it's raised where we control
all locking and logic. So it doesn't even have to appear in any
security_*() hooks.
diff --git a/security/security.c b/security/security.c
index 088a79c35c26..bf440d15615d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1221,6 +1221,33 @@ int security_sb_alloc(struct super_block *sb)
return rc;
}
+/*
+ * security_sb_device_access() - Let LSMs handle device access
+ * @sb: filesystem superblock
+ *
+ * Let an LSM take over device access management for this superblock.
+ *
+ * Return: Returns 1 if LSMs handle device access, 0 if none does and -ERRNO on
+ * failure.
+ */
+int security_sb_device_access(struct super_block *sb)
+{
+ int thisrc;
+ int rc = LSM_RET_DEFAULT(sb_device_access);
+ struct security_hook_list *hp;
+
+ hlist_for_each_entry(hp, &security_hook_heads.sb_device_access, list) {
+ thisrc = hp->hook.sb_device_access(sb);
+ if (thisrc < 0)
+ return thisrc;
+ /* At least one LSM claimed device access management. */
+ if (thisrc == 1)
+ rc = 1;
+ }
+
+ return rc;
+}
+
/**
* security_sb_delete() - Release super_block LSM associated objects
* @sb: filesystem superblock
diff --git a/fs/super.c b/fs/super.c
index 076392396e72..2295c0f76e56 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -325,7 +325,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
{
struct super_block *s = kzalloc(sizeof(struct super_block), GFP_USER);
static const struct super_operations default_op;
- int i;
+ int err, i;
if (!s)
return NULL;
@@ -362,8 +362,16 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
}
s->s_bdi = &noop_backing_dev_info;
s->s_flags = flags;
- if (s->s_user_ns != &init_user_ns)
+
+ err = security_sb_device_access(s);
+ if (err < 0)
+ goto fail;
+
+ if (err)
+ s->s_iflags |= SB_I_MANAGED_DEVICES;
+ else if (s->s_user_ns != &init_user_ns)
s->s_iflags |= SB_I_NODEV;
+
INIT_HLIST_NODE(&s->s_instances);
INIT_HLIST_BL_HEAD(&s->s_roots);
mutex_init(&s->s_sync_lock);
