Re: [PATCH v4 10/14] digest cache: Prefetch digest lists if requested

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>

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@xxxxxxxxxx>
---
  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





[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux