[dm-devel] [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 at 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;