| Message ID | 20200420134659.1640089-1-dbaryshkov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | dm-crypt: support using encrypted keys | expand |
On Mon, Apr 20 2020 at 9:46P -0400, Dmitry Baryshkov <dbaryshkov@gmail.com> wrote: > From: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > > Allow one to use encrypted in addition to user and login key types for > device encryption. > > Signed-off-by: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> I fixed up some issues, please see the following incremental patch, I'll get this folded in and staged for 5.8. Mike diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index 7056ab54d7dd..a0d9218d411b 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -2272,10 +2272,10 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string if (!strncmp(key_string, "logon:", key_desc - key_string + 1)) { type = &key_type_logon; - set_key = &set_key_user; + set_key = set_key_user; } else if (!strncmp(key_string, "user:", key_desc - key_string + 1)) { type = &key_type_user; - set_key = &set_key_user; + set_key = set_key_user; } else if (!strncmp(key_string, "encrypted:", key_desc - key_string + 1)) { type = &key_type_encrypted; set_key = set_key_encrypted; @@ -2287,8 +2287,7 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string if (!new_key_string) return -ENOMEM; - key = request_key(type, - key_desc + 1, NULL); + key = request_key(type, key_desc + 1, NULL); if (IS_ERR(key)) { kzfree(new_key_string); return PTR_ERR(key); -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
вт, 21 апр. 2020 г. в 21:27, Mike Snitzer <snitzer@redhat.com>: > > On Mon, Apr 20 2020 at 9:46P -0400, > Dmitry Baryshkov <dbaryshkov@gmail.com> wrote: > > > From: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > > > > Allow one to use encrypted in addition to user and login key types for > > device encryption. > > > > Signed-off-by: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > > I fixed up some issues, please see the following incremental patch, > I'll get this folded in and staged for 5.8. Thank you!
On Tue, Apr 21 2020 at 2:32P -0400, Dmitry Baryshkov <dbaryshkov@gmail.com> wrote: > вт, 21 апр. 2020 г. в 21:27, Mike Snitzer <snitzer@redhat.com>: > > > > On Mon, Apr 20 2020 at 9:46P -0400, > > Dmitry Baryshkov <dbaryshkov@gmail.com> wrote: > > > > > From: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > > > > > > Allow one to use encrypted in addition to user and login key types for > > > device encryption. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > > > > I fixed up some issues, please see the following incremental patch, > > I'll get this folded in and staged for 5.8. > > Thank you! Actually, I think this is needed too -- but please correct me if I'm wrong: diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index a4c4afc67a3d..ba4d15476862 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -2235,8 +2235,9 @@ static int set_key_user(struct crypt_config *cc, struct key *key) static int set_key_encrypted(struct crypt_config *cc, struct key *key) { - struct encrypted_key_payload *ekp = key->payload.data[0]; + const struct encrypted_key_payload *ekp; + ekp = dereference_key_locked(key); if (!ekp) return -EKEYREVOKED; -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 21/04/2020 20:27, Mike Snitzer wrote: > On Mon, Apr 20 2020 at 9:46P -0400, > Dmitry Baryshkov <dbaryshkov@gmail.com> wrote: > >> From: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> >> >> Allow one to use encrypted in addition to user and login key types for >> device encryption. >> >> Signed-off-by: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > > I fixed up some issues, please see the following incremental patch, > I'll get this folded in and staged for 5.8. And you just created hard dependence on encrypted key type... If you disable this type (CONFIG_ENCRYPTED_KEYS option), it cannot load the module anymore: ERROR: modpost: "key_type_encrypted" [drivers/md/dm-crypt.ko] undefined! We had this idea before, and this implementation in dm-crypt just requires dynamic key type loading implemented first. David Howells (cc) promised that moths ago, but apparently nothing was yet submitted (and the proof-of-concept patch no longer works). Mike, I think you should revert this patch from the tree until it is solved. Once fixed, we should also support "trusted" key type. Also please - do no forget to increase dm-crypt minor version here... Thanks, Milan -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Wed, Apr 22 2020 at 12:47pm -0400, Milan Broz <gmazyland@gmail.com> wrote: > On 21/04/2020 20:27, Mike Snitzer wrote: > > On Mon, Apr 20 2020 at 9:46P -0400, > > Dmitry Baryshkov <dbaryshkov@gmail.com> wrote: > > > >> From: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > >> > >> Allow one to use encrypted in addition to user and login key types for > >> device encryption. > >> > >> Signed-off-by: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > > > > I fixed up some issues, please see the following incremental patch, > > I'll get this folded in and staged for 5.8. > > And you just created hard dependence on encrypted key type... > > If you disable this type (CONFIG_ENCRYPTED_KEYS option), it cannot load the module anymore: > ERROR: modpost: "key_type_encrypted" [drivers/md/dm-crypt.ko] undefined! Yes, I was made aware via linux-next last night. > We had this idea before, and this implementation in dm-crypt just requires dynamic > key type loading implemented first. > > David Howells (cc) promised that moths ago, but apparently nothing was yet submitted > (and the proof-of-concept patch no longer works). Why is it so bad for dm-crypt to depend on CONFIG_ENCRYPTED_KEYS while we wait for the innovation from David? > Mike, I think you should revert this patch from the tree until it is solved. > > Once fixed, we should also support "trusted" key type. > > Also please - do no forget to increase dm-crypt minor version here... I fixed the patch up and staged it in linux-next to get test coverage, see: https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=5eb07fda05fbf87d9a37939d1cd445203c55e126 Doesn't mean I intend to keep it staged; just would like to validate the patch before tabling it (if that's what is ultimately decided for now). Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 22/04/2020 23:40, Mike Snitzer wrote: > On Wed, Apr 22 2020 at 12:47pm -0400, > Milan Broz <gmazyland@gmail.com> wrote: > >> On 21/04/2020 20:27, Mike Snitzer wrote: >>> On Mon, Apr 20 2020 at 9:46P -0400, >>> Dmitry Baryshkov <dbaryshkov@gmail.com> wrote: >>> >>>> From: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> >>>> >>>> Allow one to use encrypted in addition to user and login key types for >>>> device encryption. >>>> >>>> Signed-off-by: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> >>> >>> I fixed up some issues, please see the following incremental patch, >>> I'll get this folded in and staged for 5.8. >> >> And you just created hard dependence on encrypted key type... >> >> If you disable this type (CONFIG_ENCRYPTED_KEYS option), it cannot load the module anymore: >> ERROR: modpost: "key_type_encrypted" [drivers/md/dm-crypt.ko] undefined! > > Yes, I was made aware via linux-next last night. > >> We had this idea before, and this implementation in dm-crypt just requires dynamic >> key type loading implemented first. >> >> David Howells (cc) promised that moths ago, but apparently nothing was yet submitted >> (and the proof-of-concept patch no longer works). > > Why is it so bad for dm-crypt to depend on CONFIG_ENCRYPTED_KEYS while > we wait for the innovation from David? People need to compile kernel with specific features disabled, even without keyring sometimes. We also support whole CONFIG_KEYS disable - and it makes sense for some small appliances. In fact I had similar patch (support for encrypted+trusted keyes) for dm-crypt for months, with additional patch that loads key types per requests (so it can fail if the type is not available). It uses key_type_lookup function exported. IMO this is the way to go. So the idea is good, but please keep possibility to disable it. Additional dependencies not only cause problems above, but also can get some failures from initrd where the new module is missing (that happened several times, it is just problem that can be easily avoided). Milan -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hello, чт, 23 апр. 2020 г. в 09:47, Milan Broz <gmazyland@gmail.com>: > > On 22/04/2020 23:40, Mike Snitzer wrote: > > On Wed, Apr 22 2020 at 12:47pm -0400, > > Milan Broz <gmazyland@gmail.com> wrote: > > > >> On 21/04/2020 20:27, Mike Snitzer wrote: > >>> On Mon, Apr 20 2020 at 9:46P -0400, > >>> Dmitry Baryshkov <dbaryshkov@gmail.com> wrote: > >>> > >>>> From: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > >>>> > >>>> Allow one to use encrypted in addition to user and login key types for > >>>> device encryption. > >>>> > >>>> Signed-off-by: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > >>> > >>> I fixed up some issues, please see the following incremental patch, > >>> I'll get this folded in and staged for 5.8. > >> > >> And you just created hard dependence on encrypted key type... > >> > >> If you disable this type (CONFIG_ENCRYPTED_KEYS option), it cannot load the module anymore: > >> ERROR: modpost: "key_type_encrypted" [drivers/md/dm-crypt.ko] undefined! > > > > Yes, I was made aware via linux-next last night. I'm sorry for this. > > > >> We had this idea before, and this implementation in dm-crypt just requires dynamic > >> key type loading implemented first. > >> > >> David Howells (cc) promised that moths ago, but apparently nothing was yet submitted > >> (and the proof-of-concept patch no longer works). > > > > Why is it so bad for dm-crypt to depend on CONFIG_ENCRYPTED_KEYS while > > we wait for the innovation from David? > > People need to compile kernel with specific features disabled, even without keyring sometimes. > We also support whole CONFIG_KEYS disable - and it makes sense for some small appliances. > > In fact I had similar patch (support for encrypted+trusted keyes) for dm-crypt for months, > with additional patch that loads key types per requests (so it can fail if the type is not available). > It uses key_type_lookup function exported. IMO this is the way to go. I've also had a patch using key_type_lookup(). However I ended up dropping it becasue each key type still needs type-specific function to access key payload. Unless we also add an API to access payloads in a type-neutral way, it does not make sense. > So the idea is good, but please keep possibility to disable it. > Additional dependencies not only cause problems above, but also can get some failures from initrd > where the new module is missing (that happened several times, it is just problem > that can be easily avoided). It looks like Mike has already fixed it. So thanks a lot and sorry for the issues caused by the patch.
Hello, вт, 21 апр. 2020 г. в 21:59, Mike Snitzer <snitzer@redhat.com>: > > On Tue, Apr 21 2020 at 2:32P -0400, > Dmitry Baryshkov <dbaryshkov@gmail.com> wrote: > > > вт, 21 апр. 2020 г. в 21:27, Mike Snitzer <snitzer@redhat.com>: > > > > > > On Mon, Apr 20 2020 at 9:46P -0400, > > > Dmitry Baryshkov <dbaryshkov@gmail.com> wrote: > > > > > > > From: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > > > > > > > > Allow one to use encrypted in addition to user and login key types for > > > > device encryption. > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > > > > > > I fixed up some issues, please see the following incremental patch, > > > I'll get this folded in and staged for 5.8. > > > > Thank you! > > Actually, I think this is needed too -- but please correct me if I'm wrong: Yes, it looks like a correct change to me. > > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c > index a4c4afc67a3d..ba4d15476862 100644 > --- a/drivers/md/dm-crypt.c > +++ b/drivers/md/dm-crypt.c > @@ -2235,8 +2235,9 @@ static int set_key_user(struct crypt_config *cc, struct key *key) > > static int set_key_encrypted(struct crypt_config *cc, struct key *key) > { > - struct encrypted_key_payload *ekp = key->payload.data[0]; > + const struct encrypted_key_payload *ekp; > > + ekp = dereference_key_locked(key); > if (!ekp) > return -EKEYREVOKED; >
On Thu, Apr 23 2020 at 2:47am -0400, Milan Broz <gmazyland@gmail.com> wrote: > On 22/04/2020 23:40, Mike Snitzer wrote: > > On Wed, Apr 22 2020 at 12:47pm -0400, > > Milan Broz <gmazyland@gmail.com> wrote: > > > >> On 21/04/2020 20:27, Mike Snitzer wrote: > >>> On Mon, Apr 20 2020 at 9:46P -0400, > >>> Dmitry Baryshkov <dbaryshkov@gmail.com> wrote: > >>> > >>>> From: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > >>>> > >>>> Allow one to use encrypted in addition to user and login key types for > >>>> device encryption. > >>>> > >>>> Signed-off-by: Dmitry Baryshkov <dmitry_baryshkov@mentor.com> > >>> > >>> I fixed up some issues, please see the following incremental patch, > >>> I'll get this folded in and staged for 5.8. > >> > >> And you just created hard dependence on encrypted key type... > >> > >> If you disable this type (CONFIG_ENCRYPTED_KEYS option), it cannot load the module anymore: > >> ERROR: modpost: "key_type_encrypted" [drivers/md/dm-crypt.ko] undefined! > > > > Yes, I was made aware via linux-next last night. > > > >> We had this idea before, and this implementation in dm-crypt just requires dynamic > >> key type loading implemented first. > >> > >> David Howells (cc) promised that moths ago, but apparently nothing was yet submitted > >> (and the proof-of-concept patch no longer works). > > > > Why is it so bad for dm-crypt to depend on CONFIG_ENCRYPTED_KEYS while > > we wait for the innovation from David? > > People need to compile kernel with specific features disabled, even without keyring sometimes. > We also support whole CONFIG_KEYS disable - and it makes sense for some small appliances. > > In fact I had similar patch (support for encrypted+trusted keyes) for dm-crypt for months, > with additional patch that loads key types per requests (so it can fail if the type is not available). > It uses key_type_lookup function exported. IMO this is the way to go. > > So the idea is good, but please keep possibility to disable it. > Additional dependencies not only cause problems above, but also can get some failures from initrd > where the new module is missing (that happened several times, it is just problem > that can be easily avoided). Seems you didn't look at the fixed patch, here is what I ultimately staged yesterday: https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.8&id=a2b35bd064baf1f4e7504c23d493a3e149172dd1 dm-crypt doesn't have a hard dependency on CONFIG_ENCRYPTED_KEYS. If it is enabled support will be available, if not enabled support isn't. The concern about initramfs not having dep modules is a kernel tooling support issue. Not seeing any point withholding capabilities out of paranoia that a particular distribution's tooling (initramfs generation upon kernel update) isn't working as needed. Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 23/04/2020 16:06, Mike Snitzer wrote: > > Seems you didn't look at the fixed patch, here is what I ultimately > staged yesterday: > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.8&id=a2b35bd064baf1f4e7504c23d493a3e149172dd1 > > dm-crypt doesn't have a hard dependency on CONFIG_ENCRYPTED_KEYS. If it > is enabled support will be available, if not enabled support isn't. It is acceptable solution if you really want to push it now. Just you will repeat the same #ifdef exercise for the "trusted" key type. What we did last time, is here - it combines dynamic key type loading and #if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS) (we cannot avoid it if it is completely compiled out here) It is somewhat more readable for me and eliminates few ifdefs. Just it can be no longer applied, but the idea is here in two old patches: https://mbroz.fedorapeople.org/tmp/ Milan -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index 3df90daba89e..7056ab54d7dd 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -34,7 +34,9 @@ #include <crypto/aead.h> #include <crypto/authenc.h> #include <linux/rtnetlink.h> /* for struct rtattr and RTA macros only */ +#include <linux/key-type.h> #include <keys/user-type.h> +#include <keys/encrypted-type.h> #include <linux/device-mapper.h> @@ -2215,12 +2217,44 @@ static bool contains_whitespace(const char *str) return false; } +static int set_key_user(struct crypt_config *cc, struct key *key) +{ + const struct user_key_payload *ukp; + + ukp = user_key_payload_locked(key); + if (!ukp) + return -EKEYREVOKED; + + if (cc->key_size != ukp->datalen) + return -EINVAL; + + memcpy(cc->key, ukp->data, cc->key_size); + + return 0; +} + +static int set_key_encrypted(struct crypt_config *cc, struct key *key) +{ + struct encrypted_key_payload *ekp = key->payload.data[0]; + + if (!ekp) + return -EKEYREVOKED; + + if (cc->key_size != ekp->decrypted_datalen) + return -EINVAL; + + memcpy(cc->key, ekp->decrypted_data, cc->key_size); + + return 0; +} + static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string) { char *new_key_string, *key_desc; int ret; + struct key_type *type; struct key *key; - const struct user_key_payload *ukp; + int (*set_key)(struct crypt_config *cc, struct key *key); /* * Reject key_string with whitespace. dm core currently lacks code for @@ -2236,15 +2270,24 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string if (!key_desc || key_desc == key_string || !strlen(key_desc + 1)) return -EINVAL; - if (strncmp(key_string, "logon:", key_desc - key_string + 1) && - strncmp(key_string, "user:", key_desc - key_string + 1)) + if (!strncmp(key_string, "logon:", key_desc - key_string + 1)) { + type = &key_type_logon; + set_key = &set_key_user; + } else if (!strncmp(key_string, "user:", key_desc - key_string + 1)) { + type = &key_type_user; + set_key = &set_key_user; + } else if (!strncmp(key_string, "encrypted:", key_desc - key_string + 1)) { + type = &key_type_encrypted; + set_key = set_key_encrypted; + } else { return -EINVAL; + } new_key_string = kstrdup(key_string, GFP_KERNEL); if (!new_key_string) return -ENOMEM; - key = request_key(key_string[0] == 'l' ? &key_type_logon : &key_type_user, + key = request_key(type, key_desc + 1, NULL); if (IS_ERR(key)) { kzfree(new_key_string); @@ -2253,23 +2296,14 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string down_read(&key->sem); - ukp = user_key_payload_locked(key); - if (!ukp) { - up_read(&key->sem); - key_put(key); - kzfree(new_key_string); - return -EKEYREVOKED; - } - - if (cc->key_size != ukp->datalen) { + ret = set_key(cc, key); + if (ret < 0) { up_read(&key->sem); key_put(key); kzfree(new_key_string); - return -EINVAL; + return ret; } - memcpy(cc->key, ukp->data, cc->key_size); - up_read(&key->sem); key_put(key);