dm ima: allow targets to remeasure their state

RFC PATCH In review Thore Sommer 2 participants 11 messages Jan 6, 2022 – May 9, 2022

Subsystems (3)

Documentation Device mapper Keys-trusted

lore

Share & tools

lore

Thore Sommer

author

2022-01-06 20:35 UTC #0 lore raw

cover dm ima: allow targets to remeasure their state

The current DM IMA events do not cover the case where a device changes their attributes to indicate a state change. This adds a new event (dm_target_update) which allows targets to remeasure their table entries. The event includes the dm version, device metadata and the target data.

Currently only verity supports this event to ensure that device corruption can be detected using IMA which is useful for remote attestation.

The current implementation does not update the active table hash because it would require to rehash the entire table on every target change.

Thore Sommer (3):
dm ima: allow targets to remeasure their table entry dm verity: add support for IMA target update event dm ima: add documentation target update event

 .../admin-guide/device-mapper/dm-ima.rst      | 33 ++++++++
 drivers/md/dm-ima.c                           | 76 +++++++++++++++++++
 drivers/md/dm-ima.h                           |  2 +
 drivers/md/dm-verity-target.c                 |  6 ++
 4 files changed, 117 insertions(+)

Signature · 6 lines

2.34.1

--
dm-devel mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/dm-devel

Thore Sommer

author

2022-01-06 20:34 UTC #1 lore raw

RFC PATCH 1/3 dm ima: allow targets to remeasure their table entry

A new DM event dm_target_update is introduced for targets to remeasure their table entry. This is intended for targets that indicate security relevant events by updating one of their table entries (e.g. verity for corruption).

In the event the dm version, device metadata and target data gets measured.

This does not update the hash of the active table because it would require to rehash the whole table with all the other targets entries.

Signed-off-by Thore Sommer <[email protected]>

---
 drivers/md/dm-ima.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/md/dm-ima.h |  2 ++
 2 files changed, 78 insertions(+)

drivers/md/dm-ima.c +76 -0

index 957999998d70..3b1bb97263d9 100644
@@ -750,3 +750,79 @@ void dm_ima_measure_on_device_rename(struct mapped_device *md)
 	kfree(new_dev_name);
 	kfree(new_dev_uuid);
 }
+
+/*
+ * Give the option for targets to remeasure on state change.
+ */
+void dm_ima_measure_on_target_update(struct dm_target *ti)
+{
+	char *ima_buf = NULL, *target_metadata_buf = NULL, *target_data_buf = NULL;
+	struct dm_target *ti2;
+	size_t target_metadata_buf_len, target_data_buf_len;
+	unsigned int num_targets, target_index;
+	struct dm_table *table = ti->table;
+	struct mapped_device *md = table->md;
+	bool found = false;
+	bool noio = true;
+	int l = 0;
+
+	ima_buf = dm_ima_alloc(DM_IMA_MEASUREMENT_BUF_LEN, GFP_KERNEL, noio);
+	if (!ima_buf)
+		return;
+
+	target_metadata_buf = dm_ima_alloc(DM_IMA_TARGET_METADATA_BUF_LEN, GFP_KERNEL, noio);
+	if (!target_metadata_buf)
+		goto exit;
+
+	target_data_buf = dm_ima_alloc(DM_IMA_TARGET_DATA_BUF_LEN, GFP_KERNEL, noio);
+	if (!target_data_buf)
+		goto exit;
+
+	/*
+	 * Get the index of the target in the table.
+	 */
+	num_targets = dm_table_get_num_targets(table);
+	for (target_index = 0; target_index < num_targets; target_index++) {
+		ti2 = dm_table_get_target(table, target_index);
+		if (!ti)
+			goto exit;
+		if (ti == ti2) {
+			found = true;
+			break;
+		}
+	}
+	if (!found)
+		goto exit;
+
+	scnprintf(target_metadata_buf, DM_IMA_TARGET_METADATA_BUF_LEN,
+			  "target_index=%d,target_begin=%llu,target_len=%llu,",
+			  target_index, ti->begin, ti->len);
+	target_metadata_buf_len = strlen(target_metadata_buf);
+
+	if (ti->type->status)
+		ti->type->status(ti, STATUSTYPE_IMA, STATUSTYPE_IMA, target_data_buf,
+					DM_IMA_TARGET_DATA_BUF_LEN);
+	else
+		target_data_buf[0] = '\0';
+	target_data_buf_len = strlen(target_data_buf);
+
+	memcpy(ima_buf + l, DM_IMA_VERSION_STR, md->ima.dm_version_str_len);
+	l += md->ima.dm_version_str_len;
+
+	memcpy(ima_buf + l, md->ima.active_table.device_metadata,
+	       md->ima.active_table.device_metadata_len);
+	l += md->ima.active_table.device_metadata_len;
+
+	memcpy(ima_buf + l, target_metadata_buf, target_metadata_buf_len);
+	l += target_metadata_buf_len;
+
+	memcpy(ima_buf + l, target_data_buf, target_data_buf_len);
+
+	dm_ima_measure_data("dm_target_update", ima_buf, strlen(ima_buf), noio);
+
+exit:
+	kfree(ima_buf);
+	kfree(target_data_buf);
+	kfree(target_metadata_buf);
+}
+EXPORT_SYMBOL_GPL(dm_ima_measure_on_target_update);

drivers/md/dm-ima.h +2 -0

index b8c3b614670b..281a8b65f8a9 100644
@@ -63,6 +63,7 @@ void dm_ima_measure_on_device_resume(struct mapped_device *md, bool swap);
 void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all);
 void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map);
 void dm_ima_measure_on_device_rename(struct mapped_device *md);
+void dm_ima_measure_on_target_update(struct dm_target *ti);
 
 #else

@@ -72,6 +73,7 @@ static inline void dm_ima_measure_on_device_resume(struct mapped_device *md, boo
 static inline void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all) {}
 static inline void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map) {}
 static inline void dm_ima_measure_on_device_rename(struct mapped_device *md) {}
+static inline void dm_ima_measure_on_target_update(struct dm_target *ti) {}
 
 #endif /* CONFIG_IMA */

Signature · 1 line

2.34.1

Lakshmi Ramasubramanian

2022-05-06 20:26 UTC #2 lore raw

Re: [dm-devel] [RFC PATCH 1/3] dm ima: allow targets to remeasure their table entry

Hi Thore,

Quoted 4 lines · Thore Sommer

A new DM event dm_target_update is introduced for targets to remeasure their table entry. This is intended for targets that indicate security relevant events by updating one of their table entries (e.g. verity for corruption).

In the patch description please state the existing problem (or, limitation in the existing implementation) and then describe how the proposed change addresses the issue.

Quoted 27 lines

In the event the dm version, device metadata and target data gets measured.
This does not update the hash of the active table because it would require to rehash the whole table with all the other targets entries.
Signed-off-by Thore Sommer <[email protected]>
---
  drivers/md/dm-ima.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
  drivers/md/dm-ima.h |  2 ++
  2 files changed, 78 insertions(+)
drivers/md/dm-ima.c +6 -0
index 957999998d70..3b1bb97263d9 100644
@@ -750,3 +750,79 @@ void dm_ima_measure_on_device_rename(struct mapped_device *md)
  	kfree(new_dev_name);
  	kfree(new_dev_uuid);
  }
+
+/*
+ * Give the option for targets to remeasure on state change.
+ */
+void dm_ima_measure_on_target_update(struct dm_target *ti)
+{

drivers/md/dm-ima.c void dm_ima_measure_on_device_rename(struct mapped_device *md) · line 768

Quoted 10 lines

drivers/md/dm-ima.c +10 -0

+	char *ima_buf = NULL, *target_metadata_buf = NULL, *target_data_buf = NULL;
+	struct dm_target *ti2;
+	size_t target_metadata_buf_len, target_data_buf_len;
+	unsigned int num_targets, target_index;
+	struct dm_table *table = ti->table;
+	struct mapped_device *md = table->md;
+	bool found = false;
+	bool noio = true;
+	int l = 0;
+

Initializing "ima_buf" to NULL is not necessary since the statement below initializes it.

Quoted 10 lines

drivers/md/dm-ima.c +10 -0

+	ima_buf = dm_ima_alloc(DM_IMA_MEASUREMENT_BUF_LEN, GFP_KERNEL, noio);
+	if (!ima_buf)
+		return;
+
+	target_metadata_buf = dm_ima_alloc(DM_IMA_TARGET_METADATA_BUF_LEN, GFP_KERNEL, noio);
+	if (!target_metadata_buf)
+		goto exit;
+
+	target_data_buf = dm_ima_alloc(DM_IMA_TARGET_DATA_BUF_LEN, GFP_KERNEL, noio);
+	if (!target_data_buf)

drivers/md/dm-ima.c void dm_ima_measure_on_device_rename(struct mapped_device *md) · line 788

Quoted 10 lines

drivers/md/dm-ima.c +10 -0

+		goto exit;
+
+	/*
+	 * Get the index of the target in the table.
+	 */
+	num_targets = dm_table_get_num_targets(table);
+	for (target_index = 0; target_index < num_targets; target_index++) {
+		ti2 = dm_table_get_target(table, target_index);
+		if (!ti)
+			goto exit;

This check for "ti" can be done on function entry to avoid memory allocations and calls to dm_table_get_num_targets(), dm_table_get_target() when "ti" is NULL.

drivers/md/dm-ima.c void dm_ima_measure_on_device_rename(struct mapped_device *md) · line 799

Quoted 11 lines

drivers/md/dm-ima.c +11 -0

+		if (ti == ti2) {
+			found = true;
+			break;
+		}
+	}
+	if (!found)
+		goto exit;
+
+	scnprintf(target_metadata_buf, DM_IMA_TARGET_METADATA_BUF_LEN,
+			  "target_index=%d,target_begin=%llu,target_len=%llu,",
+			  target_index, ti->begin, ti->len);

Check return status of "scnprintf()" and handle any error.

thanks,
-lakshmi

Quoted 49 lines

drivers/md/dm-ima.c +29 -0

+	target_metadata_buf_len = strlen(target_metadata_buf);
+
+	if (ti->type->status)
+		ti->type->status(ti, STATUSTYPE_IMA, STATUSTYPE_IMA, target_data_buf,
+					DM_IMA_TARGET_DATA_BUF_LEN);
+	else
+		target_data_buf[0] = '\0';
+	target_data_buf_len = strlen(target_data_buf);
+
+	memcpy(ima_buf + l, DM_IMA_VERSION_STR, md->ima.dm_version_str_len);
+	l += md->ima.dm_version_str_len;
+
+	memcpy(ima_buf + l, md->ima.active_table.device_metadata,
+	       md->ima.active_table.device_metadata_len);
+	l += md->ima.active_table.device_metadata_len;
+
+	memcpy(ima_buf + l, target_metadata_buf, target_metadata_buf_len);
+	l += target_metadata_buf_len;
+
+	memcpy(ima_buf + l, target_data_buf, target_data_buf_len);
+
+	dm_ima_measure_data("dm_target_update", ima_buf, strlen(ima_buf), noio);
+
+exit:
+	kfree(ima_buf);
+	kfree(target_data_buf);
+	kfree(target_metadata_buf);
+}
+EXPORT_SYMBOL_GPL(dm_ima_measure_on_target_update);

drivers/md/dm-ima.h +2 -0

index b8c3b614670b..281a8b65f8a9 100644
@@ -63,6 +63,7 @@ void dm_ima_measure_on_device_resume(struct mapped_device *md, bool swap);
  void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all);
  void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map);
  void dm_ima_measure_on_device_rename(struct mapped_device *md);
+void dm_ima_measure_on_target_update(struct dm_target *ti);
 
  #else

@@ -72,6 +73,7 @@ static inline void dm_ima_measure_on_device_resume(struct mapped_device *md, boo
  static inline void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all) {}
  static inline void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map) {}
  static inline void dm_ima_measure_on_device_rename(struct mapped_device *md) {}
+static inline void dm_ima_measure_on_target_update(struct dm_target *ti) {}
 
  #endif /* CONFIG_IMA */

Thore Sommer

author

2022-05-09 09:55 UTC #3 lore raw

Re: [dm-devel] [RFC PATCH 1/3] dm ima: allow targets to remeasure their table entry

Hi Lakshmi,

Quoted 8 lines · Lakshmi Ramasubramanian

Hi Thore,
Quoted 4 lines · Thore Sommer
A new DM event dm_target_update is introduced for targets to remeasure their table entry. This is intended for targets that indicate security relevant events by updating one of their table entries (e.g. verity for corruption).
In the patch description please state the existing problem (or, limitation in the existing implementation) and then describe how the proposed change addresses the issue.

For v2 I will add the problem description from the cover letter also to this patch description.

Quoted 33 lines

Quoted 33 lines
In the event the dm version, device metadata and target data gets measured.
This does not update the hash of the active table because it would require to rehash the whole table with all the other targets entries.
Signed-off-by Thore Sommer <[email protected]>
---
drivers/md/dm-ima.c | 76 +++++++++++++++++++++++++++++++++++++++++++++ drivers/md/dm-ima.h | 2 ++
  2 files changed, 78 insertions(+)
drivers/md/dm-ima.c +0 -0
index 957999998d70..3b1bb97263d9 100644
@@ -750,3 +750,79 @@ void dm_ima_measure_on_device_rename(struct 
 mapped_device *md)
       kfree(new_dev_name);
       kfree(new_dev_uuid);
}
drivers/md/dm-ima.c +7 -0
+
+/*
+ * Give the option for targets to remeasure on state change.
+ */
+void dm_ima_measure_on_target_update(struct dm_target *ti)
+{
+    char *ima_buf = NULL, *target_metadata_buf = NULL, 
*target_data_buf = NULL;
+ struct dm_target *ti2;
+ size_t target_metadata_buf_len, target_data_buf_len;

Quoted 8 lines

Quoted 7 lines
+    unsigned int num_targets, target_index;
+    struct dm_table *table = ti->table;
+    struct mapped_device *md = table->md;
+    bool found = false;
+    bool noio = true;
+    int l = 0;
+
Initializing "ima_buf" to NULL is not necessary since the statement below initializes it.

Yes, I will remove that.

Quoted 17 lines

Quoted 17 lines
+    ima_buf = dm_ima_alloc(DM_IMA_MEASUREMENT_BUF_LEN, GFP_KERNEL, noio);
+    if (!ima_buf)
+        return;
+
+    target_metadata_buf = dm_ima_alloc(DM_IMA_TARGET_METADATA_BUF_LEN, GFP_KERNEL, noio); +    if (!target_metadata_buf)
+        goto exit;
+
+    target_data_buf = dm_ima_alloc(DM_IMA_TARGET_DATA_BUF_LEN, GFP_KERNEL, noio);
+    if (!target_data_buf)
+        goto exit;
+
+    /*

Quoted 8 lines

Quoted 7 lines
+     * Get the index of the target in the table.
+     */
+    num_targets = dm_table_get_num_targets(table);
+    for (target_index = 0; target_index < num_targets; target_index++) { +        ti2 = dm_table_get_target(table, target_index);
+        if (!ti)
+            goto exit;
This check for "ti" can be done on function entry to avoid memory allocations and calls to dm_table_get_num_targets(), dm_table_get_target() when "ti" is NULL.

I actually wanted to check for ti2, but I think this entire loop can be simplified to:

    num_targets = dm_table_get_num_targets(table);
    for (target_index = 0; target_index < num_targets; target_index++) {
        ti2 = dm_table_get_target(table, target_index);
        if (ti == ti2)
            break;
    }
    if (target_index == num_targets)
        goto exit;

This should work and because we iterate over all targets and if we iterated over all and did not find a match then we skip the measuring. I do not know if checking if "ti" is NULL is required because it should never be NULL in my understanding.

Quoted 12 lines

Quoted 12 lines
+        if (ti == ti2) {
+            found = true;
+            break;
+        }
+    }
+    if (!found)
+        goto exit;
+
+    scnprintf(target_metadata_buf, DM_IMA_TARGET_METADATA_BUF_LEN, +              "target_index=%d,target_begin=%llu,target_len=%llu,", +              target_index, ti->begin, ti->len);

Quoted 1 line

Check return status of "scnprintf()" and handle any error.

The other code in dm-ima.c also does not check for errors "scnprintf()". Do you have an example on how to handle "scnprintf()" errors?

Regards,
Thore

Quoted 46 lines

thanks,
-lakshmi

Quoted 42 lines

+    target_metadata_buf_len = strlen(target_metadata_buf); +
+    if (ti->type->status)
+        ti->type->status(ti, STATUSTYPE_IMA, STATUSTYPE_IMA, target_data_buf,
+                    DM_IMA_TARGET_DATA_BUF_LEN);
+    else
+        target_data_buf[0] = '\0';
+    target_data_buf_len = strlen(target_data_buf);

drivers/md/dm-ima.c +14 -0

+
+    memcpy(ima_buf + l, DM_IMA_VERSION_STR, md->ima.dm_version_str_len);
+    l += md->ima.dm_version_str_len;
+
+    memcpy(ima_buf + l, md->ima.active_table.device_metadata,
+           md->ima.active_table.device_metadata_len);
+    l += md->ima.active_table.device_metadata_len;
+
+    memcpy(ima_buf + l, target_metadata_buf, target_metadata_buf_len);
+    l += target_metadata_buf_len;
+
+    memcpy(ima_buf + l, target_data_buf, target_data_buf_len);
+
+    dm_ima_measure_data("dm_target_update", ima_buf, strlen(ima_buf),

noio);

drivers/md/dm-ima.c +7 -0

+
+exit:
+    kfree(ima_buf);
+    kfree(target_data_buf);
+    kfree(target_metadata_buf);
+}
+EXPORT_SYMBOL_GPL(dm_ima_measure_on_target_update);

drivers/md/dm-ima.h +1 -0

index b8c3b614670b..281a8b65f8a9 100644
@@ -63,6 +63,7 @@ void dm_ima_measure_on_device_resume(struct 
 mapped_device *md, bool swap);
   void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all);
   void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map);
   void dm_ima_measure_on_device_rename(struct mapped_device *md);
+void dm_ima_measure_on_target_update(struct dm_target *ti);
   #else

Quoted 8 lines

Quoted 8 lines
drivers/md/dm-ima.h +0 -0
@@ -72,6 +73,7 @@ static inline void 
dm_ima_measure_on_device_resume(struct mapped_device *md, boo
static inline void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all) {} static inline void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map) {} static inline void dm_ima_measure_on_device_rename(struct mapped_device *md) {} +static inline void dm_ima_measure_on_target_update(struct dm_target
*ti) {}
#endif /* CONFIG_IMA */

--
dm-devel mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/dm-devel

Lakshmi Ramasubramanian

2022-05-09 17:07 UTC #4 lore raw

Re: [dm-devel] [RFC PATCH 1/3] dm ima: allow targets to remeasure their table entry

Quoted 5 lines · Thore Sommer

Hi Lakshmi,
Quoted 2 lines · Lakshmi Ramasubramanian
Hi Thore,

Quoted 8 lines

Quoted 6 lines · Thore Sommer
Quoted 4 lines
A new DM event dm_target_update is introduced for targets to remeasure their table entry. This is intended for targets that indicate security relevant events by updating one of their table entries (e.g. verity for corruption).
In the patch description please state the existing problem (or, limitation in the existing implementation) and then describe how the proposed change addresses the issue.
For v2 I will add the problem description from the cover letter also to this patch description.

Sounds good Thore.

You may know this already - when the patch is merged, description in the cover letter is not included. Only the information in the patch description is included in the git history. So please describe the problem and how it is addressed in the patch description for each patch.

Quoted 82 lines

Quoted 34 lines
Quoted 34 lines
In the event the dm version, device metadata and target data gets measured.
This does not update the hash of the active table because it would require to rehash the whole table with all the other targets entries.
Signed-off-by Thore Sommer <[email protected]>
---
drivers/md/dm-ima.c | 76 +++++++++++++++++++++++++++++++++++++++++++++ drivers/md/dm-ima.h | 2 ++
  2 files changed, 78 insertions(+)
drivers/md/dm-ima.c +0 -0
index 957999998d70..3b1bb97263d9 100644
@@ -750,3 +750,79 @@ void dm_ima_measure_on_device_rename(struct 
 mapped_device *md)
       kfree(new_dev_name);
       kfree(new_dev_uuid);
}
drivers/md/dm-ima.c +7 -0
+
+/*
+ * Give the option for targets to remeasure on state change.
+ */
+void dm_ima_measure_on_target_update(struct dm_target *ti)
+{
+    char *ima_buf = NULL, *target_metadata_buf = NULL, 
*target_data_buf = NULL;
+ struct dm_target *ti2;
+ size_t target_metadata_buf_len, target_data_buf_len;
Quoted 8 lines
Quoted 7 lines
+    unsigned int num_targets, target_index;
+    struct dm_table *table = ti->table;
+    struct mapped_device *md = table->md;
+    bool found = false;
+    bool noio = true;
+    int l = 0;
+
Initializing "ima_buf" to NULL is not necessary since the statement below initializes it.
Yes, I will remove that.
Quoted 18 lines
Quoted 18 lines
+    ima_buf = dm_ima_alloc(DM_IMA_MEASUREMENT_BUF_LEN, GFP_KERNEL, noio);
+    if (!ima_buf)
+        return;
+
+    target_metadata_buf = dm_ima_alloc(DM_IMA_TARGET_METADATA_BUF_LEN, GFP_KERNEL, noio); +    if (!target_metadata_buf)
+        goto exit;
+
+    target_data_buf = dm_ima_alloc(DM_IMA_TARGET_DATA_BUF_LEN, GFP_KERNEL, noio);
+    if (!target_data_buf)
+        goto exit;
+
+    /*
+     * Get the index of the target in the table.
Quoted 8 lines
Quoted 7 lines
+     */
+    num_targets = dm_table_get_num_targets(table); +    for (target_index = 0; target_index < num_targets; target_index++) {
+        ti2 = dm_table_get_target(table, target_index); +        if (!ti)
+            goto exit;
This check for "ti" can be done on function entry to avoid memory allocations and calls to dm_table_get_num_targets(), dm_table_get_target() when "ti" is NULL.
I actually wanted to check for ti2, but I think this entire loop can be simplified to:
    num_targets = dm_table_get_num_targets(table);
    for (target_index = 0; target_index < num_targets; target_index++) { ti2 = dm_table_get_target(table, target_index);
        if (ti == ti2)
            break;
    }
    if (target_index == num_targets)
        goto exit;

Quoted 2 lines

This should work and because we iterate over all targets and if we iterated over all and did not find a match then we skip the measuring. I do not know if checking if "ti" is NULL is required because it should never be NULL in my understanding.

Sounds good.

Quoted 13 lines

Quoted 13 lines
Quoted 13 lines
+        if (ti == ti2) {
+            found = true;
+            break;
+        }
+    }
+    if (!found)
+        goto exit;
+
+    scnprintf(target_metadata_buf, DM_IMA_TARGET_METADATA_BUF_LEN, +              "target_index=%d,target_begin=%llu,target_len=%llu,", +              target_index, ti->begin, ti->len);

Quoted 4 lines

Quoted 1 line
Check return status of "scnprintf()" and handle any error.
The other code in dm-ima.c also does not check for errors "scnprintf()". Do you have an example on how to handle "scnprintf()" errors?

scnprintf returns the number of characters written into the buffer not including the trailing '\0'. Instead of calling strlen on target_metadata_buf, the return value of scnprintf can be used.

scnprintf(target_metadata_buf, DM_IMA_TARGET_METADATA_BUF_LEN, "target_index=%d,target_begin=%llu,target_len=%llu,",

		  target_index, ti->begin, ti->len);

target_metadata_buf_len = strlen(target_metadata_buf);

-lakshmi

Quoted 60 lines

Regards,
Thore
Quoted 55 lines
thanks,
-lakshmi
Quoted 51 lines
+    target_metadata_buf_len = strlen(target_metadata_buf); +
+    if (ti->type->status)
+        ti->type->status(ti, STATUSTYPE_IMA, STATUSTYPE_IMA, target_data_buf,
+                    DM_IMA_TARGET_DATA_BUF_LEN);
+    else
+        target_data_buf[0] = '\0';
+    target_data_buf_len = strlen(target_data_buf); +
+    memcpy(ima_buf + l, DM_IMA_VERSION_STR, md->ima.dm_version_str_len);
+    l += md->ima.dm_version_str_len;
drivers/md/dm-ima.c +11 -0
+
+    memcpy(ima_buf + l, md->ima.active_table.device_metadata,
+           md->ima.active_table.device_metadata_len);
+    l += md->ima.active_table.device_metadata_len;
+
+    memcpy(ima_buf + l, target_metadata_buf, target_metadata_buf_len);
+    l += target_metadata_buf_len;
+
+    memcpy(ima_buf + l, target_data_buf, target_data_buf_len);
+
+    dm_ima_measure_data("dm_target_update", ima_buf, 
strlen(ima_buf), noio);
drivers/md/dm-ima.c +7 -0
+
+exit:
+    kfree(ima_buf);
+    kfree(target_data_buf);
+    kfree(target_metadata_buf);
+}
+EXPORT_SYMBOL_GPL(dm_ima_measure_on_target_update);
drivers/md/dm-ima.h +1 -0
index b8c3b614670b..281a8b65f8a9 100644
@@ -63,6 +63,7 @@ void dm_ima_measure_on_device_resume(struct 
 mapped_device *md, bool swap);
   void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all);
   void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map);
   void dm_ima_measure_on_device_rename(struct mapped_device *md);
+void dm_ima_measure_on_target_update(struct dm_target *ti);
   #else
@@ -72,6 +73,7 @@ static inline void 
dm_ima_measure_on_device_resume(struct mapped_device *md, boo
static inline void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all) {} static inline void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map) {} static inline void dm_ima_measure_on_device_rename(struct mapped_device *md) {} +static inline void dm_ima_measure_on_target_update(struct dm_target
*ti) {}
#endif /* CONFIG_IMA */

Thore Sommer

author

2022-01-06 20:35 UTC #5 lore raw

RFC PATCH 2/3 dm verity: add support for IMA target update event

In reply to Thore Sommer

On first corruption the verity targets triggers a dm_target_update event. This allows other systems to check using IMA if the state of the device is still trustworthy via remote attestation.

Signed-off-by Thore Sommer <[email protected]>

---
 drivers/md/dm-verity-target.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/md/dm-verity-target.c +6 -0

index 80133aae0db3..09696e25bf1c 100644
@@ -16,6 +16,7 @@
 #include "dm-verity.h"
 #include "dm-verity-fec.h"
 #include "dm-verity-verify-sig.h"
+#include "dm-ima.h"
 #include <linux/module.h>
 #include <linux/reboot.h>
 #include <linux/scatterlist.h>

@@ -218,10 +219,15 @@ static int verity_handle_err(struct dm_verity *v, enum verity_block_type type,
 	char *envp[] = { verity_env, NULL };
 	const char *type_str = "";
 	struct mapped_device *md = dm_table_get_md(v->ti->table);
+	int old_hash_failed = v->hash_failed;
 
 	/* Corruption should be visible in device status in all modes */
 	v->hash_failed = 1;
 
+	/* Only remeasure on first failure */
+	if (!old_hash_failed)
+		dm_ima_measure_on_target_update(v->ti);
+
 	if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
 		goto out;

Signature · 6 lines

2.34.1

--
dm-devel mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/dm-devel

Lakshmi Ramasubramanian

2022-05-06 20:35 UTC #6 lore raw

Re: [dm-devel] [RFC PATCH 2/3] dm verity: add support for IMA target update event

Hi Thore,

Quoted 3 lines · Thore Sommer

On first corruption the verity targets triggers a dm_target_update event. This allows other systems to check using IMA if the state of the device is still trustworthy via remote attestation.

In the patch description please state the existing problem (or, limitation in the existing implementation) and then describe how the proposed change addresses the issue.

Quoted 20 lines

Signed-off-by Thore Sommer <[email protected]>

---
  drivers/md/dm-verity-target.c | 6 ++++++
  1 file changed, 6 insertions(+)

drivers/md/dm-verity-target.c +1 -0

index 80133aae0db3..09696e25bf1c 100644
@@ -16,6 +16,7 @@
  #include "dm-verity.h"
  #include "dm-verity-fec.h"
  #include "dm-verity-verify-sig.h"
+#include "dm-ima.h"
  #include <linux/module.h>
  #include <linux/reboot.h>
  #include <linux/scatterlist.h>

@@ -218,10 +219,15 @@ static int verity_handle_err(struct dm_verity *v, enum verity_block_type type,
  	char *envp[] = { verity_env, NULL };

drivers/md/dm-verity-target.c static int verity_handle_err(struct dm_verity *v, enum verity_block_type type, · line 229

Quoted 10 lines

drivers/md/dm-verity-target.c +4 -0

  	const char *type_str = "";
  	struct mapped_device *md = dm_table_get_md(v->ti->table);
+	int old_hash_failed = v->hash_failed;
 
  	/* Corruption should be visible in device status in all modes */
  	v->hash_failed = 1;
 
+	/* Only remeasure on first failure */
+	if (!old_hash_failed)
+		dm_ima_measure_on_target_update(v->ti);

It is not obvious to me why the call to measure on target update is not done here immediately. Updating the comment to explain the logic would help.

thanks,
-lakshmi

Quoted 4 lines

drivers/md/dm-verity-target.c +1 -0

+
  	if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
  		goto out;

Thore Sommer

author

2022-05-09 09:38 UTC #7 lore raw

Re: [dm-devel] [RFC PATCH 2/3] dm verity: add support for IMA target update event

Hi Lakshmi,

Quoted 7 lines · Lakshmi Ramasubramanian

Hi Thore,
Quoted 3 lines · Thore Sommer
On first corruption the verity targets triggers a dm_target_update event. This allows other systems to check using IMA if the state of the device is still trustworthy via remote attestation.
In the patch description please state the existing problem (or, limitation in the existing implementation) and then describe how the proposed change addresses the issue.

The problem is that we never see a IMA entry when a target gets marked as corrupted. The proposed change uses the new dm_target_update event to remeasure the table when the target table entry changes from valid to corrupted. I will add a more complete description to v2.

Quoted 23 lines

Signed-off-by Thore Sommer <[email protected]>

---
drivers/md/dm-verity-target.c | 6 ++++++

  1 file changed, 6 insertions(+)

drivers/md/dm-verity-target.c +1 -0

index 80133aae0db3..09696e25bf1c 100644
@@ -16,6 +16,7 @@
   #include "dm-verity.h"
   #include "dm-verity-fec.h"
   #include "dm-verity-verify-sig.h"
+#include "dm-ima.h"
   #include <linux/module.h>
   #include <linux/reboot.h>
   #include <linux/scatterlist.h>

@@ -218,10 +219,15 @@ static int verity_handle_err(struct dm_verity 
 *v, enum verity_block_type type,
       char *envp[] = { verity_env, NULL };
       const char *type_str = "";

Quoted 8 lines

drivers/md/dm-verity-target.c static int verity_handle_err(struct dm_verity *v, enum verity_block_type type, · line 229
Quoted 7 lines
drivers/md/dm-verity-target.c +4 -0
       struct mapped_device *md = dm_table_get_md(v->ti->table);
+    int old_hash_failed = v->hash_failed;
       /* Corruption should be visible in device status in all modes */
       v->hash_failed = 1;
+    /* Only remeasure on first failure */
+    if (!old_hash_failed)
+        dm_ima_measure_on_target_update(v->ti);
It is not obvious to me why the call to measure on target update is not done here immediately. Updating the comment to explain the logic would help.

The change should be only measured once, because otherwise we would create many IMA entries each for every corrupted block read if I understand the verity code correctly. So we need to check if the state before was valid, but we need to measure after the table was changed to corrupted (v->hash_failed = 1).

Something like this might be a bit clearer and does not use a extra variable:

+ if (!v->hash_failed)
+ v->hash_failed = 1;
+ dm_ima_measure_on_target_update(v->ti);

Regards,
Thore

Quoted 7 lines

thanks,
-lakshmi

Quoted 3 lines

drivers/md/dm-verity-target.c +1 -0

+
       if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
           goto out;

--
dm-devel mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/dm-devel

Thore Sommer

author

2022-01-06 20:34 UTC #8 lore raw

RFC PATCH 3/3 dm ima: add documentation target update event

In reply to Thore Sommer

The dm_target_update event can be triggered by targets to remeasure their state to reflect that change also in IMA.

This is event is currently only supported by verity.

Signed-off-by Thore Sommer <[email protected]>

---
 .../admin-guide/device-mapper/dm-ima.rst      | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

Documentation/admin-guide/device-mapper/dm-ima.rst +33 -0

index a4aa50a828e0..ac9418ea99d3 100644
@@ -93,6 +93,7 @@ Following device state changes will trigger IMA measurements:
  #. Device remove
  #. Table clear
  #. Device rename
+ #. Target update
 
 1. Table load:
 ---------------

@@ -321,6 +322,38 @@ The IMA measurement log has the following format for 'dm_device_rename':
  new_name=linear\=2,new_uuid=1234-5678;
  current_device_capacity=1024;
 
+6. Target update:
+------------------
+When a target changes updates its table it can trigger an remeasurement of that table.
+
+This is currently only implemented for 'verity' targets to detect measure corruption occurrences.
+Note that the active table hash of the device does not get updated.
+
+The IMA measurement log has the following format for 'dm_target_update':
+
+::
+
+ EVENT_NAME := "dm_target_update"
+ EVENT_DATA := <dm_version_str> ";" <device_active_metadata> ";" <target_data_row> ";"
+
+ dm_version_str := As described in the 'Table load' section above.
+ device_active_metadata := Device metadata that reflects the currently loaded active table.
+                           The format is same as 'device_metadata' described in the 'Table load' section above.
+ target_data_row
+ E.g: if a verity device gets corrupted then IMA ASCII measurement log will have an entry with:
+ (converted from ASCII to text for readability)
+
+ 10 1cc9c660afb7fddd1b7167f0c4e997ebca8b1c09 ima-buf sha256:e991f7692724257701c8e652682bd3246837ed2d655407b9e9f5a5b469e6c75b
+ dm_target_update
+ dm_version=4.45.0;
+ name=test,uuid=CRYPT-VERITY-e0d2a85fd61e41238174adaa32d296fe-test,major=253,minor=0,minor_count=1,num_targets=1;
+ target_index=0,target_begin=0,target_len=8,target_name=verity,target_version=1.8.0,hash_failed=C,
+ verity_version=1,data_device_name=7:1,hash_device_name=7:0,verity_algorithm=sha256,
+ root_digest=8c2eff0b45fc9815b94350f7a913683ef34085c734229bcf1345c31b07ac61b8,
+ salt=63010b7c63e28e6929a2f020dc71c97a0660a9f377a83c674a62feb01c5ca6b3,
+ ignore_zero_blocks=n,check_at_most_once=n;
+
+
 Supported targets:
 ==================

Signature · 1 line

2.34.1

Lakshmi Ramasubramanian

2022-05-06 20:16 UTC #9 lore raw

In reply to Thore Sommer

Hi Thore,

Quoted 2 lines · Thore Sommer

The current DM IMA events do not cover the case where a device changes their attributes to indicate a state change.

It would be good to state here what issue(s) are caused, if any, or what data\event we might be missing as a result of not measuring the device attribute changes. And, then state the benefits of the changes you have implemented in this patch series.

This adds a new event

Quoted 5 lines

(dm_target_update) which allows targets to remeasure their table entries. The event includes the dm version, device metadata and the target data.
Currently only verity supports this event to ensure that device corruption can be detected using IMA which is useful for remote attestation.

Using the term "currently" in this context seems to indicate that this is the current state (existing behavior) in the Linux kernel implementation. You could instead reword it to indicate that your proposed measurement change is used by verity to add support for detecting device corruption.

Quoted 3 lines

The current implementation does not update the active table hash because it would require to rehash the entire table on every target change.

Similar to the above comment - could be reworded to indicate this is the proposed change and not the existing behavior.

thanks,
-lakshmi

Quoted 12 lines

Thore Sommer (3):
dm ima: allow targets to remeasure their table entry dm verity: add support for IMA target update event dm ima: add documentation target update event
  .../admin-guide/device-mapper/dm-ima.rst      | 33 ++++++++
  drivers/md/dm-ima.c                           | 76 +++++++++++++++++++
  drivers/md/dm-ima.h                           |  2 +
  drivers/md/dm-verity-target.c                 |  6 ++
  4 files changed, 117 insertions(+)

Thore Sommer

author

2022-05-09 09:16 UTC #10 lore raw

Hi Lakshmi,

thank you for taking a closer look at this patch set.

Quoted 6 lines · Lakshmi Ramasubramanian

Hi Thore,
Quoted 2 lines · Thore Sommer
The current DM IMA events do not cover the case where a device changes their attributes to indicate a state change.
It would be good to state here what issue(s) are caused, if any, or what data\event we might be missing as a result of not measuring the device attribute changes. And, then state the benefits of the changes you have implemented in this patch series.

The existing behavior only measures the table content on target/device creation. This is fine for targets where the table content never changes, but some targets like verity, multipath and raid also use the table to indicate state changes. Those state changes are currently not measured via the device mapper IMA integration.

Measuring the state changes for verity this is especially important because after the initial creation the target is never corrupted and only marked as such when a corrupted block read. We want to measure that change to remotely attest that the correct file system is used and not tampered with. Doing that is not possible with the current features in the kernel.

Quoted 7 lines

This adds a new event
Quoted 5 lines
(dm_target_update) which allows targets to remeasure their table entries. The event includes the dm version, device metadata and the target data.
Currently only verity supports this event to ensure that device corruption can be detected using IMA which is useful for remote attestation.
Using the term "currently" in this context seems to indicate that this is the current state (existing behavior) in the Linux kernel implementation. You could instead reword it to indicate that your proposed measurement change is used by verity to add support for detecting device corruption.

Yes "currently" is confusing here, I will change it in v2.

Regards,
Thore

Quoted 21 lines

Quoted 4 lines
The current implementation does not update the active table hash because it would require to rehash the entire table on every target change.
Similar to the above comment - could be reworded to indicate this is the proposed change and not the existing behavior.
thanks,
-lakshmi
Quoted 12 lines
Thore Sommer (3):
dm ima: allow targets to remeasure their table entry dm verity: add support for IMA target update event dm ima: add documentation target update event
.../admin-guide/device-mapper/dm-ima.rst | 33 ++++++++ drivers/md/dm-ima.c | 76 +++++++++++++++++++ drivers/md/dm-ima.h | 2 + drivers/md/dm-verity-target.c | 6 ++
  4 files changed, 117 insertions(+)

--
dm-devel mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/dm-devel