> On Feb 5, 2021, at 3:27 AM, Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > > > On 05/02/2021 01:24, Eric Snowberg wrote: >> >>> On Feb 4, 2021, at 1:26 AM, Mickaël Salaün <mic@xxxxxxxxxxx> wrote: >>> >>> >>> On 04/02/2021 04:53, Eric Snowberg wrote: >>>> >>>>> On Feb 3, 2021, at 11:49 AM, Mickaël Salaün <mic@xxxxxxxxxxx> wrote: >>>>> >>>>> This looks good to me, and it still works for my use case. Eric's >>>>> patchset only looks for asymmetric keys in the blacklist keyring, so >>>>> even if we use the same keyring we don't look for the same key types. My >>>>> patchset only allows blacklist keys (i.e. hashes, not asymmetric keys) >>>>> to be added by user space (if authenticated), but because Eric's >>>>> asymmetric keys are loaded with KEY_ALLOC_BYPASS_RESTRICTION, it should >>>>> be OK for his use case. There should be no interference between the two >>>>> new features, but I find it a bit confusing to have such distinct use of >>>>> keys from the same keyring depending on their type. >>>> >>>> I agree, it is a bit confusing. What is the thought of having a dbx >>>> keyring, similar to how the platform keyring works? >>>> >>>> https://www.spinics.net/lists/linux-security-module/msg40262.html >>>> >>>> >>>>> On 03/02/2021 17:26, David Howells wrote: >>>>>> >>>>>> Eric Snowberg <eric.snowberg@xxxxxxxxxx> wrote: >>>>>> >>>>>>> This is the fifth patch series for adding support for >>>>>>> EFI_CERT_X509_GUID entries [1]. It has been expanded to not only include >>>>>>> dbx entries but also entries in the mokx. Additionally my series to >>>>>>> preload these certificate [2] has also been included. >>>>>> >>>>>> Okay, I've tentatively applied this to my keys-next branch. However, it >>>>>> conflicts minorly with Mickaël Salaün's patches that I've previously merged on >>>>>> the same branch. Can you have a look at the merge commit >>>>>> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next&id=fdbbe7ceeb95090d09c33ce0497e0394c82aa33d >>>>>> >>>>>> (the top patch of my keys-next branch) >>>>>> >>>>>> to see if that is okay by both of you? If so, can you give it a whirl? >>>> >>>> >>>> I’m seeing a build error within blacklist_hashes_checked with >>>> one of my configs. >>>> >>>> The config is as follows: >>>> >>>> $ grep CONFIG_SYSTEM_BLACKLIST_HASH_LIST .config >>>> CONFIG_SYSTEM_BLACKLIST_HASH_LIST=“revocation_list" >>>> >>>> $ cat certs/revocation_list >>>> "tbs:1e125ea4f38acb7b29b0c495fd8e7602c2c3353b913811a9da3a2fb505c08a32” >>>> >>>> make[1]: *** No rule to make target 'revocation_list', needed by 'certs/blacklist_hashes_checked'. Stop. >>> >>> It requires an absolute path. >> >> Ok, if I use an absolute path now with CONFIG_SYSTEM_BLACKLIST_HASH_LIST >> it works. >> >>> This is to align with other variables >>> using the config_filename macro: CONFIG_SYSTEM_TRUSTED_KEYS, >>> CONFIG_MODULE_SIG_KEY and now CONFIG_SYSTEM_REVOCATION_KEYS. >> >> I just did a quick test with CONFIG_SYSTEM_TRUSTED_KEYS. It looks like we >> can use either a relative or absolute path with CONFIG_SYSTEM_TRUSTED_KEYS. >> Shouldn’t this be consistent? > > CONFIG_SYSTEM_TRUSTED_KEYS (and similar config) works with relative path > to $(srctree) not $(srctree)/certs as in your example. Correct, I had "certs" in my relative path. > We can make CONFIG_SYSTEM_BLACKLIST_HASH_LIST works with $(srctree) with > this patch: > > diff --git a/certs/Makefile b/certs/Makefile > index eb45407ff282..92a233eaa926 100644 > --- a/certs/Makefile > +++ b/certs/Makefile > @@ -14,6 +14,8 @@ $(eval $(call config_filename,SYSTEM_BLACKLIST_HASH_LIST)) > > $(obj)/blacklist_hashes.o: $(obj)/blacklist_hashes_checked > > +CFLAGS_blacklist_hashes.o += -I$(srctree) > + > targets += blacklist_hashes_checked After applying this patch, CONFIG_SYSTEM_BLACKLIST_HASH_LIST now works like the other filename macros. It seems like this would be a good addition. I have done some additional testing, I am seeing a regression. The blacklist keyring is no longer picking up any of the hashes from the dbx during boot. I backed out the merge with my changes (fdbbe7ceeb95090d09c33ce0497e0394c82aa33d) and still see the regression. I then backed out Mickaël merge (5bf1adccf5c41dbdd51d1f4de220d335d9548598) and it fixes the regression. On a x86 with the updated dbx from uefi.org, I’d expect to see 234 bin hash entries in the blacklist keyring. With the current merged code, there is none.
