On Thu, 27 Jun 2019, Milan Broz wrote:
Hi,
I tried to test test the patch, two comments below.
On 19/06/2019 21:10, Jaskaran Khurana wrote:
The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.
The signature being provided for verification must verify the root hash and
must be trusted by the builtin keyring for verification to succeed.
The hash is added as a key of type "user" and the description is passed to
the kernel so it can look it up and use it for verification.
Kernel commandline parameter will indicate whether to check (only if
specified) or force (for all dm verity volumes) roothash signature
verification.
Kernel commandline: dm_verity.verify_sig=1 or 2 for check/force root hash
signature validation respectively.
1) I think the switch should be just boolean - enforce signatures for all dm-verity targets
(with default to false/off).
The rest should be handled by simple logic - if the root_hash_sig_key_desc option
is specified, the signature MUST be validated in the constructor, all errors should cause
failure (bad reference in keyring, bad signature, etc).
(Now it ignores for example bad reference to the keyring, this is quite misleading.)
If a user wants to activate a dm-verity device without a signature, just remove
optional argument referencing the signature.
(This is not possible with dm_verity.verify_sig set to true/on.)
2) All DM targets must provide the same mapping table status ("dmsetup table"
command) as initially configured.
The output of the command should be directly usable as mapping table constructor.
Your patch is missing that part, I tried to fix it, add-on patch is here
https://git.kernel.org/pub/scm/linux/kernel/git/mbroz/linux.git/commit/?h=dm-cryptsetup&id=a26c10806f5257e255b6a436713127e762935ad3
(feel free to fold it in your patch)
Thanks,
Milan
Hello Milan,
Thanks for testing and reviewing this. I will take care of the above
comments change it to a boolean and for the second point merge the add-on
patch that you shared.
Regards,
Jaskaran
