* Re: [PATCH v8 0/5] Enable root to update the blacklist keyring
From: Jarkko Sakkinen @ 2022-02-17 19:58 UTC (permalink / raw)
To: Mickaël Salaün
Cc: David S . Miller, Eric Snowberg, Herbert Xu, James Morris,
Mimi Zohar, Serge E . Hallyn, Tyler Hicks, keyrings, linux-crypto,
linux-integrity, linux-kernel, linux-security-module,
Ahmad Fatoum, Andreas Rammhold, David Howells, David Woodhouse
In-Reply-To: <e4707df2-ecc2-0471-87fc-c54e774fe315@digikod.net>
On Mon, Jan 31, 2022 at 12:33:51PM +0100, Mickaël Salaün wrote:
>
> On 07/01/2022 13:14, Mickaël Salaün wrote:
> >
> > On 06/01/2022 20:16, Jarkko Sakkinen wrote:
> > > On Thu, Jan 06, 2022 at 09:12:22PM +0200, Jarkko Sakkinen wrote:
> > > > On Tue, Jan 04, 2022 at 04:56:36PM +0100, Mickaël Salaün wrote:
> > > > >
> > > > > On 21/12/2021 09:50, Jarkko Sakkinen wrote:
> > > > > > On Mon, Dec 13, 2021 at 04:30:29PM +0100, Mickaël Salaün wrote:
> > > > > > > Hi Jarkko,
> > > > > > >
> > > > > > > Since everyone seems OK with this and had plenty of
> > > > > > > time to complain, could
> > > > > > > you please take this patch series in your tree? It still applies on
> > > > > > > v5.16-rc5 and it is really important to us. Please
> > > > > > > let me know if you need
> > > > > > > something more.
> > > > > > >
> > > > > > > Regards,
> > > > > > > Mickaël
> > > > > >
> > > > > > I'm off-work up until end of the year, i.e. I will
> > > > > > address only important
> > > > > > bug fixes and v5.16 up until that.
> > > > > >
> > > > > > If any of the patches is yet missing my ack, feel free to
> > > > > >
> > > > > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > >
> > > > > Thanks Jarkko. Can you please take it into your tree?
> > > >
> > > > I can yes, as I need to anyway do a revised PR for v5.17, as one commit
> > > > in my first trial had a truncated fixes tag.
> > >
> > > Please check:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git
> > >
> > > /Jarkko
> >
> > Great, thanks!
>
> Hi Jarkko,
>
> I noticed your commits https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=3ec9c3a0531ac868422be3b12fc17310ed8c07dc
> are no more referenced in your tree. Is there an issue?
This must be some sort of mistake I've made. I'll re-apply the patches.
Sorry about this.
BR, Jarkko
^ permalink raw reply
* Re: [RFC PATCH 2/2] capability: use new capable_or functionality
From: Alexei Starovoitov @ 2022-02-17 17:29 UTC (permalink / raw)
To: Christian Göttsche
Cc: selinux, Jens Axboe, Hans Verkuil, Mauro Carvalho Chehab,
David S. Miller, Jakub Kicinski, Stefan Haberland, Jan Hoeppner,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Alexander Gordeev, Sven Schnelle, Alexander Viro, Serge Hallyn,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Zhen Lei, Arnd Bergmann, Laurent Pinchart, Julia Lawall,
Greg Kroah-Hartman, Jiri Slaby, Pavel Skripkin, Du Cheng,
Eric W. Biederman, Andrew Morton, Peter Zijlstra, Alexey Gladkov,
David Hildenbrand, Rolf Eike Beer, Christian Brauner,
Cyrill Gorcunov, Peter Collingbourne, Colin Cross,
Davidlohr Bueso, Xiaofeng Cao, Nikolay Aleksandrov,
Stefano Garzarella, Florian Fainelli, Ziyang Xuan,
Alexander Aring, Eric Dumazet, Alistair Delva, Bart Van Assche,
linux-block, LKML, linux-media, Network Development, linux-s390,
Linux-Fsdevel, LSM List, bpf
In-Reply-To: <20220217145003.78982-1-cgzones@googlemail.com>
On Thu, Feb 17, 2022 at 6:50 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Use the new added capable_or macro in appropriate cases, where a task
> is required to have any of two capabilities.
>
> Reorder CAP_SYS_ADMIN last.
>
> TODO: split into subsystem patches.
Yes. Please.
The bpf side picked the existing order because we were aware
of that selinux issue.
Looks like there is no good order that works for all.
So the new helper makes a lot of sense.
> Fixes: 94c4b4fd25e6 ("block: Check ADMIN before NICE for IOPRIO_CLASS_RT")
^ permalink raw reply
* [RFC PATCH 1/2] capability: add capable_or to test for multiple caps with exactly one audit message
From: Christian Göttsche @ 2022-02-17 14:49 UTC (permalink / raw)
To: selinux; +Cc: Serge Hallyn, linux-security-module, linux-kernel
In-Reply-To: <20220217145003.78982-1-cgzones@googlemail.com>
Add the interface `capable_or()` as an alternative to or multiple
`capable()` calls, like `capable_or(CAP_SYS_NICE, CAP_SYS_ADMIN)`
instead of `capable(CAP_SYS_NICE) || capable(CAP_SYS_ADMIN)`.
`capable_or()` will in particular generate exactly one audit message,
either for the left most capability in effect or, if the task has none,
the first one.
This is especially helpful with regard to SELinux, where each audit
message about a not allowed capability will create an avc denial.
Using this function with the least invasive capability as left most
argument (e.g. CAP_SYS_NICE before CAP_SYS_ADMIN) enables policy writers
to only allow the least invasive one and SELinux domains pass this check
with only capability:sys_nice or capability:sys_admin allowed without
any avc denial message.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
include/linux/capability.h | 6 +++++
kernel/capability.c | 48 ++++++++++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+)
diff --git a/include/linux/capability.h b/include/linux/capability.h
index 65efb74c3585..5c55687a9a05 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -207,6 +207,8 @@ extern bool has_ns_capability(struct task_struct *t,
extern bool has_capability_noaudit(struct task_struct *t, int cap);
extern bool has_ns_capability_noaudit(struct task_struct *t,
struct user_namespace *ns, int cap);
+#define capable_or(...) _capable_or_impl((sizeof((int[]){__VA_ARGS__}) / sizeof(int)), __VA_ARGS__)
+extern bool _capable_or_impl(int count, ...);
extern bool capable(int cap);
extern bool ns_capable(struct user_namespace *ns, int cap);
extern bool ns_capable_noaudit(struct user_namespace *ns, int cap);
@@ -230,6 +232,10 @@ static inline bool has_ns_capability_noaudit(struct task_struct *t,
{
return true;
}
+static inline bool capable_or(int first_cap, ...)
+{
+ return true;
+}
static inline bool capable(int cap)
{
return true;
diff --git a/kernel/capability.c b/kernel/capability.c
index 46a361dde042..5b73a58b914e 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -434,6 +434,54 @@ bool ns_capable_setid(struct user_namespace *ns, int cap)
}
EXPORT_SYMBOL(ns_capable_setid);
+/**
+ * _capable_or - Determine if the current task has one of multiple superior capabilities in effect
+ * @cap: The capabilities to be tested for
+ *
+ * Return true if the current task has at least one of the given superior capabilities currently
+ * available for use, false if not.
+ *
+ * In contrast to or'ing capable() this call will create exactly one audit message, either for the
+ * left most capability in effect or (if the task has none of the tested capabilities) the first
+ * capabilit in the test list.
+ *
+ * The capabilities should be ordered from least to most invasive, i.e. CAP_SYS_ADMIN last.
+ *
+ * This sets PF_SUPERPRIV on the task if the capability is available on the
+ * assumption that it's about to be used.
+ */
+bool _capable_or_impl(int count, ...)
+{
+ va_list args;
+ const struct cred *cred = current_cred();
+ int cap, first_cap, i;
+
+ BUG_ON(count < 1);
+ BUG_ON(count > CAP_LAST_CAP);
+
+ va_start(args, count);
+
+ for (i = 0; i < count; i++) {
+ int ret;
+
+ cap = va_arg(args, int);
+ if (i == 0)
+ first_cap = cap;
+
+ ret = security_capable(cred, &init_user_ns, cap, CAP_OPT_NOAUDIT);
+ if (ret == 0)
+ goto out;
+ }
+
+ /* if none of the capabilities was allowed, create an audit event for the first one */
+ cap = first_cap;
+
+out:
+ va_end(args);
+ return ns_capable(&init_user_ns, cap);
+}
+EXPORT_SYMBOL(_capable_or_impl);
+
/**
* capable - Determine if the current task has a superior capability in effect
* @cap: The capability to be tested for
--
2.35.1
^ permalink raw reply related
* [RFC PATCH 2/2] capability: use new capable_or functionality
From: Christian Göttsche @ 2022-02-17 14:49 UTC (permalink / raw)
To: selinux
Cc: Jens Axboe, Hans Verkuil, Mauro Carvalho Chehab, David S. Miller,
Jakub Kicinski, Stefan Haberland, Jan Hoeppner, Heiko Carstens,
Vasily Gorbik, Christian Borntraeger, Alexander Gordeev,
Sven Schnelle, Alexander Viro, Serge Hallyn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Zhen Lei, Arnd Bergmann,
Laurent Pinchart, Julia Lawall, Greg Kroah-Hartman, Jiri Slaby,
Pavel Skripkin, Du Cheng, Eric W. Biederman, Andrew Morton,
Peter Zijlstra, Alexey Gladkov, David Hildenbrand, Rolf Eike Beer,
Christian Brauner, Cyrill Gorcunov, Peter Collingbourne,
Colin Cross, Davidlohr Bueso, Xiaofeng Cao, Nikolay Aleksandrov,
Stefano Garzarella, Florian Fainelli, Ziyang Xuan,
Alexander Aring, Eric Dumazet, Alistair Delva, Bart Van Assche,
linux-block, linux-kernel, linux-media, netdev, linux-s390,
linux-fsdevel, linux-security-module, bpf
Use the new added capable_or macro in appropriate cases, where a task
is required to have any of two capabilities.
Reorder CAP_SYS_ADMIN last.
TODO: split into subsystem patches.
Fixes: 94c4b4fd25e6 ("block: Check ADMIN before NICE for IOPRIO_CLASS_RT")
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
block/ioprio.c | 9 +--------
drivers/media/common/saa7146/saa7146_video.c | 2 +-
drivers/media/pci/bt8xx/bttv-driver.c | 3 +--
drivers/media/pci/saa7134/saa7134-video.c | 3 +--
drivers/media/platform/fsl-viu.c | 2 +-
drivers/media/test-drivers/vivid/vivid-vid-cap.c | 2 +-
drivers/net/caif/caif_serial.c | 2 +-
drivers/s390/block/dasd_eckd.c | 2 +-
fs/pipe.c | 2 +-
include/linux/capability.h | 4 ++--
kernel/bpf/syscall.c | 2 +-
kernel/fork.c | 2 +-
kernel/sys.c | 2 +-
net/caif/caif_socket.c | 2 +-
net/unix/scm.c | 2 +-
15 files changed, 16 insertions(+), 25 deletions(-)
diff --git a/block/ioprio.c b/block/ioprio.c
index 2fe068fcaad5..52d5da286323 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -37,14 +37,7 @@ int ioprio_check_cap(int ioprio)
switch (class) {
case IOPRIO_CLASS_RT:
- /*
- * Originally this only checked for CAP_SYS_ADMIN,
- * which was implicitly allowed for pid 0 by security
- * modules such as SELinux. Make sure we check
- * CAP_SYS_ADMIN first to avoid a denial/avc for
- * possibly missing CAP_SYS_NICE permission.
- */
- if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_NICE))
+ if (!capable_or(CAP_SYS_NICE, CAP_SYS_ADMIN))
return -EPERM;
fallthrough;
/* rt has prio field too */
diff --git a/drivers/media/common/saa7146/saa7146_video.c b/drivers/media/common/saa7146/saa7146_video.c
index 66215d9106a4..5eabc2e77cc2 100644
--- a/drivers/media/common/saa7146/saa7146_video.c
+++ b/drivers/media/common/saa7146/saa7146_video.c
@@ -470,7 +470,7 @@ static int vidioc_s_fbuf(struct file *file, void *fh, const struct v4l2_framebuf
DEB_EE("VIDIOC_S_FBUF\n");
- if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_RAWIO))
+ if (!capable_or(CAP_SYS_RAWIO, CAP_SYS_ADMIN))
return -EPERM;
/* check args */
diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
index 8cc9bec43688..c2437ff07246 100644
--- a/drivers/media/pci/bt8xx/bttv-driver.c
+++ b/drivers/media/pci/bt8xx/bttv-driver.c
@@ -2569,8 +2569,7 @@ static int bttv_s_fbuf(struct file *file, void *f,
const struct bttv_format *fmt;
int retval;
- if (!capable(CAP_SYS_ADMIN) &&
- !capable(CAP_SYS_RAWIO))
+ if (!capable_or(CAP_SYS_RAWIO, CAP_SYS_ADMIN))
return -EPERM;
/* check args */
diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
index 374c8e1087de..356b77c16f87 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -1803,8 +1803,7 @@ static int saa7134_s_fbuf(struct file *file, void *f,
struct saa7134_dev *dev = video_drvdata(file);
struct saa7134_format *fmt;
- if (!capable(CAP_SYS_ADMIN) &&
- !capable(CAP_SYS_RAWIO))
+ if (!capable_or(CAP_SYS_RAWIO, CAP_SYS_ADMIN))
return -EPERM;
/* check args */
diff --git a/drivers/media/platform/fsl-viu.c b/drivers/media/platform/fsl-viu.c
index a4bfa70b49b2..925c34c2b1b3 100644
--- a/drivers/media/platform/fsl-viu.c
+++ b/drivers/media/platform/fsl-viu.c
@@ -803,7 +803,7 @@ static int vidioc_s_fbuf(struct file *file, void *priv, const struct v4l2_frameb
const struct v4l2_framebuffer *fb = arg;
struct viu_fmt *fmt;
- if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_RAWIO))
+ if (!capable_or(CAP_SYS_RAWIO, CAP_SYS_ADMIN))
return -EPERM;
/* check args */
diff --git a/drivers/media/test-drivers/vivid/vivid-vid-cap.c b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
index b9caa4b26209..a0cfcf6c22c4 100644
--- a/drivers/media/test-drivers/vivid/vivid-vid-cap.c
+++ b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
@@ -1253,7 +1253,7 @@ int vivid_vid_cap_s_fbuf(struct file *file, void *fh,
if (dev->multiplanar)
return -ENOTTY;
- if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_RAWIO))
+ if (!capable_or(CAP_SYS_RAWIO, CAP_SYS_ADMIN))
return -EPERM;
if (dev->overlay_cap_owner)
diff --git a/drivers/net/caif/caif_serial.c b/drivers/net/caif/caif_serial.c
index 2a7af611d43a..245c30c469c2 100644
--- a/drivers/net/caif/caif_serial.c
+++ b/drivers/net/caif/caif_serial.c
@@ -326,7 +326,7 @@ static int ldisc_open(struct tty_struct *tty)
/* No write no play */
if (tty->ops->write == NULL)
return -EOPNOTSUPP;
- if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_TTY_CONFIG))
+ if (!capable_or(CAP_SYS_TTY_CONFIG, CAP_SYS_ADMIN))
return -EPERM;
/* release devices to avoid name collision */
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 8410a25a65c1..9b5d22dd3e7b 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -5319,7 +5319,7 @@ static int dasd_symm_io(struct dasd_device *device, void __user *argp)
char psf0, psf1;
int rc;
- if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_RAWIO))
+ if (!capable_or(CAP_SYS_RAWIO, CAP_SYS_ADMIN))
return -EACCES;
psf0 = psf1 = 0;
diff --git a/fs/pipe.c b/fs/pipe.c
index cc28623a67b6..47dc9b59b7a5 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -775,7 +775,7 @@ bool too_many_pipe_buffers_hard(unsigned long user_bufs)
bool pipe_is_unprivileged_user(void)
{
- return !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN);
+ return !capable_or(CAP_SYS_RESOURCE, CAP_SYS_ADMIN);
}
struct pipe_inode_info *alloc_pipe_info(void)
diff --git a/include/linux/capability.h b/include/linux/capability.h
index 5c55687a9a05..5ed55b73cb62 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -262,12 +262,12 @@ extern bool file_ns_capable(const struct file *file, struct user_namespace *ns,
extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns);
static inline bool perfmon_capable(void)
{
- return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
+ return capable_or(CAP_PERFMON, CAP_SYS_ADMIN);
}
static inline bool bpf_capable(void)
{
- return capable(CAP_BPF) || capable(CAP_SYS_ADMIN);
+ return capable_or(CAP_BPF, CAP_SYS_ADMIN);
}
static inline bool checkpoint_restore_ns_capable(struct user_namespace *ns)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index fa4505f9b611..108dd09f978a 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2243,7 +2243,7 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
!bpf_capable())
return -EPERM;
- if (is_net_admin_prog_type(type) && !capable(CAP_NET_ADMIN) && !capable(CAP_SYS_ADMIN))
+ if (is_net_admin_prog_type(type) && !capable_or(CAP_NET_ADMIN, CAP_SYS_ADMIN))
return -EPERM;
if (is_perfmon_prog_type(type) && !perfmon_capable())
return -EPERM;
diff --git a/kernel/fork.c b/kernel/fork.c
index d75a528f7b21..067702f2eb15 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2024,7 +2024,7 @@ static __latent_entropy struct task_struct *copy_process(
retval = -EAGAIN;
if (is_ucounts_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
if (p->real_cred->user != INIT_USER &&
- !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
+ !capable_or(CAP_SYS_RESOURCE, CAP_SYS_ADMIN))
goto bad_fork_free;
}
current->flags &= ~PF_NPROC_EXCEEDED;
diff --git a/kernel/sys.c b/kernel/sys.c
index ecc4cf019242..9df6c5e77620 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -481,7 +481,7 @@ static int set_user(struct cred *new)
*/
if (is_ucounts_overlimit(new->ucounts, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC)) &&
new_user != INIT_USER &&
- !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
+ !capable_or(CAP_SYS_RESOURCE, CAP_SYS_ADMIN))
current->flags |= PF_NPROC_EXCEEDED;
else
current->flags &= ~PF_NPROC_EXCEEDED;
diff --git a/net/caif/caif_socket.c b/net/caif/caif_socket.c
index 2b8892d502f7..60498148126c 100644
--- a/net/caif/caif_socket.c
+++ b/net/caif/caif_socket.c
@@ -1036,7 +1036,7 @@ static int caif_create(struct net *net, struct socket *sock, int protocol,
.usersize = sizeof_field(struct caifsock, conn_req.param)
};
- if (!capable(CAP_SYS_ADMIN) && !capable(CAP_NET_ADMIN))
+ if (!capable_or(CAP_NET_ADMIN, CAP_SYS_ADMIN))
return -EPERM;
/*
* The sock->type specifies the socket type to use.
diff --git a/net/unix/scm.c b/net/unix/scm.c
index aa27a02478dc..821be80e6c85 100644
--- a/net/unix/scm.c
+++ b/net/unix/scm.c
@@ -99,7 +99,7 @@ static inline bool too_many_unix_fds(struct task_struct *p)
struct user_struct *user = current_user();
if (unlikely(user->unix_inflight > task_rlimit(p, RLIMIT_NOFILE)))
- return !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN);
+ return !capable_or(CAP_SYS_RESOURCE, CAP_SYS_ADMIN);
return false;
}
--
2.35.1
^ permalink raw reply related
* Re: [PATCH v10 08/27] ima: Move measurement list related variables into ima_namespace
From: Mimi Zohar @ 2022-02-17 14:46 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris
In-Reply-To: <20220201203735.164593-9-stefanb@linux.ibm.com>
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
> Move measurement list related variables into the ima_namespace. This way
> a front-end like securityfs can show the measurement list inside an IMA
> namespace.
Also, in order for kexec to allocate memory for the existing
measurement list, the measurement list memory size is stored in the
binary_runtime_size variable. To avoid special-casing init_ima_ns, as
much as possible, move it into the ima_namespace.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
^ permalink raw reply
* Re: [PATCH v10 10/27] ima: Move IMA securityfs files into ima_namespace or onto stack
From: Mimi Zohar @ 2022-02-17 14:44 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris, Christian Brauner
In-Reply-To: <20220201203735.164593-11-stefanb@linux.ibm.com>
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
> Earlier we simplified how dentry creation and deletion is manged in
> securityfs. This allows us to move IMA securityfs files from global
> variables directly into ima_fs_ns_init() itself. We can now rely on
> those dentries to be cleaned up when the securityfs instance is cleaned
> when the last reference to it is dropped.
>
> Things are slightly different for the initial IMA namespace. In contrast
> to non-initial IMA namespaces it has pinning logic binding the lifetime
> of the securityfs superblock to created dentries. We need to keep this
> behavior to not regress userspace. Since IMA never removes most of the
> securityfs files the initial securityfs instance stays pinned. This also
> means even for the initial IMA namespace we don't need to keep
> references to these dentries anywhere.
>
> The ima_policy file is the exception since IMA can end up removing it
> on systems that don't allow reading or extending the IMA custom policy.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Christian Brauner <brauner@kernel.org>
Really nicely worded patch description. Thanks!
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
^ permalink raw reply
* Re: [PATCH v10 09/27] ima: Move some IMA policy and filesystem related variables into ima_namespace
From: Mimi Zohar @ 2022-02-17 14:44 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris, Christian Brauner
In-Reply-To: <20220201203735.164593-10-stefanb@linux.ibm.com>
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
> Move the variables ima_write_mutex, ima_fs_flag, and valid_policy, which
> are related to updating the IMA policy, into the ima_namespace. This way
> each IMA namespace can set these variables independently in its instance
> of securityfs.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Christian Brauner <brauner@kernel.org>
Thanks,
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
^ permalink raw reply
* [PATCH] selinux: log anon inode class name
From: Christian Göttsche @ 2022-02-17 14:34 UTC (permalink / raw)
To: selinux
Cc: James Morris, Serge E. Hallyn, Paul Moore, Stephen Smalley,
Eric Paris, Richard Guy Briggs, Ondrej Mosnacek, linux-kernel,
linux-security-module
Log the anonymous inode class name in the security hook
inode_init_security_anon. This name is the key for name based type
transitions on the anon_inode security class on creation. Example:
type=AVC msg=audit(02/16/22 22:02:50.585:216) : avc: granted { create } for pid=2136 comm=mariadbd anonclass="[io_uring]" dev="anon_inodefs" ino=6871 scontext=system_u:system_r:mysqld_t:s0 tcontext=system_u:system_r:mysqld_iouring_t:s0 tclass=anon_inode
Add a new LSM audit data type holding the inode and the class name.
Also warn if the security hook gets called with no name set; currently
the only caller fs/anon_inodes.c:anon_inode_make_secure_inode() passes
one.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
include/linux/lsm_audit.h | 5 +++++
security/lsm_audit.c | 21 +++++++++++++++++++++
security/selinux/hooks.c | 7 +++++--
3 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
index 17d02eda9538..8135a88d6d82 100644
--- a/include/linux/lsm_audit.h
+++ b/include/linux/lsm_audit.h
@@ -76,6 +76,7 @@ struct common_audit_data {
#define LSM_AUDIT_DATA_IBENDPORT 14
#define LSM_AUDIT_DATA_LOCKDOWN 15
#define LSM_AUDIT_DATA_NOTIFICATION 16
+#define LSM_AUDIT_DATA_ANONINODE 17
union {
struct path path;
struct dentry *dentry;
@@ -96,6 +97,10 @@ struct common_audit_data {
struct lsm_ibpkey_audit *ibpkey;
struct lsm_ibendport_audit *ibendport;
int reason;
+ struct {
+ struct inode *inode;
+ const char *anonclass;
+ } anoninode_struct;
} u;
/* this union contains LSM specific data */
union {
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 1897cbf6fc69..5545fed35539 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -433,6 +433,27 @@ static void dump_common_audit_data(struct audit_buffer *ab,
audit_log_format(ab, " lockdown_reason=\"%s\"",
lockdown_reasons[a->u.reason]);
break;
+ case LSM_AUDIT_DATA_ANONINODE: {
+ struct dentry *dentry;
+ struct inode *inode;
+
+ rcu_read_lock();
+ inode = a->u.anoninode_struct.inode;
+ dentry = d_find_alias_rcu(inode);
+ if (dentry) {
+ audit_log_format(ab, " name=");
+ spin_lock(&dentry->d_lock);
+ audit_log_untrustedstring(ab, dentry->d_name.name);
+ spin_unlock(&dentry->d_lock);
+ }
+ audit_log_format(ab, " anonclass=");
+ audit_log_untrustedstring(ab, a->u.anoninode_struct.anonclass);
+ audit_log_format(ab, " dev=");
+ audit_log_untrustedstring(ab, inode->i_sb->s_id);
+ audit_log_format(ab, " ino=%lu", inode->i_ino);
+ rcu_read_unlock();
+ break;
+ }
} /* switch (a->type) */
}
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index dafabb4dcc64..19c831d94d9b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2932,6 +2932,8 @@ static int selinux_inode_init_security_anon(struct inode *inode,
if (unlikely(!selinux_initialized(&selinux_state)))
return 0;
+ WARN_ON(!name);
+
isec = selinux_inode(inode);
/*
@@ -2965,8 +2967,9 @@ static int selinux_inode_init_security_anon(struct inode *inode,
* allowed to actually create this type of anonymous inode.
*/
- ad.type = LSM_AUDIT_DATA_INODE;
- ad.u.inode = inode;
+ ad.type = LSM_AUDIT_DATA_ANONINODE;
+ ad.u.anoninode_struct.inode = inode;
+ ad.u.anoninode_struct.anonclass = name ? (const char *)name->name : "unknown(null)";
return avc_has_perm(&selinux_state,
tsec->sid,
--
2.35.1
^ permalink raw reply related
* Re: [RFC PATCH] mm: create security context for memfd_secret inodes
From: Christian Göttsche @ 2022-02-17 14:24 UTC (permalink / raw)
To: Paul Moore
Cc: SElinux list, James Morris, Serge E. Hallyn,
linux-security-module, Stephen Smalley, Eric Paris, Andrew Morton,
linux-mm, linux-kernel
In-Reply-To: <CAHC9VhSdGeZ9x-0Hvk9mE=YMXbpk-tC5Ek+uGFGq5U+51qjChw@mail.gmail.com>
On Thu, 27 Jan 2022 at 00:01, Paul Moore <paul@paul-moore.com> wrote:
>
> On Tue, Jan 25, 2022 at 9:33 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > Create a security context for the inodes created by memfd_secret(2) via
> > the LSM hook inode_init_security_anon to allow a fine grained control.
> > As secret memory areas can affect hibernation and have a global shared
> > limit access control might be desirable.
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > ---
> > An alternative way of checking memfd_secret(2) is to create a new LSM
> > hook and e.g. for SELinux check via a new process class permission.
> > ---
> > mm/secretmem.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
>
> This seems reasonable to me, and I like the idea of labeling the anon
> inode as opposed to creating a new set of LSM hooks. If we want to
> apply access control policy to the memfd_secret() fds we are going to
> need to attach some sort of LSM state to the inode, we might as well
> use the mechanism we already have instead of inventing another one.
Any further comments (on design or implementation)?
Should I resend a non-rfc?
One naming question:
Should the anonymous inode class be named "[secretmem]", like
"[userfaultfd]", or "[secret_mem]" similar to "[io_uring]"?
> > diff --git a/mm/secretmem.c b/mm/secretmem.c
> > index 22b310adb53d..b61cd2f661bc 100644
> > --- a/mm/secretmem.c
> > +++ b/mm/secretmem.c
> > @@ -164,11 +164,20 @@ static struct file *secretmem_file_create(unsigned long flags)
> > {
> > struct file *file = ERR_PTR(-ENOMEM);
> > struct inode *inode;
> > + const char *anon_name = "[secretmem]";
> > + const struct qstr qname = QSTR_INIT(anon_name, strlen(anon_name));
> > + int err;
> >
> > inode = alloc_anon_inode(secretmem_mnt->mnt_sb);
> > if (IS_ERR(inode))
> > return ERR_CAST(inode);
> >
> > + err = security_inode_init_security_anon(inode, &qname, NULL);
> > + if (err) {
> > + file = ERR_PTR(err);
> > + goto err_free_inode;
> > + }
> > +
> > file = alloc_file_pseudo(inode, secretmem_mnt, "secretmem",
> > O_RDWR, &secretmem_fops);
> > if (IS_ERR(file))
> > --
> > 2.34.1
>
> --
> paul-moore.com
^ permalink raw reply
* [PATCH] security: declare member holding string literal const
From: Christian Göttsche @ 2022-02-17 14:18 UTC (permalink / raw)
To: selinux
Cc: James Morris, Serge E. Hallyn, Nathan Chancellor,
Nick Desaulniers, Paul Moore, Casey Schaufler, Xin Long,
David S. Miller, Ondrej Mosnacek, Mickaël Salaün,
Todd Kjos, Olga Kornievskaia, linux-kernel, linux-security-module,
llvm
The struct security_hook_list member lsm is assigned in
security_add_hooks() with string literals passed from the individual
security modules. Declare the function parameter and the struct member
const to signal their immutability.
Reported by Clang [-Wwrite-strings]:
security/selinux/hooks.c:7388:63: error: passing 'const char [8]' to parameter of type 'char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), selinux);
^~~~~~~~~
./include/linux/lsm_hooks.h:1629:11: note: passing argument to parameter 'lsm' here
char *lsm);
^
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
include/linux/lsm_hooks.h | 4 ++--
security/security.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 419b5febc3ca..47cdf3fbecef 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1595,7 +1595,7 @@ struct security_hook_list {
struct hlist_node list;
struct hlist_head *head;
union security_list_options hook;
- char *lsm;
+ const char *lsm;
} __randomize_layout;
/*
@@ -1630,7 +1630,7 @@ extern struct security_hook_heads security_hook_heads;
extern char *lsm_names;
extern void security_add_hooks(struct security_hook_list *hooks, int count,
- char *lsm);
+ const char *lsm);
#define LSM_FLAG_LEGACY_MAJOR BIT(0)
#define LSM_FLAG_EXCLUSIVE BIT(1)
diff --git a/security/security.c b/security/security.c
index 9663ffcca4b0..a48eb3badfdd 100644
--- a/security/security.c
+++ b/security/security.c
@@ -478,7 +478,7 @@ static int lsm_append(const char *new, char **result)
* Each LSM has to register its hooks with the infrastructure.
*/
void __init security_add_hooks(struct security_hook_list *hooks, int count,
- char *lsm)
+ const char *lsm)
{
int i;
--
2.35.1
^ permalink raw reply related
* Re: [PATCH net v3 2/2] security: implement sctp_assoc_established hook in selinux
From: Ondrej Mosnacek @ 2022-02-17 13:41 UTC (permalink / raw)
To: Paul Moore
Cc: Xin Long, Marcelo Ricardo Leitner, Jakub Kicinski, netdev,
David Miller, SElinux list, Richard Haines, Vlad Yasevich,
Neil Horman, open list:SCTP PROTOCOL, LSM List, LKML,
Prashanth Prahlad
In-Reply-To: <CAHC9VhSHxk0MUR1krpmbot6iG-vqH48sRgKOnJQ0LsFTs6Jvqg@mail.gmail.com>
On Tue, Feb 15, 2022 at 9:03 PM Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Feb 14, 2022 at 11:13 PM Xin Long <lucien.xin@gmail.com> wrote:
> > Looks okay to me.
> >
> > The difference from the old one is that: with
> > selinux_sctp_process_new_assoc() called in
> > selinux_sctp_assoc_established(), the client sksec->peer_sid is using
> > the first asoc's peer_secid, instead of the latest asoc's peer_secid.
> > And not sure if it will cause any problems when doing the extra check
> > sksec->peer_sid != asoc->peer_secid for the latest asoc and *returns
> > err*. But I don't know about selinux, I guess there must be a reason
> > from selinux side.
>
> Generally speaking we don't want to change any SELinux socket labels
> once it has been created. While the peer_sid is a bit different,
> changing it after userspace has access to the socket could be
> problematic. In the case where the peer_sid differs between the two
> we have a permission check which allows policy to control this
> behavior which seems like the best option at this point.
I think that maybe Xin was referring to the fact that on error return
from the hook the return code information is lost and the assoc is
just silently dropped (but I may have misunderstood). In case of a
denial (avc_has_perm() returning -EACCESS) this isn't much of a
problem, because the denial is logged in the audit log, so there is a
way to figure out why opening the association failed. In case of other
errors we could indeed do better and either log an SELINUX_ERR audit
event or at least pr_err() into the console, but there are likely
several other existing cases like this, so it would be best to do this
cleanup independently in another patch (if anyone feels up to the
task...).
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH net v3 2/2] security: implement sctp_assoc_established hook in selinux
From: Ondrej Mosnacek @ 2022-02-17 13:32 UTC (permalink / raw)
To: Paul Moore
Cc: network dev, David S. Miller, Jakub Kicinski, SElinux list,
Xin Long, Richard Haines, Vlad Yasevich, Neil Horman,
Marcelo Ricardo Leitner, linux-sctp @ vger . kernel . org,
Linux Security Module list, Linux kernel mailing list,
Prashanth Prahlad
In-Reply-To: <CAHC9VhT90617FoqQJBCrDQ8gceVVA6a1h74h6T4ZOwNk6RVB3g@mail.gmail.com>
On Mon, Feb 14, 2022 at 11:14 PM Paul Moore <paul@paul-moore.com> wrote:
> On Sat, Feb 12, 2022 at 12:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Do this by extracting the peer labeling per-association logic from
> > selinux_sctp_assoc_request() into a new helper
> > selinux_sctp_process_new_assoc() and use this helper in both
> > selinux_sctp_assoc_request() and selinux_sctp_assoc_established(). This
> > ensures that the peer labeling behavior as documented in
> > Documentation/security/SCTP.rst is applied both on the client and server
> > side:
> > """
> > An SCTP socket will only have one peer label assigned to it. This will be
> > assigned during the establishment of the first association. Any further
> > associations on this socket will have their packet peer label compared to
> > the sockets peer label, and only if they are different will the
> > ``association`` permission be validated. This is validated by checking the
> > socket peer sid against the received packets peer sid to determine whether
> > the association should be allowed or denied.
> > """
> >
> > At the same time, it also ensures that the peer label of the association
> > is set to the correct value, such that if it is peeled off into a new
> > socket, the socket's peer label will then be set to the association's
> > peer label, same as it already works on the server side.
> >
> > While selinux_inet_conn_established() (which we are replacing by
> > selinux_sctp_assoc_established() for SCTP) only deals with assigning a
> > peer label to the connection (socket), in case of SCTP we need to also
> > copy the (local) socket label to the association, so that
> > selinux_sctp_sk_clone() can then pick it up for the new socket in case
> > of SCTP peeloff.
> >
> > Careful readers will notice that the selinux_sctp_process_new_assoc()
> > helper also includes the "IPv4 packet received over an IPv6 socket"
> > check, even though it hadn't been in selinux_sctp_assoc_request()
> > before. While such check is not necessary in
> > selinux_inet_conn_request() (because struct request_sock's family field
> > is already set according to the skb's family), here it is needed, as we
> > don't have request_sock and we take the initial family from the socket.
> > In selinux_sctp_assoc_established() it is similarly needed as well (and
> > also selinux_inet_conn_established() already has it).
> >
> > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> > Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
> > Based-on-patch-by: Xin Long <lucien.xin@gmail.com>
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> > security/selinux/hooks.c | 90 +++++++++++++++++++++++++++++-----------
> > 1 file changed, 66 insertions(+), 24 deletions(-)
>
> This patch, and patch 1/2, look good to me; I'm assuming this resolves
> all of the known SELinux/SCTP problems identified before the new year?
No, not really. There is still the inconsistency that peeloff sockets
go through the socket_[post_]create hooks and then the label computed
is overwritten by the sctp_sk_clone hook. But it's a different issue
unrelated to this one. I'm still in the process of cooking up the
patches and figuring out the consequences (other LSMs would be
affected by the change, too, so it is tricky...).
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH v10 06/27] ima: Move arch_policy_entry into ima_namespace
From: Stefan Berger @ 2022-02-16 21:19 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris, Christian Brauner
In-Reply-To: <c4170de11d64d5927a8e2a2e0f7e8a6e69c6a558.camel@linux.ibm.com>
On 2/16/22 15:56, Mimi Zohar wrote:
> On Wed, 2022-02-16 at 15:48 -0500, Stefan Berger wrote:
>> On 2/16/22 11:39, Mimi Zohar wrote:
>>> On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote
>>>
>>> Let's update the patch description providing a bit more background
>>> info:
>>>
>>> The archictecture specific policy rules, currently defined for EFI and
>>> powerpc, require the kexec kernel image and kernel modules to be
>>> validly signed and measured, based on the system's secure boot and/or
>>> trusted boot mode and the IMA_ARCH_POLICY Kconfig option being enabled.
>>>
>>>> Move the arch_policy_entry pointer into ima_namespace.
>>> Perhaps include something about namespaces being allowed or not allowed
>>> to kexec a new kernel or load kernel modules.
>> Namespaces are not allowed to kexec but special-casing the init_ima_ns
>> in the code to handle namespaces differently makes it much harder to
>> read the code. I would avoid special-casing init_ima_ns as much as
>> possible and therefore I have moved the arch_policy_entry into the
>> ima_namespace.
> Please include this in the patch description, but re-write the last
> line in the 3rd person, like:
>
> To avoid special-casing init_ima_ns, as much as possible, move the
> arch_policy_entry into the ima_namespace.
I took the paragraph on the background as well as this sentence.
>
> thanks,
>
> Mimi
>
>
^ permalink raw reply
* Re: [PATCH v10 06/27] ima: Move arch_policy_entry into ima_namespace
From: Mimi Zohar @ 2022-02-16 20:56 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris, Christian Brauner
In-Reply-To: <c350ccf1-f968-8b01-2f0d-015cabf39781@linux.ibm.com>
On Wed, 2022-02-16 at 15:48 -0500, Stefan Berger wrote:
> On 2/16/22 11:39, Mimi Zohar wrote:
> > On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote
> >
> > Let's update the patch description providing a bit more background
> > info:
> >
> > The archictecture specific policy rules, currently defined for EFI and
> > powerpc, require the kexec kernel image and kernel modules to be
> > validly signed and measured, based on the system's secure boot and/or
> > trusted boot mode and the IMA_ARCH_POLICY Kconfig option being enabled.
> >
> >> Move the arch_policy_entry pointer into ima_namespace.
> > Perhaps include something about namespaces being allowed or not allowed
> > to kexec a new kernel or load kernel modules.
>
> Namespaces are not allowed to kexec but special-casing the init_ima_ns
> in the code to handle namespaces differently makes it much harder to
> read the code. I would avoid special-casing init_ima_ns as much as
> possible and therefore I have moved the arch_policy_entry into the
> ima_namespace.
Please include this in the patch description, but re-write the last
line in the 3rd person, like:
To avoid special-casing init_ima_ns, as much as possible, move the
arch_policy_entry into the ima_namespace.
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v10 06/27] ima: Move arch_policy_entry into ima_namespace
From: Stefan Berger @ 2022-02-16 20:48 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris, Christian Brauner
In-Reply-To: <bf435ffa5d176213acabb8c576c159d2cbd4d395.camel@linux.ibm.com>
On 2/16/22 11:39, Mimi Zohar wrote:
> On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote
>
> Let's update the patch description providing a bit more background
> info:
>
> The archictecture specific policy rules, currently defined for EFI and
> powerpc, require the kexec kernel image and kernel modules to be
> validly signed and measured, based on the system's secure boot and/or
> trusted boot mode and the IMA_ARCH_POLICY Kconfig option being enabled.
>
>> Move the arch_policy_entry pointer into ima_namespace.
> Perhaps include something about namespaces being allowed or not allowed
> to kexec a new kernel or load kernel modules.
Namespaces are not allowed to kexec but special-casing the init_ima_ns
in the code to handle namespaces differently makes it much harder to
read the code. I would avoid special-casing init_ima_ns as much as
possible and therefore I have moved the arch_policy_entry into the
ima_namespace.
Stefan
> thanks,
>
> Mimi
>> When freeing the memory set the pointer to NULL.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Acked-by: Christian Brauner <brauner@kernel.org>
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
^ permalink raw reply
* Re: [RFC PATCH 0/1] SELinux-namespaces
From: Paul Moore @ 2022-02-16 20:47 UTC (permalink / raw)
To: Igor Baranov
Cc: hw.likun, jamorris, linux-security-module, selinux,
stephen.smalley.work, xiujianfeng, alexander.kozhevnikov,
yusongping, artem.kuzin
In-Reply-To: <20220216125206.20975-1-igor.baranov@huawei.com>
On Wed, Feb 16, 2022 at 7:52 AM Igor Baranov <igor.baranov@huawei.com> wrote:
>
> Hi all!
> Our team at Huawei decided to revive the work on SELinux namespaces.
> We took https://github.com/stephensmalley/selinux-kernel/tree/working-selinuxns
> as a basis with some patches from selinuxns-xattr.
Hello!
For reference there is a *slightly* more recent forward port of those
patches in the main SELinux repo under the working-selinuxns branch.
I haven't forward ported those patches since v5.10-rc1 as there are
some rather significant technical hurdles around persistent object
labeling which I don't believe have been adequately resolved yet. The
prefixed/namespaces xattr approach that you mention above may work for
a limited number of namespaces, but I worry there is a scalability
issue that needs to be resolved; we can't simply keep adding xattrs to
inodes.
* https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
Also, are there rest of your patches online anywhere? Patch 1/1 isn't
very interesting on its own.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v10 05/27] ima: Define ima_namespace struct and start moving variables into it
From: Stefan Berger @ 2022-02-16 20:25 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris, Christian Brauner
In-Reply-To: <429010298df589687e1d1a09bac21e302d642c7e.camel@linux.ibm.com>
On 2/16/22 09:41, Mimi Zohar wrote:
> On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
>> Define the ima_namespace structure and the ima_namespace variable
>> init_ima_ns for the host's IMA namespace. Implement basic functions for
>> namespacing support.
> Implement the basic functions - ima_ns_init() and ima_init_namespace()
> - for namespacing support.
>
>> Move variables related to the IMA policy into the ima_namespace. This way
>> the IMA policy of an IMA namespace can be set and displayed using a
>> front-end like securityfs.
>> Implement ima_ns_from_file() to get the IMA namespace via the user
>> namespace of the securityfs superblock that a file belongs to.
> Currently, ima_ns_from_file() doesn't exist in this patch.
Ah, left-over from previous version. Remove.
>
>> To get the current ima_namespace use &init_ima_ns when a function
>> that is related to a policy rule is called.
> In preparation for IMA namespacing, update the existing functions to
> pass the ima_namespace struct. For now, ...
>
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Acked-by: Christian Brauner <brauner@kernel.org>
> After addressing the one inline comment,
Done.
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Thanks.
^ permalink raw reply
* Re: [RFC PATCH 0/1] SELinux-namespaces
From: Casey Schaufler @ 2022-02-16 17:08 UTC (permalink / raw)
To: Igor Baranov, paul
Cc: hw.likun, jamorris, linux-security-module, selinux,
stephen.smalley.work, xiujianfeng, alexander.kozhevnikov,
yusongping, artem.kuzin, Casey Schaufler
In-Reply-To: <20220216125206.20975-1-igor.baranov@huawei.com>
On 2/16/2022 4:52 AM, Igor Baranov wrote:
> Hi all!
> Our team at Huawei decided to revive the work on SELinux namespaces.
> We took https://github.com/stephensmalley/selinux-kernel/tree/working-selinuxns
> as a basis with some patches from selinuxns-xattr.
> We reworked them significantly, fixing and adding functionality.
> As a result we managed to run a CentOS Docker container with SELinux in enforcing mode!
>
> We would like to start our discussion with the smallest, but most basic
> change: we gave each namespace a unique identifier.
> It is assigned to a namespace from the global counter
> that is incremented each time you create it.
>
> All the objects which in the original patchset kept a pointer to their
> namespace now store its identifier. It's needed only to determine whether
> an object belongs to our (in the current context) namespace or not.
> The aim of this change is to reduce the height of the Babel tower of pointers,
> because in the original patch there was such a mess and such bugs,
> that we decided to cut this Gordian knot, removing some pointers altogether.
>
> This is a very small part of our changes, but we see the point of discussing
> more when we come to this.
>
> Particularly because there are alternative approaches,
> such as Casey Schaufler's suggestion, which is mentioned
> in http://namei.org/presentations/selinux_namespacing_lca2018.pdf
> "How to deal with secids (32-bit IDs) which are passed to core kernel and
> cached there - Make them global" which seems quite promising for us too.
The promise of secids is that they are opaque identifiers and
thus only need to be meaningful to the policy engine.
> In the case of this approach, it is not necessary to store the namespace ID
> in objects, because it can be deduced from sid. But a detailed study of
> this area reveals some painful challenges. For example: fragmentation of
> the global sid space when loading/unloading different policies. And the
> depth of the rabbit hole is not obvious from current positions. This is a
> separate big topic.
The downside of secids is that eventually you do need to map them
to something meaningful to the policy engine. This is why I have always
(since 1986) objected to them. The SELinux implementation would be much
simpler without them. (I'm ignoring the networking implications for the
moment. :) ). Nonetheless, with separate AVC instances the mapping
should be (I've been told) straight forward.
>
> Igor Baranov (1):
> Replace state pointer with namespace id
>
> security/selinux/hooks.c | 29 ++++++++++++++++++++++++-----
> security/selinux/include/objsec.h | 4 +++-
> security/selinux/include/security.h | 2 ++
> 3 files changed, 29 insertions(+), 6 deletions(-)
>
^ permalink raw reply
* Re: [PATCH v10 06/27] ima: Move arch_policy_entry into ima_namespace
From: Mimi Zohar @ 2022-02-16 16:39 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris, Christian Brauner
In-Reply-To: <20220201203735.164593-7-stefanb@linux.ibm.com>
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote
Let's update the patch description providing a bit more background
info:
The archictecture specific policy rules, currently defined for EFI and
powerpc, require the kexec kernel image and kernel modules to be
validly signed and measured, based on the system's secure boot and/or
trusted boot mode and the IMA_ARCH_POLICY Kconfig option being enabled.
> Move the arch_policy_entry pointer into ima_namespace.
Perhaps include something about namespaces being allowed or not allowed
to kexec a new kernel or load kernel modules.
thanks,
Mimi
>
> When freeing the memory set the pointer to NULL.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Christian Brauner <brauner@kernel.org>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
^ permalink raw reply
* Re: [PATCH v10 05/27] ima: Define ima_namespace struct and start moving variables into it
From: Mimi Zohar @ 2022-02-16 14:41 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris, Christian Brauner
In-Reply-To: <20220201203735.164593-6-stefanb@linux.ibm.com>
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
> Define the ima_namespace structure and the ima_namespace variable
> init_ima_ns for the host's IMA namespace. Implement basic functions for
> namespacing support.
Implement the basic functions - ima_ns_init() and ima_init_namespace()
- for namespacing support.
>
> Move variables related to the IMA policy into the ima_namespace. This way
> the IMA policy of an IMA namespace can be set and displayed using a
> front-end like securityfs.
> Implement ima_ns_from_file() to get the IMA namespace via the user
> namespace of the securityfs superblock that a file belongs to.
Currently, ima_ns_from_file() doesn't exist in this patch.
>
> To get the current ima_namespace use &init_ima_ns when a function
> that is related to a policy rule is called.
In preparation for IMA namespacing, update the existing functions to
pass the ima_namespace struct. For now, ...
>
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Christian Brauner <brauner@kernel.org>
After addressing the one inline comment,
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>
> ---
>
> @@ -119,6 +117,17 @@ struct ima_kexec_hdr {
> u64 count;
> };
>
> +struct ima_namespace {
> + /* policy rules */
> + struct list_head ima_default_rules;
> + struct list_head ima_policy_rules;
> + struct list_head ima_temp_rules;
These local policy variables weren't previously commented, but with the
move to a structure it would be good to add comments. For example, the
architecture policy rules persist even after a custom policy is loaded.
ima_default_rules: /* Kconfig, builtin, & arch rules */
ima_policy_rules: /* arch & custom rules */
> +
> + struct list_head __rcu *ima_rules; /* current policy */
/* Pointer to the current policy */.
> + int ima_policy_flag;
> +} __randomize_layout;
> +extern struct ima_namespace init_ima_ns;
> +
> extern const int read_idmap[];
>
> #ifdef CONFIG_HAVE_IMA_KEXEC
>
> {
> + struct ima_namespace *ns = &init_ima_ns;
> char *data;
> ssize_t result;
>
^ permalink raw reply
* Re: [PATCH v10 07/27] ima: Move ima_htable into ima_namespace
From: Mimi Zohar @ 2022-02-16 14:41 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris, Christian Brauner
In-Reply-To: <20220201203735.164593-8-stefanb@linux.ibm.com>
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
> Move ima_htable into ima_namespace. This way a front-end like
> securityfs can show the number of violations of an IMA namespace.
^can show the number of measurement records and number of violations
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Christian Brauner <brauner@kernel.org>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
^ permalink raw reply
* [RFC PATCH 0/1] SELinux-namespaces
From: Igor Baranov @ 2022-02-16 12:52 UTC (permalink / raw)
To: paul
Cc: hw.likun, jamorris, linux-security-module, selinux,
stephen.smalley.work, xiujianfeng, alexander.kozhevnikov,
yusongping, artem.kuzin, Igor Baranov
In-Reply-To: <CAHC9VhR3ZbcNM8awhJs9_NXmdUXHO4XoH8s2d3MjhMXwkgbh=Q@mail.gmail.com>
Hi all!
Our team at Huawei decided to revive the work on SELinux namespaces.
We took https://github.com/stephensmalley/selinux-kernel/tree/working-selinuxns
as a basis with some patches from selinuxns-xattr.
We reworked them significantly, fixing and adding functionality.
As a result we managed to run a CentOS Docker container with SELinux in enforcing mode!
We would like to start our discussion with the smallest, but most basic
change: we gave each namespace a unique identifier.
It is assigned to a namespace from the global counter
that is incremented each time you create it.
All the objects which in the original patchset kept a pointer to their
namespace now store its identifier. It's needed only to determine whether
an object belongs to our (in the current context) namespace or not.
The aim of this change is to reduce the height of the Babel tower of pointers,
because in the original patch there was such a mess and such bugs,
that we decided to cut this Gordian knot, removing some pointers altogether.
This is a very small part of our changes, but we see the point of discussing
more when we come to this.
Particularly because there are alternative approaches,
such as Casey Schaufler's suggestion, which is mentioned
in http://namei.org/presentations/selinux_namespacing_lca2018.pdf
"How to deal with secids (32-bit IDs) which are passed to core kernel and
cached there - Make them global" which seems quite promising for us too.
In the case of this approach, it is not necessary to store the namespace ID
in objects, because it can be deduced from sid. But a detailed study of
this area reveals some painful challenges. For example: fragmentation of
the global sid space when loading/unloading different policies. And the
depth of the rabbit hole is not obvious from current positions. This is a
separate big topic.
Igor Baranov (1):
Replace state pointer with namespace id
security/selinux/hooks.c | 29 ++++++++++++++++++++++++-----
security/selinux/include/objsec.h | 4 +++-
security/selinux/include/security.h | 2 ++
3 files changed, 29 insertions(+), 6 deletions(-)
--
2.34.1
^ permalink raw reply
* [RFC PATCH 1/1] selinuxns: Replace state pointer with namespace id
From: Igor Baranov @ 2022-02-16 12:52 UTC (permalink / raw)
To: paul
Cc: hw.likun, jamorris, linux-security-module, selinux,
stephen.smalley.work, xiujianfeng, alexander.kozhevnikov,
yusongping, artem.kuzin, Igor Baranov
In-Reply-To: <20220216125206.20975-1-igor.baranov@huawei.com>
All the objects which in the original patchset kept a pointer to their
namespace now store its identifier. It's needed only to determine whether
an object belongs to our (in the current context) namespace or not.
The aim of this change is to reduce the height of the Babel tower of pointers.
Signed-off-by: Igor Baranov <igor.baranov@huawei.com>
---
security/selinux/hooks.c | 29 ++++++++++++++++++++++++-----
security/selinux/include/objsec.h | 4 +++-
security/selinux/include/security.h | 2 ++
3 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a1716ce534dd..69bc8a5fc5b4 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -106,6 +106,9 @@
/* SECMARK reference count */
static atomic_t selinux_secmark_refcount = ATOMIC_INIT(0);
+/* Sequential namespace id */
+static atomic64_t selinux_namespace_id = ATOMIC_INIT(0);
+
#ifdef CONFIG_SECURITY_SELINUX_DEVELOP
static int selinux_enforcing_boot __initdata;
@@ -812,6 +815,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
goto out;
root_isec->sid = rootcontext_sid;
+ root_isec->namespace_id = current_selinux_state->id;
root_isec->initialized = LABEL_INITIALIZED;
}
@@ -1556,6 +1560,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
isec->initialized = LABEL_INITIALIZED;
isec->sid = sid;
+ isec->namespace_id = current_selinux_state->id;
}
out_unlock:
@@ -2588,7 +2593,7 @@ sbsec_alloc(struct superblock_security_head *sbsech)
sbsec->sid = SECINITSID_UNLABELED;
sbsec->def_sid = SECINITSID_FILE;
sbsec->mntpoint_sid = SECINITSID_UNLABELED;
- sbsec->state = get_selinux_state(current_selinux_state);
+ sbsec->namespace_id = current_selinux_state->id;
return sbsec;
}
@@ -2599,12 +2604,12 @@ find_sbsec(struct superblock_security_head *sbsech)
struct superblock_security_struct *cur, *new;
cur = container_of(sbsech->head.next, struct superblock_security_struct, sbsec_list);
- if (cur->state == current_selinux_state)
+ if (cur->namespace_id == current_selinux_state->id)
return cur;
spin_lock(&sbsech->lock);
list_for_each_entry(cur, &sbsech->head, sbsec_list) {
- if (cur->state == current_selinux_state)
+ if (cur->namespace_id == current_selinux_state->id)
goto unlock;
}
spin_unlock(&sbsech->lock);
@@ -2617,7 +2622,7 @@ find_sbsec(struct superblock_security_head *sbsech)
spin_lock(&sbsech->lock);
list_for_each_entry(cur, &sbsech->head, sbsec_list) {
- if (cur->state == current_selinux_state)
+ if (cur->namespace_id == current_selinux_state->id)
goto unlock;
}
list_add(&new->sbsec_list, &sbsech->head);
@@ -2649,7 +2654,6 @@ static void selinux_sb_free_security(struct super_block *sb)
struct superblock_security_head *sbsech = selinux_head_of_superblock(sb);
list_for_each_entry_safe(entry, tmp, &sbsech->head, sbsec_list) {
- put_selinux_state(entry->state);
kfree(entry);
}
}
@@ -2911,6 +2915,7 @@ static int selinux_inode_alloc_security(struct inode *inode)
isec->sclass = SECCLASS_FILE;
isec->task_sid = sid;
isec->initialized = LABEL_INVALID;
+ isec->namespace_id = current_selinux_state->id;
return 0;
}
@@ -2985,6 +2990,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
struct inode_security_struct *isec = selinux_inode(inode);
isec->sclass = inode_mode_to_security_class(inode->i_mode);
isec->sid = newsid;
+ isec->namespace_id = current_selinux_state->id;
isec->initialized = LABEL_INITIALIZED;
}
@@ -3320,6 +3326,7 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
spin_lock(&isec->lock);
isec->sclass = inode_mode_to_security_class(inode->i_mode);
isec->sid = newsid;
+ isec->namespace_id = current_selinux_state->id;
isec->initialized = LABEL_INITIALIZED;
spin_unlock(&isec->lock);
@@ -3479,6 +3486,7 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
spin_lock(&isec->lock);
isec->sclass = inode_mode_to_security_class(inode->i_mode);
isec->sid = newsid;
+ isec->namespace_id = current_selinux_state->id;
isec->initialized = LABEL_INITIALIZED;
spin_unlock(&isec->lock);
return 0;
@@ -3636,6 +3644,7 @@ static int selinux_file_alloc_security(struct file *file)
fsec->sid = sid;
fsec->fown_sid = sid;
+ fsec->namespace_id = current_selinux_state->id;
return 0;
}
@@ -3950,6 +3959,7 @@ static int selinux_file_open(struct file *file)
*/
fsec->isid = isec->sid;
fsec->pseqno = avc_policy_seqno(current_selinux_state);
+ fsec->namespace_id = current_selinux_state->id;
/*
* Since the inode label or policy seqno may have changed
* between the selinux_inode_permission check and the saving
@@ -4268,6 +4278,7 @@ static void selinux_task_to_inode(struct task_struct *p,
spin_lock(&isec->lock);
isec->sclass = inode_mode_to_security_class(inode->i_mode);
isec->sid = sid;
+ isec->namespace_id = current_selinux_state->id;
isec->initialized = LABEL_INITIALIZED;
spin_unlock(&isec->lock);
}
@@ -4634,6 +4645,7 @@ static int selinux_socket_post_create(struct socket *sock, int family,
isec->sclass = sclass;
isec->sid = sid;
+ isec->namespace_id = current_selinux_state->id;
isec->initialized = LABEL_INITIALIZED;
if (sock->sk) {
@@ -4930,6 +4942,7 @@ static int selinux_socket_accept(struct socket *sock, struct socket *newsock)
newisec = inode_security_novalidate(SOCK_INODE(newsock));
newisec->sclass = sclass;
newisec->sid = sid;
+ newisec->namespace_id = current_selinux_state->id;
newisec->initialized = LABEL_INITIALIZED;
return 0;
@@ -7332,6 +7345,11 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
static void selinux_state_free(struct work_struct *work);
+u64 selinux_new_state_id(void)
+{
+ return atomic64_fetch_add(1, &selinux_namespace_id);
+}
+
int selinux_state_create(struct selinux_state *parent, struct selinux_state **state)
{
struct selinux_state *newstate;
@@ -7341,6 +7359,7 @@ int selinux_state_create(struct selinux_state *parent, struct selinux_state **st
if (!newstate)
return -ENOMEM;
+ newstate->id = selinux_new_state_id();
refcount_set(&newstate->count, 1);
INIT_WORK(&newstate->work, selinux_state_free);
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 6ad1db45b0d9..24ef0bc68eda 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -42,6 +42,7 @@ struct inode_security_struct {
u16 sclass; /* security class of this object */
unsigned char initialized; /* initialization flag */
spinlock_t lock;
+ u64 namespace_id; /* pointer to selinux_state */
};
struct file_security_struct {
@@ -49,6 +50,7 @@ struct file_security_struct {
u32 fown_sid; /* SID of file owner (for SIGIO) */
u32 isid; /* SID of inode at the time of file open */
u32 pseqno; /* Policy seqno at the time of file open */
+ u64 namespace_id;
};
struct superblock_security_head {
@@ -66,7 +68,7 @@ struct superblock_security_struct {
struct mutex lock;
struct list_head isec_head;
spinlock_t isec_lock;
- struct selinux_state *state; /* pointer to selinux_state */
+ u64 namespace_id; /* id of selinux_state */
struct list_head sbsec_list;
};
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index a5b698aae38c..b80622770543 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -111,8 +111,10 @@ struct selinux_state {
struct selinux_policy __rcu *policy;
struct mutex policy_mutex;
struct selinux_state *parent;
+ u64 id;
} __randomize_layout;
+u64 selinux_new_state_id(void);
int selinux_state_create(struct selinux_state *parent, struct selinux_state **state);
void __put_selinux_state(struct selinux_state *state);
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 4/4] module, KEYS: Make use of platform keyring for signature verification
From: Michal Suchánek @ 2022-02-16 12:09 UTC (permalink / raw)
To: Mimi Zohar
Cc: Catalin Marinas, Will Deacon, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Philipp Rudo, Baoquan He, Alexander Egorenkov, AKASHI Takahiro,
James Morse, Dave Young, Kairui Song, Martin Schwidefsky,
linux-arm-kernel, linux-kernel, linux-s390, linux-modules,
keyrings, linux-security-module, stable, Eric Snowberg
In-Reply-To: <edb305079c28e49021166423af0378f8d218f269.camel@linux.ibm.com>
On Wed, Feb 16, 2022 at 06:58:51AM -0500, Mimi Zohar wrote:
> On Wed, 2022-02-16 at 11:56 +0100, Michal Suchánek wrote:
> > On Tue, Feb 15, 2022 at 05:12:32PM -0500, Mimi Zohar wrote:
> > > On Tue, 2022-02-15 at 21:47 +0100, Michal Suchánek wrote:
> > > > Hello,
> > > >
> > > > On Tue, Feb 15, 2022 at 03:08:18PM -0500, Mimi Zohar wrote:
> > > > > [Cc'ing Eric Snowberg]
> > > > >
> > > > > Hi Michal,
> > > > >
> > > > > On Tue, 2022-02-15 at 20:39 +0100, Michal Suchanek wrote:
> > > > > > Commit 278311e417be ("kexec, KEYS: Make use of platform keyring for signature verify")
> > > > > > adds support for use of platform keyring in kexec verification but
> > > > > > support for modules is missing.
> > > > > >
> > > > > > Add support for verification of modules with keys from platform keyring
> > > > > > as well.
> > > > >
> > > > > Permission for loading the pre-OS keys onto the "platform" keyring and
> > > > > using them is limited to verifying the kexec kernel image, nothing
> > > > > else.
> > > >
> > > > Why is the platform keyring limited to kexec, and nothing else?
> > > >
> > > > It should either be used for everything or for nothing. You have the
> > > > option to compile it in and then it should be used, and the option to
> > > > not compile it in and then it cannot be used.
> > > >
> > > > There are two basic use cases:
> > > >
> > > > (1) there is a vendor key which is very hard to use so you sign
> > > > something small and simple like shim with the vendor key, and sign your
> > > > kernel and modules with your own key that's typically enrolled with shim
> > > > MOK, and built into the kernel.
> > > >
> > > > (2) you import your key into the firmware, and possibly disable the
> > > > vendor key. You can load the kernel directly without shim, and then your
> > > > signing key is typically in the platform keyring and built into the
> > > > kernel.
> > > >
> > > > In neither case do I see any reason to use some keyrings for kexec and
> > > > other keyrings for modules.
> > >
> > > When building your own kernel there isn't a problem. Additional keys
> > > may be built into the kernel image, which are loaded onto the
> > > ".builtin_trusted_keys" keyring, and may be stored in MOK. Normally
> > > different keys are used for signing the kernel image and kernel
> >
> > That's actually not normal.
> >
> > > modules. Kernel modules can be signed by the build time ephemeral
> > > kernel module signing key, which is built into the kernel and
> > > automatically loaded onto the ".builtin_trusted_keys" keyring.
> >
> > Right, there is this advice to use ephemeral key to sign modules.
> >
> > I don't think that's a sound advice in general. It covers only the
> > special case when you build the kernel once, only rebuild the whole
> > kernel and never just one module, don't use any 3rd party module, don't
> > bother signing firmware (I am not sure that is supported right now but
> > if you are into integrity and stuff you can see that it makes sense to
> > sign it, too).
> >
> > And you need to manage the key you use for the kernel signing, anyway.
> > Sure, you could use the same ephemeral key as for the modules, enroll
> > it, and shred it but then it is NOT a key different from the one you use
> > for modules.
> >
> > Or you could maintain a long-lived key for the kernel, but if you do I
> > do NOT see any reason to not use it also for modules, in-tree and
> > out-of-tree.
>
> If signing ALL kernel modules, in-tree and out-of-tree, with the same
> key as the kernel image, is your real intention, then by all means
Why would you sign them with different keys, specifically?
For out of tree modules, sure. But that's an ADDITIONAL key, not
REMOVAL of a key.
> write a complete patch description with the motivation for why kernel
> module signatures need to be verified against this one pre-OS key
> stored only in the platform keyring. Such a major change like this
> shouldn't be buried here.
No, in my book it does not make sense to verify anything against the
pre-os key at all in the common case.
However, if you do verify the kernel against the pre-os key it does not
make sense to not verify modules against the pre-os key. There is no
sense using different key for kernel and modules. They are both built in
the same environment with access to same the keys.
> Otherwise, I suggest looking at Eric Snowberg's "Enroll kernel keys
> thru MOK patch set" patch set [1], as previously mentioned, which is
> queued to be upstreamed by Jarkko. It loads MOK keys onto the
> '.machine' keyring, which is linked to the '.secondary_trusted_keys"
> keyring. A subsequent patch set will enable IMA support.
I don't really care how many keyrings there are. What I care about is
that they are used conssitently.
Thanks
Michal
^ permalink raw reply
* Re: [PATCH 4/4] module, KEYS: Make use of platform keyring for signature verification
From: Mimi Zohar @ 2022-02-16 11:58 UTC (permalink / raw)
To: Michal Suchánek
Cc: Catalin Marinas, Will Deacon, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Philipp Rudo, Baoquan He, Alexander Egorenkov, AKASHI Takahiro,
James Morse, Dave Young, Kairui Song, Martin Schwidefsky,
linux-arm-kernel, linux-kernel, linux-s390, linux-modules,
keyrings, linux-security-module, stable, Eric Snowberg
In-Reply-To: <20220216105645.GS3113@kunlun.suse.cz>
On Wed, 2022-02-16 at 11:56 +0100, Michal Suchánek wrote:
> On Tue, Feb 15, 2022 at 05:12:32PM -0500, Mimi Zohar wrote:
> > On Tue, 2022-02-15 at 21:47 +0100, Michal Suchánek wrote:
> > > Hello,
> > >
> > > On Tue, Feb 15, 2022 at 03:08:18PM -0500, Mimi Zohar wrote:
> > > > [Cc'ing Eric Snowberg]
> > > >
> > > > Hi Michal,
> > > >
> > > > On Tue, 2022-02-15 at 20:39 +0100, Michal Suchanek wrote:
> > > > > Commit 278311e417be ("kexec, KEYS: Make use of platform keyring for signature verify")
> > > > > adds support for use of platform keyring in kexec verification but
> > > > > support for modules is missing.
> > > > >
> > > > > Add support for verification of modules with keys from platform keyring
> > > > > as well.
> > > >
> > > > Permission for loading the pre-OS keys onto the "platform" keyring and
> > > > using them is limited to verifying the kexec kernel image, nothing
> > > > else.
> > >
> > > Why is the platform keyring limited to kexec, and nothing else?
> > >
> > > It should either be used for everything or for nothing. You have the
> > > option to compile it in and then it should be used, and the option to
> > > not compile it in and then it cannot be used.
> > >
> > > There are two basic use cases:
> > >
> > > (1) there is a vendor key which is very hard to use so you sign
> > > something small and simple like shim with the vendor key, and sign your
> > > kernel and modules with your own key that's typically enrolled with shim
> > > MOK, and built into the kernel.
> > >
> > > (2) you import your key into the firmware, and possibly disable the
> > > vendor key. You can load the kernel directly without shim, and then your
> > > signing key is typically in the platform keyring and built into the
> > > kernel.
> > >
> > > In neither case do I see any reason to use some keyrings for kexec and
> > > other keyrings for modules.
> >
> > When building your own kernel there isn't a problem. Additional keys
> > may be built into the kernel image, which are loaded onto the
> > ".builtin_trusted_keys" keyring, and may be stored in MOK. Normally
> > different keys are used for signing the kernel image and kernel
>
> That's actually not normal.
>
> > modules. Kernel modules can be signed by the build time ephemeral
> > kernel module signing key, which is built into the kernel and
> > automatically loaded onto the ".builtin_trusted_keys" keyring.
>
> Right, there is this advice to use ephemeral key to sign modules.
>
> I don't think that's a sound advice in general. It covers only the
> special case when you build the kernel once, only rebuild the whole
> kernel and never just one module, don't use any 3rd party module, don't
> bother signing firmware (I am not sure that is supported right now but
> if you are into integrity and stuff you can see that it makes sense to
> sign it, too).
>
> And you need to manage the key you use for the kernel signing, anyway.
> Sure, you could use the same ephemeral key as for the modules, enroll
> it, and shred it but then it is NOT a key different from the one you use
> for modules.
>
> Or you could maintain a long-lived key for the kernel, but if you do I
> do NOT see any reason to not use it also for modules, in-tree and
> out-of-tree.
If signing ALL kernel modules, in-tree and out-of-tree, with the same
key as the kernel image, is your real intention, then by all means
write a complete patch description with the motivation for why kernel
module signatures need to be verified against this one pre-OS key
stored only in the platform keyring. Such a major change like this
shouldn't be buried here.
Otherwise, I suggest looking at Eric Snowberg's "Enroll kernel keys
thru MOK patch set" patch set [1], as previously mentioned, which is
queued to be upstreamed by Jarkko. It loads MOK keys onto the
'.machine' keyring, which is linked to the '.secondary_trusted_keys"
keyring. A subsequent patch set will enable IMA support.
[1]
https://lore.kernel.org/lkml/20220126025834.255493-1-eric.snowberg@oracle.com/
--
thanks,
Mimi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox