On Wed, 25 Sept 2024 at 10:03, Milan Broz <gmazyland@xxxxxxxxx> wrote: > > On 9/24/24 11:59 PM, Eric Biggers wrote: > > On Wed, Sep 25, 2024 at 12:36:01AM +0300, Jarkko Sakkinen wrote: > >> On Tue Sep 24, 2024 at 9:27 PM EEST, Mikulas Patocka wrote: > >>> > >>> > >>> On Tue, 24 Sep 2024, Jarkko Sakkinen wrote: > >>> > >>>> On Mon Sep 23, 2024 at 5:04 PM EEST, Mikulas Patocka wrote: > >>>>> > >>>>> > >>>>> On Sun, 22 Sep 2024, luca.boccassi@xxxxxxxxx wrote: > >>>>> > >>>>>> From: Luca Boccassi <bluca@xxxxxxxxxx> > >>>>>> > >>>>>> If enabled, we fallback to the platform keyring if the trusted keyring doesn't have > >>>>>> the key used to sign the roothash. But if pkcs7_verify() rejects the key for other > >>>>>> reasons, such as usage restrictions, we do not fallback. Do so. > >>>>>> > >>>>>> Follow-up for 6fce1f40e95182ebbfe1ee3096b8fc0b37903269 > >>>>>> > >>>>>> Suggested-by: Serge Hallyn <serge@xxxxxxxxxx> > >>>>>> Signed-off-by: Luca Boccassi <bluca@xxxxxxxxxx> > >>>>> > >>>>> Hi > >>>>> > >>>>> I'm not an expert in keyrings. > >>>>> > >>>>> I added keyring maintainers to the CC. Please review this patch and > >>>>> Ack/Nack it. > >>>>> > >>>>> Mikulas > >>>>> > >>>>>> --- > >>>>>> drivers/md/dm-verity-verify-sig.c | 2 +- > >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c > >>>>>> index d351d7d39c60..a9e2c6c0a33c 100644 > >>>>>> --- a/drivers/md/dm-verity-verify-sig.c > >>>>>> +++ b/drivers/md/dm-verity-verify-sig.c > >>>>>> @@ -127,7 +127,7 @@ int verity_verify_root_hash(const void *root_hash, size_t root_hash_len, > >>>>>> #endif > >>>>>> VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL); > >>>>>> #ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING > >>>>>> - if (ret == -ENOKEY) > >>>>>> + if (ret == -ENOKEY || ret == -EKEYREJECTED) > >>>>>> ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data, > >>>>>> sig_len, > >>>>>> VERIFY_USE_PLATFORM_KEYRING, > >>>>>> -- > >>>>>> 2.39.5 > >>>>>> > >>>> > >>>> I know nothing about dm-verity. What does it even do? > >>>> > >>>> BR, Jarkko > >>> > >>> dm-verity provides a read-only device with integrity checking. dm-verity > >>> stores hash for every block on the block device and checks the hash when > >>> reading the block. If the hash doesn't match, it can do one of these > >>> actions (depending on configuration): > >>> - return I/O error > >>> - try to correct the data using forward error correction > >>> - log the mismatch and do nothing > >>> - restart the machine > >>> - call panic() > >>> > >>> dm-verity is mostly used for the immutable system partition on Android > >>> phones. For more info, see > >>> Documentation/admin-guide/device-mapper/verity.rst > >>> > >>> The above patch changes the way that the signature of the root hash is > >>> verified. I have no clue whether the patch can or can't subvert system > >>> security, that's why I'd like to have some more reviews of the patch > >>> before accepting it. > >> > >> I guess someone who knows all this already should review it. > >> > >> Doesn't dm-verity have a maintainer? > > (This reminds me of a nice comment from Neil about "little walled gardens" between MD & DM. > Apparently it applies to other subsystems as well. Sorry, I couldn't resist to mention it :-) > > > This patch only affects dm-verity's in-kernel signature verification support, > > which has only been present since Linux v5.4 and is not used by Android or > > Chrome OS. The whole feature seems weird to me, and it is prone to be misused; > > signatures are best verified by trusted userspace code instead (e.g. initramfs). > > But apparently there are people who use the dm-verity in-kernel signatures. I > > think systemd has some support for it, as does the recently-upstreamed IPE LSM. > > I don't know what else. The exact semantics of the "trusted" and "platform" > > keyrings are not entirely clear to me, but given that dm-verity already trusts > > keys from both keyrings this patch seems reasonable. The people who actually > > use this feature are in the best position to make that call, though. > > When we added support for this to veritysetup (--root-hash-signature), I think it was > a requirement from Microsoft. > > Anyway, if you have a trusted key compiled-in the kernel in one keyring, I do not think > it would cause problems if stored in another. > > But it scares me that we cannot easily test userspace for this in CI, as it requires compiling > own kernel with our own keys. > > Do people use veritysetup (libcryptsetup) here, or does it run with its separate userspace tooling? This is used with libcryptsetup commonly, and often with veritysetup. It is fairly easy to test in a VM or on baremetal, it is not required to build your own kernel - that's the reason for supporting secondary+platform keyrings (the first one allows you to enroll keys in MOK, the second one for UEFI DB). We would even have a CI testing this for every PR and merge in systemd on Github, _except_ there is currently an issue (unrelated to dmverity) that happens when nesting KVM with UEFI secure boot enabled on top of HyperV, which means it cannot be used reliably on Github Actions. Once that is solved, this will be again part of the systemd CI integration tests. But it is used regularly by developers on their machines. It might not be commonly used by kernel developers, I do not know as I am not a kernel developer, but it is becoming more and more common in userspace and among image builders. For example the mkosi image builder, using systemd-repart, can very easily build distro images using signed dm verity. I am at All Systems Go and just today there were multiple talks by multiple people using dmverity images for their distros/platforms/products, especially with systemd-sysext, which is all about signed dm-verity. In 6.12 we will also have IPE which allows to enable trusted code integrity checks that cannot be trivially bypassed by other userspace processes running with root or caps. This has been, still is and will be for the foreseeable future, in use in the Azure infrastructure. Hope this provides some clarity, let me know if you need more info.
