Re: [PATCH v18 12/21] dm: add finalize hook to target_type

[Fan Wu]

On 5/20/2024 5:31 AM, Mikulas Patocka wrote:
> On Fri, 17 May 2024, Fan Wu wrote:
>
> > > So, it seems that the preresume callback provides the guarantee that you
> > > looking for.
> > >
> > > > -Fan
> > >
> > > Mikulas
> >
> > Thanks for the info. I have tested and verified that the preresume() hook can
> > also work for our case.
> >
> >  From the source code
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/md/dm-ioctl.c#n1149,
> > the whole resume process appears to be:
> >
> > 1. Check if there is a new map for the device. If so, attempt to activate the
> > new map using dm_swap_table() (where the finalize() callback occurs).
> >
> > 2. Check if the device is suspended. If so, use dm_resume() (where the
> > preresume() callback occurs) to resume the device.
> >
> > 3. If a new map is activated, use dm_table_destroy() to destroy the old map.
> >
> > For our case:
> >
> > - Using the finalize() callback, the metadata of the dm-verity target inside
> > the table is attached to the mapped device every time a new table is
> > activated.
> > - Using the preresume() callback, the same metadata is attached every time the
> > device resumes from suspension.
> >
> > If I understand the code correctly, resuming from suspension is a necessary
> > step for loading a new mapping table. Thus, the preresume() callback covers
> > all conditions where the finalize() callback would be triggered.
>
> Yes.
>
> > However, the preresume() callback can also be triggered when the device
> > resumes from suspension without loading a new table, in which case there
> > is no new metadata in the table to attach to the mapped device.
>
> Yes.
>
> > In the scenario where the finalize() callback succeeds but the preresume()
> > callback fails, it seems the device will remain in a suspended state, the
> > newly activated table will be kept, and the old table will be destroyed, so it
> > seems there is no inconsistency using finalize() even preresume() potentially
> > fails.
>
> What does your security module do when the verification of the dm-verity
> hash fails? Does it halt the whole system? Does it destroy just the
> failing dm device? Or does it attempt to recover somehow from this
> situation?

I'm not sure which hash verification is being referred to here, but it 
could be either root hash signature verification or block-level hash 
verification. Our security module does not intervene in these processes, 
so the behavior remains as dm-verity currently handles it.

Within the device mapper, our security module uses the device mapper 
callback to duplicate the root hash of a dm-verity target and record the 
signature verification state of the dm-verity target, then attach this 
information to the security field of the block_device structure. This 
process can only fail if the system is out of memory.

With the root hash and signature verification state attached to the 
security field of the block device, the security system can access this 
important metadata to enforce policies. For example, these policies can 
include only allowing files from a dm-verity volume specified by its 
root hash to execute or only allowing files from a verified signed 
dm-verity volume to execute.

> > I believe both the finalize() callback proposed by Mike and the preresume()
> > callback suggested by Mikulas can work for our case. I am fine with either
> > approach, but I would like to know which one is preferred by the maintainers
> > and would appreciate an ACK for the chosen approach.
> >
> > -Fan
>
> I would prefer preresume - we shouldn't add new callbacks unless it's
> necessary.
>
> Mikulas

Thanks for the confirmation. I will switch to use prereume and I will 
send a new version later this week.

-Fan