linux-kernel - Re: [PATCH v18 12/21] dm: add finalize hook to target

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 20 May 2024 14:31:16 +0200 (CEST)
From: Mikulas Patocka <mpatocka@...hat.com>
To: Fan Wu <wufan@...ux.microsoft.com>
cc: Mike Snitzer <snitzer@...nel.org>, corbet@....net, zohar@...ux.ibm.com, 
    jmorris@...ei.org, serge@...lyn.com, tytso@....edu, ebiggers@...nel.org, 
    axboe@...nel.dk, agk@...hat.com, eparis@...hat.com, paul@...l-moore.com, 
    linux-doc@...r.kernel.org, linux-integrity@...r.kernel.org, 
    linux-security-module@...r.kernel.org, fsverity@...ts.linux.dev, 
    linux-block@...r.kernel.org, dm-devel@...ts.linux.dev, 
    audit@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v18 12/21] dm: add finalize hook to target_type



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