Received-SPF: pass (google.com: domain of linux-kernel+bounces-146609-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <d50530db-4a5e-4f58-997f-82090797398b@huaweicloud.com>
Date: Tue, 16 Apr 2024 12:15:54 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 03/14] digest_cache: Add securityfs interface
To: Jarkko Sakkinen <jarkko@kernel.org>, corbet@lwn.net, paul@paul-moore.com,
 jmorris@namei.org, serge@hallyn.com, akpm@linux-foundation.org,
 shuah@kernel.org, mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com,
 mic@digikod.net
Cc: linux-security-module@vger.kernel.org, linux-doc@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
 bpf@vger.kernel.org, zohar@linux.ibm.com, dmitry.kasatkin@gmail.com,
 linux-integrity@vger.kernel.org, wufan@linux.microsoft.com,
 pbrobinson@gmail.com, zbyszek@in.waw.pl, hch@lst.de, mjg59@srcf.ucam.org,
 pmatilai@redhat.com, jannh@google.com, dhowells@redhat.com,
 jikos@kernel.org, mkoutny@suse.com, ppavlu@suse.com, petr.vorel@gmail.com,
 mzerqung@0pointer.de, kgold@linux.ibm.com,
 Roberto Sassu <roberto.sassu@huawei.com>
References: <20240415142436.2545003-1-roberto.sassu@huaweicloud.com>
 <20240415142436.2545003-4-roberto.sassu@huaweicloud.com>
 <D0KY3YSZCLTG.24OGZPYS4AKDY@kernel.org>
From: Roberto Sassu <roberto.sassu@huaweicloud.com>
In-Reply-To: <D0KY3YSZCLTG.24OGZPYS4AKDY@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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@huawei.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@huawei.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/main.c
>> 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@huawei.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).

Thanks

Roberto

>> +
>> +	return 0;
>> +}
>> +
>> +late_initcall(digest_cache_path_init);
> 
> 
> BR, Jarkko