On 28/08/2020 22:27, Tushar Sugandhi wrote:Correct. That’s one of the reasons why we don’t want to measure
Currently, dm-crypt does not take advantage of IMA measuring
capabilities, and ultimately the benefits of remote attestation.
Measure various dm-crypt constructs by calling various device-mapper
functions - dm_ima_*() that use IMA measuring capabilities. Implement
ima_measure_dm_crypt_data() to measure various dm-crypt constructs.
Ensure that ima_measure_dm_crypt_data() is non intrusive, i.e. failures
in this function and the call-stack below should not affect the core
functionality of dm-crypt.
Just my opinion, but I really do not like to add every relevant DM table option
as a harcoded value here. (But maybe it is necessary, dunno).
Why you cannot measure the whole device-mapper table as a string?
(output of STATUSTYPE_TABLE).
But it has own problems, like table can contain key etc.
Anyway with the above, the whole measurement can reside in DM core (I hope).Could you please provide more info – where in DM core? Currently, I have
But there are some problems - we can activate device with optional flagsallow_discards translates to ti->num_discard_bios. And I am already measuring it. But thanks for validating the need to measure it.
(for example allow_discards) - should this be IMA measurement?
And what about device size (you already measure offset)?Ok. I will measure device size. I will try and find where device size attribute is. But if you could point me to it, that would really help.
IMO it depends on situation (policy).Do you mean I should conditionally measure the attributes based on some policy?
It is quite often that we add performance flags later (like these no_workqueue in 5.9).The optional parameters like no_read_workqueue, no_write_workqueue, same_cpu_crypt, submit_from_crypt_cpus, are part of cc->flags. And I am already measuring cc->flags.
Some of them should be measured because there is possible security/data integrity impact.
And one note - input table accepts also a device path, but output of table is alwaysGood point. I thought about using the device path instead of
major:minor, see:
# dmsetup create test --table "0 2097152 crypt aes-cbc-essiv:sha256 9c1185a5c5e9fc54612808977ee8f548b2258d31ddadef707ba62c166051b9e3 0 /dev/sdg 0"
# dmsetup table test --showkeys
0 2097152 crypt aes-cbc-essiv:sha256 9c1185a5c5e9fc54612808977ee8f548b2258d31ddadef707ba62c166051b9e3 0 8:96 0
I think dm_table_device_name() should work here (as you use it), but major number can change
on different activations (in general) later, isn't it problem here?
(Like dynamic major for DM devices that depends on module sequence load.)
Milan
Register dm-crypt as supported data source for IMA measurement in ima.h.
A demonstrative usage of above functionality on a system:
If the IMA policy contains the following rule:
measure func=CRITICAL_DATA critical_kernel_data_sources=dm-crypt template=ima-buf
and, the following commands are used to setup a crypt target:
#key="faf453b4ee938cff2f0d2c869a0b743f59125c0a37f5bcd8f1dbbd911a78abaa"
#arg="'0 1953125 crypt aes-xts-plain64 "
#arg="$arg $key 0 "
#arg="$arg /dev/loop0 0 1 allow_discards'"
#tgt_name="test-crypt"
#cmd="dmsetup create $tgt_name --table $arg"
#eval $cmd
then, the IMA log at
/sys/kernel/security/integrity/ima/ascii_runtime_measurements should
contain the dm-crypt measurements. And, the following IMA log entry
should be added in the IMA log,
ima-buf sha1:039d8ff71918608d585adca3e5aab2e3f41f84d6
1598637500:520585536:dm-crypt:add_target
74695f6e756d5f646973636172645f62696f733d313b7065725f62696f5f646
174615f73697a653d3834383b646d7265715f73746172743d3136383b74666d
735f636f756e743d313b6f6e5f6469736b5f7461675f73697a653d303b696e7
46567726974795f69765f73697a653d303b696e746567726974795f7461675f
73697a653d303b69765f73697a653d31363b69765f6f66667365743d303b736
563746f725f73686966743d303b736563746f725f73697a653d3531323b666c
6167733d323b6369706865725f666c6167733d303b73746172743d303b6b657
95f6d61635f73697a653d303b6b65795f65787472615f73697a653d303b6b65
795f70617274733d313b6b65795f73697a653d33323b6369706865725f73747
2696e673d6165732d7874732d706c61696e36343b6465766963655f6e616d65
3d3235333a303b
where, the ascii representation of the above data is:
ti_num_discard_bios=1;per_bio_data_size=848;dmreq_start=168;
tfms_count=1;on_disk_tag_size=0;integrity_iv_size=0;
integrity_tag_size=0;iv_size=16;iv_offset=0;sector_shift=0;
sector_size=512;flags=2;cipher_flags=0;start=0;key_mac_size=0;
key_extra_size=0;key_parts=1;key_size=32;
cipher_string=aes-xts-plain64;device_name=253:0;
Some of the above values can be verified using:
#dmsetup table --showkeys
where, the output of the command should be similar to:
test-crypt: 0 1953125 crypt aes-xts-plain64
faf453b4ee938cff2f0d2c869a0b743f59125c0a37f5bcd8f1dbbd911a78abaa
0 7:0 0 1 allow_discards
Signed-off-by: Tushar Sugandhi <tusharsu@xxxxxxxxxxxxxxxxxxx>
---
drivers/md/dm-crypt.c | 171 +++++++++++++++++++++++++++++++++
security/integrity/ima/Kconfig | 3 +-
security/integrity/ima/ima.h | 1 +
3 files changed, 173 insertions(+), 2 deletions(-)
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 148960721254..47fb2ce15211 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -2529,6 +2529,8 @@ static void crypt_dtr(struct dm_target *ti)
ti->private = NULL;
+ dm_ima_exit_measurements(ti->type);
+
if (!cc)
return;
@@ -2991,6 +2993,167 @@ static int crypt_report_zones(struct dm_target *ti,
#endif
+#ifdef CONFIG_IMA
+/*
+ * append integer values to dm-crypt specific data
+ * to be measured through IMA
+ */
+static int ima_append_num_values(struct dm_target *ti,
+ const char *key,
+ long long num_val)
+{
+ char *num_str = NULL;
+ int length = 0;
+ int r = 0;
+
+ if (!ti || !key) {
+ r = -EINVAL;
+ goto error;
+ }
+
+ length = snprintf(NULL, 0, "%lld", num_val);
+ num_str = kzalloc(length + 1, GFP_KERNEL);
+ if (!num_str) {
+ r = -ENOMEM;
+ goto error;
+ }
+ snprintf(num_str, length + 1, "%lld", num_val);
+ dm_ima_append_measurement_list(ti->type,
+ key,
+ (const void *)num_str,
+ length);
+ kzfree(num_str);
+ return r;
+error:
+ DMERR("appending num values to IMA measurement list failed %d", r);
+ return r;
+}
+/*
+ * Measure dm-crypt specific data through IMA.
+ * It appends all the needed data to the list as a key-val pair using
+ * dm_ima_append_measurement_list() and internal ima_append_num_values(),
+ * and finally measures the list using dm_ima_finalize_and_measure().
+ */
+static void ima_measure_dm_crypt_data(struct dm_target *ti, const char *desc)
+{
+ int r = 0;
+ struct crypt_config *cc = NULL;
+ const char *devname = dm_table_device_name(ti->table);
+
+ if (!ti) {
+ r = -EINVAL;
+ goto out;
+ }
+
+ cc = ti->private;
+
+ if (devname) {
+ dm_ima_append_measurement_list(ti->type,
+ "device_name",
+ (const void *)devname,
+ strlen(devname));
+ }
+
+ if (cc->cipher_string) {
+ dm_ima_append_measurement_list(ti->type,
+ "cipher_string",
+ (const void *)cc->cipher_string,
+ strlen(cc->cipher_string));
+ }
+
+ if (cc->cipher_auth) {
+ dm_ima_append_measurement_list(ti->type,
+ "cipher_auth",
+ (const void *)cc->cipher_auth,
+ strlen(cc->cipher_auth));
+ }
+
+ r = ima_append_num_values(ti, "key_size", cc->key_size);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "key_parts", cc->key_parts);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "key_extra_size", cc->key_extra_size);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "key_mac_size", cc->key_mac_size);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "start", cc->start);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "cipher_flags", cc->cipher_flags);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "flags", cc->flags);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "sector_size", cc->sector_size);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "sector_shift", cc->sector_shift);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "iv_offset", cc->iv_offset);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "iv_size", cc->iv_size);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "integrity_tag_size", cc->integrity_tag_size);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "integrity_iv_size", cc->integrity_iv_size);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "on_disk_tag_size", cc->on_disk_tag_size);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "tfms_count", cc->tfms_count);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "dmreq_start", cc->dmreq_start);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "per_bio_data_size", cc->per_bio_data_size);
+ if (r)
+ goto out;
+
+ r = ima_append_num_values(ti, "ti_num_discard_bios",
+ ti->num_discard_bios);
+ if (r)
+ goto out;
+
+ dm_ima_finalize_and_measure(ti->type, desc, false);
+ return;
+
+out:
+ DMERR("IMA measurement of dm-crypt data failed %d", r);
+
+}
+#else
+static inline void ima_measure_dm_crypt_data(struct dm_target *ti,
+ const char *desc) {}
+#endif /* CONFIG_IMA */
+
+
/*
* Construct an encryption mapping:
* <cipher> [<key>|:<key_size>:<user|logon>:<key_description>] <iv_offset> <dev_path> <start>
@@ -3186,6 +3349,10 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
ti->num_flush_bios = 1;
+ dm_ima_init_measurements(ti->type);
+
+ ima_measure_dm_crypt_data(ti, "add_target");
+
return 0;
bad:
@@ -3324,6 +3491,8 @@ static void crypt_postsuspend(struct dm_target *ti)
struct crypt_config *cc = ti->private;
set_bit(DM_CRYPT_SUSPENDED, &cc->flags);
+
+ ima_measure_dm_crypt_data(ti, "post_suspend");
}
static int crypt_preresume(struct dm_target *ti)
@@ -3343,6 +3512,8 @@ static void crypt_resume(struct dm_target *ti)
struct crypt_config *cc = ti->private;
clear_bit(DM_CRYPT_SUSPENDED, &cc->flags);
+
+ ima_measure_dm_crypt_data(ti, "resume");
}
/* Message interface
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 953314d145bb..ad643cc5aad4 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -324,8 +324,7 @@ config IMA_MEASURE_ASYMMETRIC_KEYS
config IMA_QUEUE_EARLY_BOOT_DATA
bool
- depends on IMA_MEASURE_ASYMMETRIC_KEYS
- depends on SYSTEM_TRUSTED_KEYRING
+ depends on (IMA_MEASURE_ASYMMETRIC_KEYS && SYSTEM_TRUSTED_KEYRING) || DM_CRYPT
default y
config IMA_SECURE_AND_OR_TRUSTED_BOOT
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 422fe833037d..bc922aa2ff92 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -230,6 +230,7 @@ struct modsig;
#define __ima_supported_kernel_data_sources(source) \
source(MIN_SOURCE, min_source) \
+ source(DM_CRYPT, dm-crypt) \
source(MAX_SOURCE, max_source)
#define __ima_enum_stringify(ENUM, str) (#str),