Date: Tue, 16 Apr 2024 12:34:18 +0200
From: Roberto Sassu <roberto.sassu@...weicloud.com>
To: Jarkko Sakkinen <jarkko@...nel.org>, 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 10/14] digest cache: Prefetch digest lists if requested

On 4/15/2024 9:42 PM, Jarkko Sakkinen wrote:
> On Mon Apr 15, 2024 at 5:24 PM EEST, Roberto Sassu wrote:
>> From: Roberto Sassu <roberto.sassu@...wei.com>
>>
>> A desirable goal when doing integrity measurements is that they are done
>> always in the same order across boots, so that the resulting PCR value
>> becomes predictable and suitable for sealing policies. However, due to
>> parallel execution of system services at boot, a deterministic order of
>> measurements is difficult to achieve.
>>
>> The digest_cache LSM is not exempted from this issue. Under the assumption
>> that only the digest list is measured, and file measurements are omitted if
>> their digest is found in that digest list, a PCR can be predictable only if
>> all files belong to the same digest list. Otherwise, it will still be
>> unpredictable, since files accessed in a non-deterministic order will cause
>> digest lists to be measured in a non-deterministic order too.
>>
>> Overcome this issue, if prefetching is enabled, by searching a digest list
>> file name in digest_list_dir_lookup_filename() among the entries of the
>> linked list built by digest_cache_dir_create(). If the file name does not
>> match, read the digest list to trigger its measurement. Otherwise, also
>> create a digest cache and return that to the caller. Release the extra
>> reference of the directory digest cache in digest_cache_new(), since it was
>> only used for the search and it is not going to be returned.
>>
>> Prefetching needs to be explicitly enabled by setting the new
>> security.dig_prefetch xattr to 1 in the directory containing the digest
>> lists. The newly introduced function digest_cache_prefetch_requested()
>> checks first if the DIR_PREFETCH bit is set in dig_owner, otherwise it
>> reads the xattr. digest_cache_create() sets DIR_PREFETCH in dig_owner, if
>> prefetching is enabled, before declaring the digest cache as initialized.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
>> ---
>>   include/uapi/linux/xattr.h       |  3 +
>>   security/digest_cache/dir.c      | 55 +++++++++++++++++-
>>   security/digest_cache/internal.h | 11 +++-
>>   security/digest_cache/main.c     | 95 +++++++++++++++++++++++++++++++-
>>   security/digest_cache/populate.c |  8 ++-
>>   security/digest_cache/verif.c    |  5 +-
>>   6 files changed, 170 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/uapi/linux/xattr.h b/include/uapi/linux/xattr.h
>> index 8a58cf4bce65..8af33d38d9e8 100644
>> --- a/include/uapi/linux/xattr.h
>> +++ b/include/uapi/linux/xattr.h
>> @@ -57,6 +57,9 @@
>>   #define XATTR_DIGEST_LIST_SUFFIX "digest_list"
>>   #define XATTR_NAME_DIGEST_LIST XATTR_SECURITY_PREFIX XATTR_DIGEST_LIST_SUFFIX
>>   
>> +#define XATTR_DIG_PREFETCH_SUFFIX "dig_prefetch"
>> +#define XATTR_NAME_DIG_PREFETCH XATTR_SECURITY_PREFIX XATTR_DIG_PREFETCH_SUFFIX
>> +
>>   #define XATTR_SELINUX_SUFFIX "selinux"
>>   #define XATTR_NAME_SELINUX XATTR_SECURITY_PREFIX XATTR_SELINUX_SUFFIX
>>   
>> diff --git a/security/digest_cache/dir.c b/security/digest_cache/dir.c
>> index 7bfcdd5f7ef1..a7d203c15386 100644
>> --- a/security/digest_cache/dir.c
>> +++ b/security/digest_cache/dir.c
>> @@ -54,6 +54,7 @@ static bool digest_cache_dir_iter(struct dir_context *__ctx, const char *name,
>>   	new_entry->seq_num = UINT_MAX;
>>   	new_entry->digest_cache = NULL;
>>   	mutex_init(&new_entry->digest_cache_mutex);
>> +	new_entry->prefetched = false;
>>   
>>   	if (new_entry->name[0] < '0' || new_entry->name[0] > '9')
>>   		goto out;
>> @@ -127,6 +128,7 @@ int digest_cache_dir_create(struct digest_cache *digest_cache,
>>    * @digest_cache: Digest cache
>>    * @digest: Digest to search
>>    * @algo: Algorithm of the digest to search
>> + * @filename: File name of the digest list to search
>>    *
>>    * This function iterates over the linked list created by
>>    * digest_cache_dir_create() and looks up the digest in the digest cache of
>> @@ -149,7 +151,8 @@ digest_cache_dir_lookup_digest(struct dentry *dentry,
>>   		if (!dir_entry->digest_cache) {
>>   			cache = digest_cache_create(dentry, digest_list_path,
>>   						    digest_cache->path_str,
>> -						    dir_entry->name);
>> +						    dir_entry->name, false,
>> +						    false);
>>   			/* Ignore digest caches that cannot be instantiated. */
>>   			if (!cache) {
>>   				mutex_unlock(&dir_entry->digest_cache_mutex);
>> @@ -158,6 +161,8 @@ digest_cache_dir_lookup_digest(struct dentry *dentry,
>>   
>>   			/* Consume extra ref. from digest_cache_create(). */
>>   			dir_entry->digest_cache = cache;
>> +			/* Digest list was read, mark entry as prefetched. */
>> +			dir_entry->prefetched = true;
>>   		}
>>   		mutex_unlock(&dir_entry->digest_cache_mutex);
>>   
>> @@ -171,6 +176,54 @@ digest_cache_dir_lookup_digest(struct dentry *dentry,
>>   	return 0UL;
>>   }
>>   
>> +/**
>> + * digest_cache_dir_lookup_filename - Lookup a digest list
>> + * @dentry: Dentry of the file whose digest list is looked up
>> + * @digest_list_path: Path structure of the digest list directory
>> + * @digest_cache: Digest cache
>> + * @filename: File name of the digest list to search
>> + *
>> + * This function iterates over the linked list created by
>> + * digest_cache_dir_create() and looks up a digest list with a matching file
>> + * name among the entries. If there is no match, it prefetches (reads) the
>> + * current digest list. Otherwise, it returns the digest cache pointer from
>> + * digest_cache_create() to the caller.
>> + *
>> + * Return: A digest cache pointer if the digest list if found, NULL otherwise.
>> + */
>> +struct digest_cache *
>> +digest_cache_dir_lookup_filename(struct dentry *dentry,
>> +				 struct path *digest_list_path,
>> +				 struct digest_cache *digest_cache,
>> +				 char *filename)
>> +{
>> +	struct digest_cache *cache;
>> +	struct dir_entry *dir_entry;
>> +	bool filename_found;
>> +
>> +	list_for_each_entry(dir_entry, &digest_cache->dir_entries, list) {
>> +		mutex_lock(&dir_entry->digest_cache_mutex);
>> +		filename_found = !strcmp(dir_entry->name, filename);
>> +		if (!filename_found && dir_entry->prefetched) {
>> +			mutex_unlock(&dir_entry->digest_cache_mutex);
>> +			continue;
>> +		}
>> +
>> +		cache = digest_cache_create(dentry, digest_list_path,
>> +					    digest_cache->path_str,
>> +					    dir_entry->name, false,
>> +					    filename_found ? false : true);
>> +
>> +		dir_entry->prefetched = true;
>> +		mutex_unlock(&dir_entry->digest_cache_mutex);
>> +
>> +		if (filename_found)
>> +			return cache;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>>   /**
>>    * digest_cache_dir_free - Free the stored file list and put digest caches
>>    * @digest_cache: Digest cache
>> diff --git a/security/digest_cache/internal.h b/security/digest_cache/internal.h
>> index b7afca8e04da..c13b35f6b2c0 100644
>> --- a/security/digest_cache/internal.h
>> +++ b/security/digest_cache/internal.h
>> @@ -17,6 +17,7 @@
>>   #define INIT_IN_PROGRESS	0	/* Digest cache being initialized. */
>>   #define INVALID			1	/* Digest cache marked as invalid. */
>>   #define IS_DIR			2	/* Digest cache created from dir. */
>> +#define DIR_PREFETCH		3	/* Prefetching requested for dir. */
>>   
>>   /**
>>    * struct readdir_callback - Structure to store information for dir iteration
>> @@ -37,6 +38,7 @@ struct readdir_callback {
>>    * @digest_cache: Digest cache associated to the directory entry
>>    * @digest_cache_mutex: Protects @digest_cache
>>    * @seq_num: Sequence number of the directory entry from file name
>> + * @prefetched: Whether the digest list has been already prefetched
>>    * @name: File name of the directory entry
>>    *
>>    * This structure represents a directory entry with a digest cache created
>> @@ -47,6 +49,7 @@ struct dir_entry {
>>   	struct digest_cache *digest_cache;
>>   	struct mutex digest_cache_mutex;
>>   	unsigned int seq_num;
>> +	bool prefetched;
>>   	char name[];
>>   } __packed;
>>   
>> @@ -205,7 +208,8 @@ digest_cache_from_file_sec(const struct file *file)
>>   /* main.c */
>>   struct digest_cache *digest_cache_create(struct dentry *dentry,
>>   					 struct path *digest_list_path,
>> -					 char *path_str, char *filename);
>> +					 char *path_str, char *filename,
>> +					 bool prefetch_req, bool prefetch);
>>   
>>   /* htable.c */
>>   int digest_cache_htable_init(struct digest_cache *digest_cache, u64 num_digests,
>> @@ -236,6 +240,11 @@ digest_cache_dir_lookup_digest(struct dentry *dentry,
>>   			       struct path *digest_list_path,
>>   			       struct digest_cache *digest_cache, u8 *digest,
>>   			       enum hash_algo algo);
>> +struct digest_cache *
>> +digest_cache_dir_lookup_filename(struct dentry *dentry,
>> +				 struct path *digest_list_path,
>> +				 struct digest_cache *digest_cache,
>> +				 char *filename);
>>   void digest_cache_dir_free(struct digest_cache *digest_cache);
>>   
>>   #endif /* _DIGEST_CACHE_INTERNAL_H */
>> diff --git a/security/digest_cache/main.c b/security/digest_cache/main.c
>> index 15f1486610a3..a5616fd07c1d 100644
>> --- a/security/digest_cache/main.c
>> +++ b/security/digest_cache/main.c
>> @@ -83,6 +83,8 @@ static void digest_cache_free(struct digest_cache *digest_cache)
>>    * @digest_list_path: Path structure of the digest list
>>    * @path_str: Path string of the digest list
>>    * @filename: Digest list file name (can be an empty string)
>> + * @prefetch_req: Whether prefetching has been requested
>> + * @prefetch: Whether prefetching of a digest list is being done
>>    *
>>    * This function first locates, from the passed path, the digest list inode
>>    * from which the digest cache will be created or retrieved (if it already
>> @@ -109,7 +111,8 @@ static void digest_cache_free(struct digest_cache *digest_cache)
>>    */
>>   struct digest_cache *digest_cache_create(struct dentry *dentry,
>>   					 struct path *digest_list_path,
>> -					 char *path_str, char *filename)
>> +					 char *path_str, char *filename,
>> +					 bool prefetch_req, bool prefetch)
> 
> 
> This has enough parameters to consider separate "digest_cache_descriptor"
> or whatever. In some architectures this is beyond register parameters,
> which is IMHO good threshold to consider that.
> 
> This will make e.g. tracing easier as you have to map only one parameter
> to a known struct to inspect the values.

Uhm, seems still good (6 parameters). Sure, I can make a structure to 
encapsulate the parameters, but will be used only for this purpose. Not 
sure the code understandability will improve.

Thanks

Roberto

>>   {
>>   	struct path file_path;
>>   	struct digest_cache *digest_cache = NULL;
>> @@ -148,6 +151,16 @@ struct digest_cache *digest_cache_create(struct dentry *dentry,
>>   				 dentry->d_name.name);
>>   			goto out;
>>   		}
>> +
>> +		if (prefetch) {
>> +			/* Fine to fail, we are just prefetching. */
>> +			ret = digest_cache_populate(NULL, digest_list_path,
>> +						    path_str, filename);
>> +			pr_debug("Digest list %s/%s %s prefetched\n",
>> +				 path_str, filename,
>> +				 !ret ? "has been" : "cannot be");
>> +			goto out;
>> +		}
>>   	}
>>   
>>   	dig_sec = digest_cache_get_security(inode);
>> @@ -176,6 +189,11 @@ struct digest_cache *digest_cache_create(struct dentry *dentry,
>>   
>>   	/* Make the other lock contenders wait until creation complete. */
>>   	set_bit(INIT_IN_PROGRESS, &dig_sec->dig_owner->flags);
>> +
>> +	/* Set DIR_PREFETCH if prefetching was requested. */
>> +	if (prefetch_req)
>> +		set_bit(DIR_PREFETCH, &digest_cache->flags);
>> +
>>   	mutex_unlock(&dig_sec->dig_owner_mutex);
>>   
>>   	if (S_ISREG(inode->i_mode)) {
>> @@ -220,6 +238,52 @@ struct digest_cache *digest_cache_create(struct dentry *dentry,
>>   	return digest_cache;
>>   }
>>   
>> +/**
>> + * digest_cache_prefetch_requested - Verify if prefetching is requested
>> + * @digest_list_path: Path structure of the digest list directory
>> + * @path_str: Path string of the digest list directory
>> + *
>> + * This function verifies whether or not digest list prefetching is requested.
>> + * If dig_owner exists in the inode security blob, it checks the DIR_PREFETCH
>> + * bit (faster). Otherwise, it reads the new security.dig_prefetch xattr.
>> + *
>> + * Return: True if prefetching is requested, false otherwise.
>> + */
>> +static bool digest_cache_prefetch_requested(struct path *digest_list_path,
>> +					    char *path_str)
>> +{
>> +	struct digest_cache_security *dig_sec;
>> +	bool prefetch_req = false;
>> +	char prefetch_value;
>> +	struct inode *inode;
>> +	int ret;
>> +
>> +	inode = d_backing_inode(digest_list_path->dentry);
>> +	dig_sec = digest_cache_get_security(inode);
>> +	if (unlikely(!dig_sec))
>> +		return false;
>> +
>> +	mutex_lock(&dig_sec->dig_owner_mutex);
>> +	if (dig_sec->dig_owner) {
>> +		/* Reliable test: DIR_PREFETCH set with dig_owner_mutex held. */
>> +		prefetch_req = test_bit(DIR_PREFETCH,
>> +					&dig_sec->dig_owner->flags);
>> +		mutex_unlock(&dig_sec->dig_owner_mutex);
>> +		return prefetch_req;
>> +	}
>> +	mutex_unlock(&dig_sec->dig_owner_mutex);
>> +
>> +	ret = vfs_getxattr(&nop_mnt_idmap, digest_list_path->dentry,
>> +			   XATTR_NAME_DIG_PREFETCH, &prefetch_value, 1);
>> +	if (ret == 1 && prefetch_value == '1') {
>> +		pr_debug("Prefetching has been enabled for directory %s\n",
>> +			 path_str);
>> +		prefetch_req = true;
>> +	}
>> +
>> +	return prefetch_req;
>> +}
>> +
>>   /**
>>    * digest_cache_new - Retrieve digest list file name and request digest cache
>>    * @dentry: Dentry of the inode for which the digest cache will be used
>> @@ -230,13 +294,19 @@ struct digest_cache *digest_cache_create(struct dentry *dentry,
>>    * with that file name. If security.digest_list is not found, this function
>>    * requests the creation of a digest cache on the parent directory.
>>    *
>> + * On prefetching, if the default path is a directory and if
>> + * security.digest_list is found, this function first retrieves the directory
>> + * digest cache, and then calls digest_cache_dir_lookup_filename() to retrieve
>> + * the desired digest cache in that directory.
>> + *
>>    * Return: A new digest cache on success, NULL on error.
>>    */
>>   static struct digest_cache *digest_cache_new(struct dentry *dentry)
>>   {
>>   	char filename[NAME_MAX + 1] = { 0 };
>> -	struct digest_cache *digest_cache = NULL;
>> +	struct digest_cache *digest_cache = NULL, *found;
>>   	struct path default_path;
>> +	bool prefetch_req = false;
>>   	int ret;
>>   
>>   	ret = kern_path(default_path_str, 0, &default_path);
>> @@ -273,9 +343,28 @@ static struct digest_cache *digest_cache_new(struct dentry *dentry)
>>   	pr_debug("Found %s xattr in %s, default path: %s, digest list: %s\n",
>>   		 XATTR_NAME_DIGEST_LIST, dentry->d_name.name, default_path_str,
>>   		 filename);
>> +
>> +	if (filename[0])
>> +		prefetch_req = digest_cache_prefetch_requested(&default_path,
>> +							default_path_str);
>>   create:
>> +	/* On prefetching, retrieve the directory digest cache. */
>>   	digest_cache = digest_cache_create(dentry, &default_path,
>> -					   default_path_str, filename);
>> +					   default_path_str,
>> +					   !prefetch_req ? filename : "",
>> +					   prefetch_req, false);
>> +	if (!digest_cache)
>> +		goto out;
>> +
>> +	if (prefetch_req) {
>> +		/* Find the digest cache with a matching file name. */
>> +		found = digest_cache_dir_lookup_filename(dentry, &default_path,
>> +							 digest_cache,
>> +							 filename);
>> +		/* Release ref. to the directory digest cache. */
>> +		digest_cache_put(digest_cache);
>> +		digest_cache = found;
>> +	}
>>   out:
>>   	path_put(&default_path);
>>   	return digest_cache;
>> diff --git a/security/digest_cache/populate.c b/security/digest_cache/populate.c
>> index 9c2fc2295310..17e7b011c367 100644
>> --- a/security/digest_cache/populate.c
>> +++ b/security/digest_cache/populate.c
>> @@ -143,6 +143,12 @@ int digest_cache_populate(struct digest_cache *digest_cache,
>>   		return ret;
>>   	}
>>   
>> +	/* The caller wants just to read digest lists. */
>> +	if (!digest_cache) {
>> +		ret = 0;
>> +		goto out_vfree;
>> +	}
>> +
>>   	data_len = digest_cache_strip_modsig(data, ret);
>>   
>>   	/* Digest list parsers initialize the hash table and add the digests. */
>> @@ -151,7 +157,7 @@ int digest_cache_populate(struct digest_cache *digest_cache,
>>   	if (ret < 0)
>>   		pr_debug("Error parsing digest list %s%s%s, ret: %d\n",
>>   			 path_str, filename[0] ? "/" : "", filename, ret);
>> -
>> +out_vfree:
>>   	vfree(data);
>>   	return ret;
>>   }
>> diff --git a/security/digest_cache/verif.c b/security/digest_cache/verif.c
>> index 04023240d3b4..c42ae93261e2 100644
>> --- a/security/digest_cache/verif.c
>> +++ b/security/digest_cache/verif.c
>> @@ -33,7 +33,7 @@ static void free_verif(struct digest_cache_verif *verif)
>>    * This function lets a verifier supply verification data about a digest list
>>    * being read to populate the digest cache.
>>    *
>> - * Return: Zero on success, -ENOMEM if out of memory.
>> + * Return: Zero on success, -ENOMEM if out of memory, -ENOENT on prefetching.
>>    */
>>   int digest_cache_verif_set(struct file *file, const char *verif_id, void *data,
>>   			   size_t size)
>> @@ -41,6 +41,9 @@ int digest_cache_verif_set(struct file *file, const char *verif_id, void *data,
>>   	struct digest_cache *digest_cache = digest_cache_from_file_sec(file);
>>   	struct digest_cache_verif *new_verif;
>>   
>> +	if (!digest_cache)
>> +		return -ENOENT;
>> +
>>   	/*
>>   	 * All allocations must be atomic (non-sleepable) since kprobe does not
>>   	 * allow otherwise (kprobe is needed for testing).
> 
> 
> BR, Jarkko

Powered by blists - more mailing lists