[RFC PATCH 0/3] dm ima: allow targets to remeasure their state

[RFC PATCH 0/3] dm ima: allow targets to remeasure their state

[Thore Sommer]

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(+)

-- 
2.34.1

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

[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 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 <public@thson.de>
---
 drivers/md/dm-ima.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/md/dm-ima.h |  2 ++
 2 files changed, 78 insertions(+)

diff --git a/drivers/md/dm-ima.c b/drivers/md/dm-ima.c
index 957999998d70..3b1bb97263d9 100644
--- a/drivers/md/dm-ima.c
+++ b/drivers/md/dm-ima.c
@@ -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);
diff --git a/drivers/md/dm-ima.h b/drivers/md/dm-ima.h
index b8c3b614670b..281a8b65f8a9 100644
--- a/drivers/md/dm-ima.h
+++ b/drivers/md/dm-ima.h
@@ -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 */
 
-- 
2.34.1

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

[Lakshmi Ramasubramanian]

Hi Thore,

On 1/6/2022 12:34 PM, Thore Sommer wrote:
> 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.

> 
> 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 <public@thson.de>
> ---
>   drivers/md/dm-ima.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
>   drivers/md/dm-ima.h |  2 ++
>   2 files changed, 78 insertions(+)
> 
> diff --git a/drivers/md/dm-ima.c b/drivers/md/dm-ima.c
> index 957999998d70..3b1bb97263d9 100644
> --- a/drivers/md/dm-ima.c
> +++ b/drivers/md/dm-ima.c
> @@ -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;
> +
Initializing "ima_buf" to NULL is not necessary since the statement 
below initializes it.

> +	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;
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.

> +		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

> +	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);
> diff --git a/drivers/md/dm-ima.h b/drivers/md/dm-ima.h
> index b8c3b614670b..281a8b65f8a9 100644
> --- a/drivers/md/dm-ima.h
> +++ b/drivers/md/dm-ima.h
> @@ -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 */
>   

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

[Thore Sommer]

Hi Lakshmi,

On 06.05.22 22:25, Lakshmi Ramasubramanian wrote:
> Hi Thore,
> 
> On 1/6/2022 12:34 PM, Thore Sommer wrote:
>> 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.

> 
>>
>> 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 <public@thson.de>
>> ---
>>   drivers/md/dm-ima.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/md/dm-ima.h |  2 ++
>>   2 files changed, 78 insertions(+)
>>
>> diff --git a/drivers/md/dm-ima.c b/drivers/md/dm-ima.c
>> index 957999998d70..3b1bb97263d9 100644
>> --- a/drivers/md/dm-ima.c
>> +++ b/drivers/md/dm-ima.c
>> @@ -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;
>> +
> Initializing "ima_buf" to NULL is not necessary since the statement 
> below initializes it.
Yes, I will remove that.

> 
>> +    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;
> 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.

> 
>> +        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.

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


> 
> thanks,
>   -lakshmi
> 
>> +    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);
>> diff --git a/drivers/md/dm-ima.h b/drivers/md/dm-ima.h
>> index b8c3b614670b..281a8b65f8a9 100644
>> --- a/drivers/md/dm-ima.h
>> +++ b/drivers/md/dm-ima.h
>> @@ -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 */

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

[Lakshmi Ramasubramanian]

On 5/9/2022 2:55 AM, Thore Sommer wrote:
> Hi Lakshmi,
> 
> On 06.05.22 22:25, Lakshmi Ramasubramanian wrote:
>> Hi Thore,
>>
>> On 1/6/2022 12:34 PM, Thore Sommer wrote:
>>> 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.

> 
>>
>>>
>>> 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 <public@thson.de>
>>> ---
>>>   drivers/md/dm-ima.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
>>>   drivers/md/dm-ima.h |  2 ++
>>>   2 files changed, 78 insertions(+)
>>>
>>> diff --git a/drivers/md/dm-ima.c b/drivers/md/dm-ima.c
>>> index 957999998d70..3b1bb97263d9 100644
>>> --- a/drivers/md/dm-ima.c
>>> +++ b/drivers/md/dm-ima.c
>>> @@ -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;
>>> +
>> Initializing "ima_buf" to NULL is not necessary since the statement 
>> below initializes it.
> Yes, I will remove that.
> 
>>
>>> +    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;
>> 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.
Sounds good.

> 
>>
>>> +        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.
> 
> 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

> 
> Regards,
> Thore
> 
> 
>>
>> thanks,
>>   -lakshmi
>>
>>> +    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);
>>> diff --git a/drivers/md/dm-ima.h b/drivers/md/dm-ima.h
>>> index b8c3b614670b..281a8b65f8a9 100644
>>> --- a/drivers/md/dm-ima.h
>>> +++ b/drivers/md/dm-ima.h
>>> @@ -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 */

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

[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 <public@thson.de>
---
 drivers/md/dm-verity-target.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 80133aae0db3..09696e25bf1c 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -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;
 
-- 
2.34.1

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

[Lakshmi Ramasubramanian]

Hi Thore,

On 1/6/2022 12:34 PM, Thore Sommer wrote:
> 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.

> 
> Signed-off-by: Thore Sommer <public@thson.de>
> ---
>   drivers/md/dm-verity-target.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 80133aae0db3..09696e25bf1c 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -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);
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

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

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

[Thore Sommer]

Hi Lakshmi,

On 06.05.22 22:35, Lakshmi Ramasubramanian wrote:
> Hi Thore,
> 
> On 1/6/2022 12:34 PM, Thore Sommer wrote:
>> 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.

> 
>>
>> Signed-off-by: Thore Sommer <public@thson.de>
>> ---
>>   drivers/md/dm-verity-target.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/md/dm-verity-target.c 
>> b/drivers/md/dm-verity-target.c
>> index 80133aae0db3..09696e25bf1c 100644
>> --- a/drivers/md/dm-verity-target.c
>> +++ b/drivers/md/dm-verity-target.c
>> @@ -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);
> 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

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

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

[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 <public@thson.de>
---
 .../admin-guide/device-mapper/dm-ima.rst      | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/Documentation/admin-guide/device-mapper/dm-ima.rst b/Documentation/admin-guide/device-mapper/dm-ima.rst
index a4aa50a828e0..ac9418ea99d3 100644
--- a/Documentation/admin-guide/device-mapper/dm-ima.rst
+++ b/Documentation/admin-guide/device-mapper/dm-ima.rst
@@ -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:
 ==================
 
-- 
2.34.1

Re: [RFC PATCH 0/3] dm ima: allow targets to remeasure their state

[Lakshmi Ramasubramanian]

Hi Thore,

On 1/6/2022 12:34 PM, Thore Sommer wrote:
> 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
> (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.

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

> 
> 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(+)
> 

Re: [RFC PATCH 0/3] dm ima: allow targets to remeasure their state

[Thore Sommer]

Hi Lakshmi,

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

On 06.05.22 22:16, Lakshmi Ramasubramanian wrote:
> Hi Thore,
> 
> On 1/6/2022 12:34 PM, Thore Sommer wrote:
>> 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.

> 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.
> 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


> 
>>
>> 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
> 
>>
>> 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(+)
>>