| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <D0LMH9CLMVY5.2XEMXNU4K5985@kernel.org>
Date: Tue, 16 Apr 2024 17:38:46 +0300
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Roberto Sassu" <roberto.sassu@...weicloud.com>, <corbet@....net>,
<paul@...l-moore.com>, <jmorris@...ei.org>, <serge@...lyn.com>,
<akpm@...ux-foundation.org>, <shuah@...nel.org>,
<mcoquelin.stm32@...il.com>, <alexandre.torgue@...s.st.com>,
<mic@...ikod.net>
Cc: <linux-security-module@...r.kernel.org>, <linux-doc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>,
<bpf@...r.kernel.org>, <zohar@...ux.ibm.com>, <dmitry.kasatkin@...il.com>,
<linux-integrity@...r.kernel.org>, <wufan@...ux.microsoft.com>,
<pbrobinson@...il.com>, <zbyszek@...waw.pl>, <hch@....de>,
<mjg59@...f.ucam.org>, <pmatilai@...hat.com>, <jannh@...gle.com>,
<dhowells@...hat.com>, <jikos@...nel.org>, <mkoutny@...e.com>,
<ppavlu@...e.com>, <petr.vorel@...il.com>, <mzerqung@...inter.de>,
<kgold@...ux.ibm.com>, "Roberto Sassu" <roberto.sassu@...wei.com>
Subject: Re: [PATCH v4 03/14] digest_cache: Add securityfs interface
On Tue Apr 16, 2024 at 1:15 PM EEST, Roberto Sassu wrote:
> On 4/15/2024 9:32 PM, Jarkko Sakkinen wrote:
> > On Mon Apr 15, 2024 at 5:24 PM EEST, Roberto Sassu wrote:
> >> From: Roberto Sassu <roberto.sassu@...wei.com>
> >>
> >> Add the digest_cache_path file in securityfs, to let root change/read the
> >> default path (file or directory) from where digest lists are looked up.
> >>
> >> An RW semaphore prevents the default path from changing while
> >> digest_list_new() and read_default_path() are executed, so that those read
> >> a stable value. Multiple digest_list_new() and read_default_path() calls,
> >> instead, can be done in parallel, since they are the readers.
> >>
> >> Changing the default path does not affect digest caches created with the
> >> old path.
> >>
> >> Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
> >> ---
> >> security/digest_cache/Kconfig | 4 ++
> >> security/digest_cache/Makefile | 2 +-
> >> security/digest_cache/internal.h | 1 +
> >> security/digest_cache/main.c | 10 +++-
> >> security/digest_cache/secfs.c | 87 ++++++++++++++++++++++++++++++++
> >> 5 files changed, 102 insertions(+), 2 deletions(-)
> >> create mode 100644 security/digest_cache/secfs.c
> >>
> >> diff --git a/security/digest_cache/Kconfig b/security/digest_cache/Kconfig
> >> index e53fbf0779d6..dfabe5d6e3ca 100644
> >> --- a/security/digest_cache/Kconfig
> >> +++ b/security/digest_cache/Kconfig
> >> @@ -14,3 +14,7 @@ config DIGEST_LIST_DEFAULT_PATH
> >> default "/etc/digest_lists"
> >> help
> >> Default directory where digest_cache LSM expects to find digest lists.
> >> +
> >> + It can be changed at run-time, by writing the new path to the
> >> + securityfs interface. Digest caches created with the old path are
> >> + not affected by the change.
> >> diff --git a/security/digest_cache/Makefile b/security/digest_cache/Makefile
> >> index 48848c41253e..1330655e33b1 100644
> >> --- a/security/digest_cache/Makefile
> >> +++ b/security/digest_cache/Makefile
> >> @@ -4,4 +4,4 @@
> >>
> >> obj-$(CONFIG_SECURITY_DIGEST_CACHE) += digest_cache.o
> >>
> >> -digest_cache-y := main.o
> >> +digest_cache-y := main.o secfs.o
> >> diff --git a/security/digest_cache/internal.h b/security/digest_cache/internal.h
> >> index 5f04844af3a5..bbf5eefe5c82 100644
> >> --- a/security/digest_cache/internal.h
> >> +++ b/security/digest_cache/internal.h
> >> @@ -49,6 +49,7 @@ struct digest_cache_security {
> >>
> >> extern struct lsm_blob_sizes digest_cache_blob_sizes;
> >> extern char *default_path_str;
> >> +extern struct rw_semaphore default_path_sem;
> >>
> >> static inline struct digest_cache_security *
> >> digest_cache_get_security(const struct inode *inode)
> >> diff --git a/security/digest_cache/main.c b/security/digest_cache/mainc
> >> index 14dba8915e99..661c8d106791 100644
> >> --- a/security/digest_cache/main.c
> >> +++ b/security/digest_cache/main.c
> >> @@ -18,6 +18,9 @@ static struct kmem_cache *digest_cache_cache __read_mostly;
> >>
> >> char *default_path_str = CONFIG_DIGEST_LIST_DEFAULT_PATH;
> >>
> >> +/* Protects default_path_str. */
> >> +struct rw_semaphore default_path_sem;
> >> +
> >> /**
> >> * digest_cache_alloc_init - Allocate and initialize a new digest cache
> >> * @path_str: Path string of the digest list
> >> @@ -274,9 +277,12 @@ struct digest_cache *digest_cache_get(struct dentry *dentry)
> >>
> >> /* Serialize accesses to inode for which the digest cache is used. */
> >> mutex_lock(&dig_sec->dig_user_mutex);
> >> - if (!dig_sec->dig_user)
> >> + if (!dig_sec->dig_user) {
> >> + down_read(&default_path_sem);
> >> /* Consume extra reference from digest_cache_create(). */
> >> dig_sec->dig_user = digest_cache_new(dentry);
> >> + up_read(&default_path_sem);
> >> + }
> >>
> >> if (dig_sec->dig_user)
> >> /* Increment ref. count for reference returned to the caller. */
> >> @@ -386,6 +392,8 @@ static const struct lsm_id digest_cache_lsmid = {
> >> */
> >> static int __init digest_cache_init(void)
> >> {
> >> + init_rwsem(&default_path_sem);
> >> +
> >> digest_cache_cache = kmem_cache_create("digest_cache_cache",
> >> sizeof(struct digest_cache),
> >> 0, SLAB_PANIC,
> >> diff --git a/security/digest_cache/secfs.c b/security/digest_cache/secfs.c
> >> new file mode 100644
> >> index 000000000000..d3a37bf3588e
> >> --- /dev/null
> >> +++ b/security/digest_cache/secfs.c
> >> @@ -0,0 +1,87 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> >> + *
> >> + * Author: Roberto Sassu <roberto.sassu@...wei.com>
> >> + *
> >> + * Implement the securityfs interface of the digest_cache LSM.
> >> + */
> >> +
> >> +#define pr_fmt(fmt) "DIGEST CACHE: "fmt
> >> +#include <linux/security.h>
> >> +
> >> +#include "internal.h"
> >> +
> >> +static struct dentry *default_path_dentry;
> >> +
> >> +/**
> >> + * write_default_path - Write default path
> >> + * @file: File descriptor of the securityfs file
> >> + * @buf: User space buffer
> >> + * @datalen: Amount of data to write
> >> + * @ppos: Current position in the file
> >> + *
> >> + * This function sets the new default path where digest lists can be found.
> >> + * Can be either a regular file or a directory.
> >> + *
> >> + * Return: Length of path written on success, a POSIX error code otherwise.
> >> + */
> >> +static ssize_t write_default_path(struct file *file, const char __user *buf,
> >> + size_t datalen, loff_t *ppos)
> >> +{
> >> + char *new_default_path_str;
> >> +
> >> + new_default_path_str = memdup_user_nul(buf, datalen);
> >> + if (IS_ERR(new_default_path_str))
> >> + return PTR_ERR(new_default_path_str);
> >> +
> >> + down_write(&default_path_sem);
> >> + kfree_const(default_path_str);
> >> + default_path_str = new_default_path_str;
> >> + up_write(&default_path_sem);
> >> + return datalen;
> >> +}
> >> +
> >> +/**
> >> + * read_default_path - Read default path
> >> + * @file: File descriptor of the securityfs file
> >> + * @buf: User space buffer
> >> + * @datalen: Amount of data to read
> >> + * @ppos: Current position in the file
> >> + *
> >> + * This function returns the current default path where digest lists can be
> >> + * found. Can be either a regular file or a directory.
> >> + *
> >> + * Return: Length of path read on success, a POSIX error code otherwise.
> >> + */
> >> +static ssize_t read_default_path(struct file *file, char __user *buf,
> >> + size_t datalen, loff_t *ppos)
> >> +{
> >> + int ret;
> >> +
> >> + down_read(&default_path_sem);
> >> + ret = simple_read_from_buffer(buf, datalen, ppos, default_path_str,
> >> + strlen(default_path_str) + 1);
> >> + up_read(&default_path_sem);
> >> + return ret;
> >> +}
> >> +
> >> +static const struct file_operations default_path_ops = {
> >> + .open = generic_file_open,
> >> + .write = write_default_path,
> >> + .read = read_default_path,
> >> + .llseek = generic_file_llseek,
> >> +};
> >> +
> >> +static int __init digest_cache_path_init(void)
> >> +{
> >> + default_path_dentry = securityfs_create_file("digest_cache_path", 0660,
> >> + NULL, NULL,
> >> + &default_path_ops);
> >> + if (IS_ERR(default_path_dentry))
> >> + return -EFAULT;
> >
> > Nit: when overwriting error value with another error value it would be
> > best to document it with an inline comment. Otherwise, it is fine.
>
> Seems to make sense to return the right error. Will check why this one
> (I probably took from somewhere).
Yeah, I mean often when I read legacy code from kernel and find
places like these I spend even few hours finding the root for
doing something like this so it really has value to do it when
the code is still fresh :-) Nothing wrong in the action itself
when it makes sense given the circumstances.
BR, Jarkko
Powered by blists - more mailing lists