From be84f32bb2c981ca670922e047cdde1488b233de Mon Sep 17 00:00:00 2001 From: Stefan Berger Date: Fri, 22 Mar 2024 10:03:12 -0400 Subject: ima: Fix use-after-free on a dentry's dname.name ->d_name.name can change on rename and the earlier value can be freed; there are conditions sufficient to stabilize it (->d_lock on dentry, ->d_lock on its parent, ->i_rwsem exclusive on the parent's inode, rename_lock), but none of those are met at any of the sites. Take a stable snapshot of the name instead. Link: https://lore.kernel.org/all/20240202182732.GE2087318@ZenIV/ Signed-off-by: Al Viro Signed-off-by: Stefan Berger Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_api.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) (limited to 'security/integrity/ima/ima_api.c') diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index b37d043d5748..1856981e33df 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -245,8 +245,8 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file, const char *audit_cause = "failed"; struct inode *inode = file_inode(file); struct inode *real_inode = d_real_inode(file_dentry(file)); - const char *filename = file->f_path.dentry->d_name.name; struct ima_max_digest_data hash; + struct name_snapshot filename; struct kstat stat; int result = 0; int length; @@ -317,9 +317,13 @@ out: if (file->f_flags & O_DIRECT) audit_cause = "failed(directio)"; + take_dentry_name_snapshot(&filename, file->f_path.dentry); + integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, - filename, "collect_data", audit_cause, - result, 0); + filename.name.name, "collect_data", + audit_cause, result, 0); + + release_dentry_name_snapshot(&filename); } return result; } @@ -432,6 +436,7 @@ out: */ const char *ima_d_path(const struct path *path, char **pathbuf, char *namebuf) { + struct name_snapshot filename; char *pathname = NULL; *pathbuf = __getname(); @@ -445,7 +450,10 @@ const char *ima_d_path(const struct path *path, char **pathbuf, char *namebuf) } if (!pathname) { - strscpy(namebuf, path->dentry->d_name.name, NAME_MAX); + take_dentry_name_snapshot(&filename, path->dentry); + strscpy(namebuf, filename.name.name, NAME_MAX); + release_dentry_name_snapshot(&filename); + pathname = namebuf; } -- cgit v1.2.3-59-g8ed1b From 38aa3f5ac6d2de6b471ecb6e1cd878957ae7e8de Mon Sep 17 00:00:00 2001 From: "Gustavo A. R. Silva" Date: Thu, 4 Apr 2024 09:00:48 -0600 Subject: integrity: Avoid -Wflex-array-member-not-at-end warnings -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting ready to enable it globally. There is currently an object (`hdr)` in `struct ima_max_digest_data` that contains a flexible structure (`struct ima_digest_data`): struct ima_max_digest_data { struct ima_digest_data hdr; u8 digest[HASH_MAX_DIGESTSIZE]; } __packed; So, in order to avoid ending up with a flexible-array member in the middle of a struct, we use the `__struct_group()` helper to separate the flexible array from the rest of the members in the flexible structure: struct ima_digest_data { __struct_group(ima_digest_data_hdr, hdr, __packed, ... the rest of the members ); u8 digest[]; } __packed; And similarly for `struct evm_ima_xattr_data`. With the change described above, we can now declare an object of the type of the tagged `struct ima_digest_data_hdr`, without embedding the flexible array in the middle of another struct: struct ima_max_digest_data { struct ima_digest_data_hdr hdr; u8 digest[HASH_MAX_DIGESTSIZE]; } __packed; And similarly for `struct evm_digest` and `struct evm_xattr`. We also use `container_of()` whenever we need to retrieve a pointer to the flexible structure. So, with these changes, fix the following warnings: security/integrity/evm/evm.h:64:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/evm/../integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/evm/../integrity.h:68:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/ima/../integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/ima/../integrity.h:68:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/integrity.h:68:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/platform_certs/../integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/platform_certs/../integrity.h:68:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] Link: https://github.com/KSPP/linux/issues/202 Signed-off-by: Gustavo A. R. Silva Signed-off-by: Mimi Zohar --- security/integrity/evm/evm.h | 2 +- security/integrity/ima/ima_api.c | 6 ++++-- security/integrity/ima/ima_appraise.c | 4 +++- security/integrity/ima/ima_init.c | 6 ++++-- security/integrity/ima/ima_main.c | 6 ++++-- security/integrity/ima/ima_template_lib.c | 10 ++++++---- security/integrity/integrity.h | 12 +++++++++--- 7 files changed, 31 insertions(+), 15 deletions(-) (limited to 'security/integrity/ima/ima_api.c') diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h index eb1a2c343bd7..72e3341ae6f7 100644 --- a/security/integrity/evm/evm.h +++ b/security/integrity/evm/evm.h @@ -61,7 +61,7 @@ extern int evm_hmac_attrs; extern struct list_head evm_config_xattrnames; struct evm_digest { - struct ima_digest_data hdr; + struct ima_digest_data_hdr hdr; char digest[IMA_MAX_DIGEST_SIZE]; } __packed; diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 1856981e33df..3d286de231e1 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -246,6 +246,8 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file, struct inode *inode = file_inode(file); struct inode *real_inode = d_real_inode(file_dentry(file)); struct ima_max_digest_data hash; + struct ima_digest_data *hash_hdr = container_of(&hash.hdr, + struct ima_digest_data, hdr); struct name_snapshot filename; struct kstat stat; int result = 0; @@ -286,9 +288,9 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file, result = -ENODATA; } } else if (buf) { - result = ima_calc_buffer_hash(buf, size, &hash.hdr); + result = ima_calc_buffer_hash(buf, size, hash_hdr); } else { - result = ima_calc_file_hash(file, &hash.hdr); + result = ima_calc_file_hash(file, hash_hdr); } if (result && result != -EBADF && result != -EINVAL) diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 3497741caea9..656c709b974f 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -378,7 +378,9 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, } rc = calc_file_id_hash(IMA_VERITY_DIGSIG, iint->ima_hash->algo, - iint->ima_hash->digest, &hash.hdr); + iint->ima_hash->digest, + container_of(&hash.hdr, + struct ima_digest_data, hdr)); if (rc) { *cause = "sigv3-hashing-error"; *status = INTEGRITY_FAIL; diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 393f5c7912d5..4e208239a40e 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -48,12 +48,14 @@ static int __init ima_add_boot_aggregate(void) struct ima_event_data event_data = { .iint = iint, .filename = boot_aggregate_name }; struct ima_max_digest_data hash; + struct ima_digest_data *hash_hdr = container_of(&hash.hdr, + struct ima_digest_data, hdr); int result = -ENOMEM; int violation = 0; memset(iint, 0, sizeof(*iint)); memset(&hash, 0, sizeof(hash)); - iint->ima_hash = &hash.hdr; + iint->ima_hash = hash_hdr; iint->ima_hash->algo = ima_hash_algo; iint->ima_hash->length = hash_digest_size[ima_hash_algo]; @@ -70,7 +72,7 @@ static int __init ima_add_boot_aggregate(void) * is not found. */ if (ima_tpm_chip) { - result = ima_calc_boot_aggregate(&hash.hdr); + result = ima_calc_boot_aggregate(hash_hdr); if (result < 0) { audit_cause = "hashing_error"; goto err_out; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 4b4348d681a6..fff155f230a5 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -948,6 +948,8 @@ int process_buffer_measurement(struct mnt_idmap *idmap, .buf_len = size}; struct ima_template_desc *template; struct ima_max_digest_data hash; + struct ima_digest_data *hash_hdr = container_of(&hash.hdr, + struct ima_digest_data, hdr); char digest_hash[IMA_MAX_DIGEST_SIZE]; int digest_hash_len = hash_digest_size[ima_hash_algo]; int violation = 0; @@ -986,7 +988,7 @@ int process_buffer_measurement(struct mnt_idmap *idmap, if (!pcr) pcr = CONFIG_IMA_MEASURE_PCR_IDX; - iint.ima_hash = &hash.hdr; + iint.ima_hash = hash_hdr; iint.ima_hash->algo = ima_hash_algo; iint.ima_hash->length = hash_digest_size[ima_hash_algo]; @@ -997,7 +999,7 @@ int process_buffer_measurement(struct mnt_idmap *idmap, } if (buf_hash) { - memcpy(digest_hash, hash.hdr.digest, digest_hash_len); + memcpy(digest_hash, hash_hdr->digest, digest_hash_len); ret = ima_calc_buffer_hash(digest_hash, digest_hash_len, iint.ima_hash); diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 3b2cb8f1002e..4183956c53af 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -339,6 +339,8 @@ int ima_eventdigest_init(struct ima_event_data *event_data, struct ima_field_data *field_data) { struct ima_max_digest_data hash; + struct ima_digest_data *hash_hdr = container_of(&hash.hdr, + struct ima_digest_data, hdr); u8 *cur_digest = NULL; u32 cur_digestsize = 0; struct inode *inode; @@ -358,7 +360,7 @@ int ima_eventdigest_init(struct ima_event_data *event_data, if ((const char *)event_data->filename == boot_aggregate_name) { if (ima_tpm_chip) { hash.hdr.algo = HASH_ALGO_SHA1; - result = ima_calc_boot_aggregate(&hash.hdr); + result = ima_calc_boot_aggregate(hash_hdr); /* algo can change depending on available PCR banks */ if (!result && hash.hdr.algo != HASH_ALGO_SHA1) @@ -368,7 +370,7 @@ int ima_eventdigest_init(struct ima_event_data *event_data, memset(&hash, 0, sizeof(hash)); } - cur_digest = hash.hdr.digest; + cur_digest = hash_hdr->digest; cur_digestsize = hash_digest_size[HASH_ALGO_SHA1]; goto out; } @@ -379,14 +381,14 @@ int ima_eventdigest_init(struct ima_event_data *event_data, inode = file_inode(event_data->file); hash.hdr.algo = ima_template_hash_algo_allowed(ima_hash_algo) ? ima_hash_algo : HASH_ALGO_SHA1; - result = ima_calc_file_hash(event_data->file, &hash.hdr); + result = ima_calc_file_hash(event_data->file, hash_hdr); if (result) { integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, event_data->filename, "collect_data", "failed", result, 0); return result; } - cur_digest = hash.hdr.digest; + cur_digest = hash_hdr->digest; cur_digestsize = hash.hdr.length; out: return ima_eventdigest_init_common(cur_digest, cur_digestsize, diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 50d6f798e613..660f76cb69d3 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -31,19 +31,24 @@ enum evm_ima_xattr_type { }; struct evm_ima_xattr_data { - u8 type; + /* New members must be added within the __struct_group() macro below. */ + __struct_group(evm_ima_xattr_data_hdr, hdr, __packed, + u8 type; + ); u8 data[]; } __packed; /* Only used in the EVM HMAC code. */ struct evm_xattr { - struct evm_ima_xattr_data data; + struct evm_ima_xattr_data_hdr data; u8 digest[SHA1_DIGEST_SIZE]; } __packed; #define IMA_MAX_DIGEST_SIZE HASH_MAX_DIGESTSIZE struct ima_digest_data { + /* New members must be added within the __struct_group() macro below. */ + __struct_group(ima_digest_data_hdr, hdr, __packed, u8 algo; u8 length; union { @@ -57,6 +62,7 @@ struct ima_digest_data { } ng; u8 data[2]; } xattr; + ); u8 digest[]; } __packed; @@ -65,7 +71,7 @@ struct ima_digest_data { * with the maximum hash size, define ima_max_digest_data struct. */ struct ima_max_digest_data { - struct ima_digest_data hdr; + struct ima_digest_data_hdr hdr; u8 digest[HASH_MAX_DIGESTSIZE]; } __packed; -- cgit v1.2.3-59-g8ed1b From 309e2b775da8b2c28fccc4ac2621801f06920ce0 Mon Sep 17 00:00:00 2001 From: Stefan Berger Date: Fri, 23 Feb 2024 12:25:08 -0500 Subject: ima: Move file-change detection variables into new structure Move all the variables used for file change detection into a structure that can be used by IMA and EVM. Implement an inline function for storing the identification of an inode and one for detecting changes to an inode based on this new structure. Co-developed-by: Mimi Zohar Signed-off-by: Stefan Berger Signed-off-by: Mimi Zohar --- include/linux/integrity.h | 34 ++++++++++++++++++++++++++++++++++ security/integrity/ima/ima.h | 4 +--- security/integrity/ima/ima_api.c | 10 +++++----- security/integrity/ima/ima_iint.c | 2 +- security/integrity/ima/ima_main.c | 7 +++---- 5 files changed, 44 insertions(+), 13 deletions(-) (limited to 'security/integrity/ima/ima_api.c') diff --git a/include/linux/integrity.h b/include/linux/integrity.h index 459b79683783..f5842372359b 100644 --- a/include/linux/integrity.h +++ b/include/linux/integrity.h @@ -8,6 +8,7 @@ #define _LINUX_INTEGRITY_H #include +#include enum integrity_status { INTEGRITY_PASS = 0, @@ -28,4 +29,37 @@ static inline void integrity_load_keys(void) } #endif /* CONFIG_INTEGRITY */ +/* An inode's attributes for detection of changes */ +struct integrity_inode_attributes { + u64 version; /* track inode changes */ + unsigned long ino; + dev_t dev; +}; + +/* + * On stacked filesystems the i_version alone is not enough to detect file data + * or metadata change. Additional metadata is required. + */ +static inline void +integrity_inode_attrs_store(struct integrity_inode_attributes *attrs, + u64 i_version, const struct inode *inode) +{ + attrs->version = i_version; + attrs->dev = inode->i_sb->s_dev; + attrs->ino = inode->i_ino; +} + +/* + * On stacked filesystems detect whether the inode or its content has changed. + */ +static inline bool +integrity_inode_attrs_changed(const struct integrity_inode_attributes *attrs, + const struct inode *inode) +{ + return (inode->i_sb->s_dev != attrs->dev || + inode->i_ino != attrs->ino || + !inode_eq_iversion(inode, attrs->version)); +} + + #endif /* _LINUX_INTEGRITY_H */ diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 11d7c0332207..9151b5369cdc 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -175,12 +175,10 @@ struct ima_kexec_hdr { /* IMA integrity metadata associated with an inode */ struct ima_iint_cache { struct mutex mutex; /* protects: version, flags, digest */ - u64 version; /* track inode changes */ + struct integrity_inode_attributes real_inode; unsigned long flags; unsigned long measured_pcrs; unsigned long atomic_flags; - unsigned long real_ino; - dev_t real_dev; enum integrity_status ima_file_status:4; enum integrity_status ima_mmap_status:4; enum integrity_status ima_bprm_status:4; diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 3d286de231e1..984e861f6e33 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -305,11 +305,11 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file, iint->ima_hash = tmpbuf; memcpy(iint->ima_hash, &hash, length); - iint->version = i_version; - if (real_inode != inode) { - iint->real_ino = real_inode->i_ino; - iint->real_dev = real_inode->i_sb->s_dev; - } + if (real_inode == inode) + iint->real_inode.version = i_version; + else + integrity_inode_attrs_store(&iint->real_inode, i_version, + real_inode); /* Possibly temporary failure due to type of read (eg. O_DIRECT) */ if (!result) diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c index e7c9c216c1c6..e23412a2c56b 100644 --- a/security/integrity/ima/ima_iint.c +++ b/security/integrity/ima/ima_iint.c @@ -59,7 +59,7 @@ static void ima_iint_init_always(struct ima_iint_cache *iint, struct inode *inode) { iint->ima_hash = NULL; - iint->version = 0; + iint->real_inode.version = 0; iint->flags = 0UL; iint->atomic_flags = 0UL; iint->ima_file_status = INTEGRITY_UNKNOWN; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index eebf629f192e..4b215d85c14b 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -173,7 +173,7 @@ static void ima_check_last_writer(struct ima_iint_cache *iint, STATX_CHANGE_COOKIE, AT_STATX_SYNC_AS_STAT) || !(stat.result_mask & STATX_CHANGE_COOKIE) || - stat.change_cookie != iint->version) { + stat.change_cookie != iint->real_inode.version) { iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE); iint->measured_pcrs = 0; if (update) @@ -292,9 +292,8 @@ static int process_measurement(struct file *file, const struct cred *cred, if (real_inode != inode && (action & IMA_DO_MASK) && (iint->flags & IMA_DONE_MASK)) { if (!IS_I_VERSION(real_inode) || - real_inode->i_sb->s_dev != iint->real_dev || - real_inode->i_ino != iint->real_ino || - !inode_eq_iversion(real_inode, iint->version)) { + integrity_inode_attrs_changed(&iint->real_inode, + real_inode)) { iint->flags &= ~IMA_DONE_MASK; iint->measured_pcrs = 0; } -- cgit v1.2.3-59-g8ed1b