From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Biggers Date: Fri, 26 Jan 2018 00:37:48 +0000 Subject: Re: [PATCH v3] fscrypt: add support for the encrypted key type Message-Id: <20180126003748.f2uhgwhulcltyok6@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="windows-1250" Content-Transfer-Encoding: base64 List-Id: References: <20180118131359.8365-1-git@andred.net> In-Reply-To: <20180118131359.8365-1-git@andred.net> To: =?iso-8859-1?Q?Andr=E9?= Draszik Cc: linux-kernel@vger.kernel.org, Mimi Zohar , David Howells , James Morris , "Serge E. Hallyn" , "Theodore Y. Ts'o" , Jaegeuk Kim , Kees Cook , Eric Biggers , linux-integrity@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, linux-fscrypt@vger.kernel.org T24gVGh1LCBKYW4gMTgsIDIwMTggYXQgMDE6MTM6NTlQTSArMDAwMCwgQW5kcukgRHJhc3ppayB3 cm90ZToKPiAtc3RhdGljIGludCB2YWxpZGF0ZV91c2VyX2tleShzdHJ1Y3QgZnNjcnlwdF9pbmZv ICpjcnlwdF9pbmZvLAo+ICtzdGF0aWMgaW5saW5lIHN0cnVjdCBrZXkgKmZzY3J5cHRfZ2V0X2Vu Y3J5cHRlZF9rZXkoY29uc3QgY2hhciAqZGVzY3JpcHRpb24pCj4gK3sKPiArCWlmIChJU19FTkFC TEVEKENPTkZJR19FTkNSWVBURURfS0VZUykpCj4gKwkJcmV0dXJuIHJlcXVlc3Rfa2V5KCZrZXlf dHlwZV9lbmNyeXB0ZWQsIGRlc2NyaXB0aW9uLCBOVUxMKTsKPiArCXJldHVybiBFUlJfUFRSKC1F Tk9LRVkpOwo+ICt9Cj4gKwo+ICtzdGF0aWMgaW50IHZhbGlkYXRlX2tleXJpbmdfa2V5KHN0cnVj dCBmc2NyeXB0X2luZm8gKmNyeXB0X2luZm8sCj4gIAkJCXN0cnVjdCBmc2NyeXB0X2NvbnRleHQg KmN0eCwgdTggKnJhd19rZXksCj4gIAkJCWNvbnN0IGNoYXIgKnByZWZpeCwgaW50IG1pbl9rZXlz aXplKQo+ICB7Cj4gIAljaGFyICpkZXNjcmlwdGlvbjsKPiAgCXN0cnVjdCBrZXkgKmtleXJpbmdf a2V5Owo+IC0Jc3RydWN0IGZzY3J5cHRfa2V5ICptYXN0ZXJfa2V5Owo+IC0JY29uc3Qgc3RydWN0 IHVzZXJfa2V5X3BheWxvYWQgKnVrcDsKPiArCWNvbnN0IHU4ICptYXN0ZXJfa2V5Owo+ICsJdTMy IG1hc3Rlcl9rZXlfbGVuOwo+ICAJaW50IHJlczsKPiAgCj4gIAlkZXNjcmlwdGlvbiA9IGthc3By aW50ZihHRlBfTk9GUywgIiVzJSpwaE4iLCBwcmVmaXgsCj4gQEAgLTgzLDM5ICs5Myw1NSBAQCBz dGF0aWMgaW50IHZhbGlkYXRlX3VzZXJfa2V5KHN0cnVjdCBmc2NyeXB0X2luZm8gKmNyeXB0X2lu Zm8sCj4gIAkJcmV0dXJuIC1FTk9NRU07Cj4gIAo+ICAJa2V5cmluZ19rZXkgPSByZXF1ZXN0X2tl eSgma2V5X3R5cGVfbG9nb24sIGRlc2NyaXB0aW9uLCBOVUxMKTsKPiArCWlmIChrZXlyaW5nX2tl eSA9IEVSUl9QVFIoLUVOT0tFWSkpCj4gKwkJa2V5cmluZ19rZXkgPSBmc2NyeXB0X2dldF9lbmNy eXB0ZWRfa2V5KGRlc2NyaXB0aW9uKTsKPiAgCWtmcmVlKGRlc2NyaXB0aW9uKTsKPiAgCWlmIChJ U19FUlIoa2V5cmluZ19rZXkpKQo+ICAJCXJldHVybiBQVFJfRVJSKGtleXJpbmdfa2V5KTsKPiAg CWRvd25fcmVhZCgma2V5cmluZ19rZXktPnNlbSk7Cj4gIAo+IC0JaWYgKGtleXJpbmdfa2V5LT50 eXBlICE9ICZrZXlfdHlwZV9sb2dvbikgewo+ICsJaWYgKGtleXJpbmdfa2V5LT50eXBlID0gJmtl eV90eXBlX2xvZ29uKSB7Cj4gKwkJY29uc3Qgc3RydWN0IHVzZXJfa2V5X3BheWxvYWQgKnVrcDsK PiArCQljb25zdCBzdHJ1Y3QgZnNjcnlwdF9rZXkgKmZrOwo+ICsKPiArCQl1a3AgPSB1c2VyX2tl eV9wYXlsb2FkX2xvY2tlZChrZXlyaW5nX2tleSk7Cj4gKwkJaWYgKCF1a3ApIHsKPiArCQkJLyog a2V5IHdhcyByZXZva2VkIGJlZm9yZSB3ZSBhY3F1aXJlZCBpdHMgc2VtYXBob3JlICovCj4gKwkJ CXJlcyA9IC1FS0VZUkVWT0tFRDsKPiArCQkJZ290byBvdXQ7Cj4gKwkJfQo+ICsJCWlmICh1a3At PmRhdGFsZW4gIT0gc2l6ZW9mKHN0cnVjdCBmc2NyeXB0X2tleSkpIHsKPiArCQkJcmVzID0gLUVJ TlZBTDsKPiArCQkJZ290byBvdXQ7Cj4gKwkJfQo+ICsJCWZrID0gKHN0cnVjdCBmc2NyeXB0X2tl eSAqKXVrcC0+ZGF0YTsKPiArCQltYXN0ZXJfa2V5ID0gZmstPnJhdzsKPiArCQltYXN0ZXJfa2V5 X2xlbiA9IGZrLT5zaXplOwo+ICsJfSBlbHNlIGlmIChrZXlyaW5nX2tleS0+dHlwZSA9ICZrZXlf dHlwZV9lbmNyeXB0ZWQpIHsKPiArCQljb25zdCBzdHJ1Y3QgZW5jcnlwdGVkX2tleV9wYXlsb2Fk ICpla3A7Cj4gKwo+ICsJCWVrcCA9IGtleXJpbmdfa2V5LT5wYXlsb2FkLmRhdGFbMF07Cj4gKwkJ bWFzdGVyX2tleSA9IGVrcC0+ZGVjcnlwdGVkX2RhdGE7Cj4gKwkJbWFzdGVyX2tleV9sZW4gPSBl a3AtPmRlY3J5cHRlZF9kYXRhbGVuOwo+ICsJfSBlbHNlIHsKPiAgCQlwcmludGtfb25jZShLRVJO X1dBUk5JTkcKPiAtCQkJCSIlczoga2V5IHR5cGUgbXVzdCBiZSBsb2dvblxuIiwgX19mdW5jX18p Owo+ICsJCQkJIiVzOiBrZXkgdHlwZSBtdXN0IGJlIGxvZ29uIG9yIGVuY3J5cHRlZFxuIiwKPiAr CQkJCV9fZnVuY19fKTsKPiAgCQlyZXMgPSAtRU5PS0VZOwo+ICAJCWdvdG8gb3V0Owo+ICAJfQo+ IC0JdWtwID0gdXNlcl9rZXlfcGF5bG9hZF9sb2NrZWQoa2V5cmluZ19rZXkpOwo+IC0JaWYgKCF1 a3ApIHsKPiAtCQkvKiBrZXkgd2FzIHJldm9rZWQgYmVmb3JlIHdlIGFjcXVpcmVkIGl0cyBzZW1h cGhvcmUgKi8KPiAtCQlyZXMgPSAtRUtFWVJFVk9LRUQ7Cj4gLQkJZ290byBvdXQ7Cj4gLQl9Cj4g LQlpZiAodWtwLT5kYXRhbGVuICE9IHNpemVvZihzdHJ1Y3QgZnNjcnlwdF9rZXkpKSB7Cj4gLQkJ cmVzID0gLUVJTlZBTDsKPiAtCQlnb3RvIG91dDsKPiAtCX0KPiAtCW1hc3Rlcl9rZXkgPSAoc3Ry dWN0IGZzY3J5cHRfa2V5ICopdWtwLT5kYXRhOwo+ICAJQlVJTERfQlVHX09OKEZTX0FFU18xMjhf RUNCX0tFWV9TSVpFICE9IEZTX0tFWV9ERVJJVkFUSU9OX05PTkNFX1NJWkUpOwo+ICAKPiAtCWlm IChtYXN0ZXJfa2V5LT5zaXplIDwgbWluX2tleXNpemUgfHwgbWFzdGVyX2tleS0+c2l6ZSA+IEZT X01BWF9LRVlfU0laRQo+IC0JICAgIHx8IG1hc3Rlcl9rZXktPnNpemUgJSBBRVNfQkxPQ0tfU0la RSAhPSAwKSB7Cj4gKwlpZiAobWFzdGVyX2tleV9sZW4gPCBtaW5fa2V5c2l6ZSB8fCBtYXN0ZXJf a2V5X2xlbiA+IEZTX01BWF9LRVlfU0laRQo+ICsJICAgIHx8IG1hc3Rlcl9rZXlfbGVuICUgQUVT X0JMT0NLX1NJWkUgIT0gMCkgewo+ICAJCXByaW50a19vbmNlKEtFUk5fV0FSTklORwo+IC0JCQkJ IiVzOiBrZXkgc2l6ZSBpbmNvcnJlY3Q6ICVkXG4iLAo+IC0JCQkJX19mdW5jX18sIG1hc3Rlcl9r ZXktPnNpemUpOwo+ICsJCQkJIiVzOiBrZXkgc2l6ZSBpbmNvcnJlY3Q6ICV1XG4iLAo+ICsJCQkJ X19mdW5jX18sIG1hc3Rlcl9rZXlfbGVuKTsKPiAgCQlyZXMgPSAtRU5PS0VZOwo+ICAJCWdvdG8g b3V0Owo+ICAJfQo+IC0JcmVzID0gZGVyaXZlX2tleV9hZXMoY3R4LT5ub25jZSwgbWFzdGVyX2tl eSwgcmF3X2tleSk7Cj4gKwlyZXMgPSBkZXJpdmVfa2V5X2FlcyhjdHgtPm5vbmNlLCBtYXN0ZXJf a2V5LCBtYXN0ZXJfa2V5X2xlbiwgcmF3X2tleSk7Cj4gKwo+ICBvdXQ6Cj4gIAl1cF9yZWFkKCZr ZXlyaW5nX2tleS0+c2VtKTsKPiAgCWtleV9wdXQoa2V5cmluZ19rZXkpOwo+IEBAIC0zMDIsMTIg KzMyOCwxMiBAQCBpbnQgZnNjcnlwdF9nZXRfZW5jcnlwdGlvbl9pbmZvKHN0cnVjdCBpbm9kZSAq aW5vZGUpCj4gIAlpZiAoIXJhd19rZXkpCj4gIAkJZ290byBvdXQ7Cj4gIAo+IC0JcmVzID0gdmFs aWRhdGVfdXNlcl9rZXkoY3J5cHRfaW5mbywgJmN0eCwgcmF3X2tleSwgRlNfS0VZX0RFU0NfUFJF RklYLAo+IC0JCQkJa2V5c2l6ZSk7Cj4gKwlyZXMgPSB2YWxpZGF0ZV9rZXlyaW5nX2tleShjcnlw dF9pbmZvLCAmY3R4LCByYXdfa2V5LAo+ICsJCQkJICAgRlNfS0VZX0RFU0NfUFJFRklYLCBrZXlz aXplKTsKPiAgCWlmIChyZXMgJiYgaW5vZGUtPmlfc2ItPnNfY29wLT5rZXlfcHJlZml4KSB7Cj4g LQkJaW50IHJlczIgPSB2YWxpZGF0ZV91c2VyX2tleShjcnlwdF9pbmZvLCAmY3R4LCByYXdfa2V5 LAo+IC0JCQkJCSAgICAgaW5vZGUtPmlfc2ItPnNfY29wLT5rZXlfcHJlZml4LAo+IC0JCQkJCSAg ICAga2V5c2l6ZSk7Cj4gKwkJaW50IHJlczIgPSB2YWxpZGF0ZV9rZXlyaW5nX2tleShjcnlwdF9p bmZvLCAmY3R4LCByYXdfa2V5LAo+ICsJCQkJCQlpbm9kZS0+aV9zYi0+c19jb3AtPmtleV9wcmVm aXgsCj4gKwkJCQkJCWtleXNpemUpOwo+ICAJCWlmIChyZXMyKSB7Cj4gIAkJCWlmIChyZXMyID0g LUVOT0tFWSkKPiAgCQkJCXJlcyA9IC1FTk9LRVk7Cj4gLS0gCj4gMi4xNS4xCj4gCj4gLS0KPiBU byB1bnN1YnNjcmliZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUg a2V5cmluZ3MiIGluCj4gdGhlIGJvZHkgb2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2VyLmtl cm5lbC5vcmcKPiBNb3JlIG1ham9yZG9tbyBpbmZvIGF0ICBodHRwOi8vdmdlci5rZXJuZWwub3Jn L21ham9yZG9tby1pbmZvLmh0bWwKClRoZSBjb2RlIGNoYW5nZXMgaGVyZSBsb29rIGZpbmUsIGJ1 dCB1bmZvcnR1bmF0ZWx5IHRoZXJlIGlzIGEgbG9jayBvcmRlcmluZwpwcm9ibGVtIGV4cG9zZWQg YnkgdXNpbmcgYSBrZXkgdHlwZSB0aGF0IGltcGxlbWVudHMgdGhlIGtleV90eXBlIC0+cmVhZCgp Cm1ldGhvZC4gIFRoZSBwcm9ibGVtIGlzIHRoYXQgZW5jcnlwdGVkX3JlYWQoKSBjYW4gdGFrZSBh IHBhZ2UgZmF1bHQgZHVyaW5nCmNvcHlfdG9fdXNlcigpIHdoaWxlIGhvbGRpbmcgdGhlIGtleSBz ZW1hcGhvcmUsIHdoaWNoIHRoZW4gKHdpdGggZXh0NCkgY2FuCnRyaWdnZXIgdGhlIHN0YXJ0IG9m IGEgamJkMiB0cmFuc2FjdGlvbi4gIE1lYW53aGlsZQpmc2NyeXB0X2dldF9lbmNyeXB0aW9uX2lu Zm8oKSBjYW4gYmUgY2FsbGVkIGZyb20gd2l0aGluIGEgamJkMiB0cmFuc2FjdGlvbiB2aWEKZnNj cnlwdF9pbmhlcml0X2NvbnRleHQoKSwgd2hpY2ggcmVzdWx0cyBpbiBhIGRpZmZlcmVudCBsb2Nr aW5nIG9yZGVyLiAgV2UKcHJvYmFibHkgd2lsbCBuZWVkIHRvIGZpeCB0aGlzIGJ5IHJlbW92aW5n IHRoZSBjYWxsIHRvCmZzY3J5cHRfZ2V0X2VuY3J5cHRpb25faW5mbygpIGZyb20gZnNjcnlwdF9p bmhlcml0X2NvbnRleHQoKS4KCkkndmUgcGFzdGVkIHRoZSBsb2NrZGVwIHJlcG9ydCBJIGdvdCBi ZWxvdzoKClsgIDQzMi43MDUzMzldIApbICA0MzIuNzA1NjgxXSA9PT09PT09PT09PT09PT09PT09 PT09PT09PT0KWyAgNDMyLjcwNzAxNV0gV0FSTklORzogcG9zc2libGUgY2lyY3VsYXIgbG9ja2lu ZyBkZXBlbmRlbmN5IGRldGVjdGVkClsgIDQzMi43MDgxNTVdIDQuMTUuMC1yYzgtbmV4dC0yMDE4 MDExOS0wMDAxOS1nYWI3ZWM0NmI2OTM2ICMzMSBOb3QgdGFpbnRlZApbICA0MzIuNzA5MzY1XSAt LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0KWyAg NDMyLjcxMDQ4Ml0ga2V5Y3RsLzg3NyBpcyB0cnlpbmcgdG8gYWNxdWlyZSBsb2NrOgpbICA0MzIu NzExMjg2XSAgKCZtbS0+bW1hcF9zZW0peysrKyt9LCBhdDogWzwwMDAwMDAwMGU4YmJhMWM3Pl0g X19taWdodF9mYXVsdCsweGNlLzB4MWEwClsgIDQzMi43MTI2ODhdIApbICA0MzIuNzEyNjg4XSBi dXQgdGFzayBpcyBhbHJlYWR5IGhvbGRpbmcgbG9jazoKWyAgNDMyLjcxMzY4NF0gICgmdHlwZS0+ bG9ja19jbGFzcyMyKXsrKysrfSwgYXQ6IFs8MDAwMDAwMDA4Y2NmYTE1Nj5dIGtleWN0bF9yZWFk X2tleSsweDE3NC8weDI0MApbICA0MzIuNzE1MTc2XSAKWyAgNDMyLjcxNTE3Nl0gd2hpY2ggbG9j ayBhbHJlYWR5IGRlcGVuZHMgb24gdGhlIG5ldyBsb2NrLgpbICA0MzIuNzE1MTc2XSAKWyAgNDMy LjcxNjYwMV0gClsgIDQzMi43MTY2MDFdIHRoZSBleGlzdGluZyBkZXBlbmRlbmN5IGNoYWluIChp biByZXZlcnNlIG9yZGVyKSBpczoKWyAgNDMyLjcxNzkwMV0gClsgIDQzMi43MTc5MDFdIC0+ICMz ICgmdHlwZS0+bG9ja19jbGFzcyMyKXsrKysrfToKWyAgNDMyLjcxODkyNF0gICAgICAgIGxvY2tf YWNxdWlyZSsweGFhLzB4MTcwClsgIDQzMi43MTk2MzJdICAgICAgICBkb3duX3JlYWQrMHgzNy8w eGEwClsgIDQzMi43MjAyOThdICAgICAgICB2YWxpZGF0ZV9rZXlyaW5nX2tleS5pc3JhLjErMHg5 Ny8weDQxMApbICA0MzIuNzIxMjE4XSAgICAgICAgZnNjcnlwdF9nZXRfZW5jcnlwdGlvbl9pbmZv KzB4NzI0LzB4MTIwMApbICA0MzIuNzIyMjMzXSAgICAgICAgZnNjcnlwdF9pbmhlcml0X2NvbnRl eHQrMHgyYzEvMHgzMzAKWyAgNDMyLjcyMzA2N10gICAgICAgIF9fZXh0NF9uZXdfaW5vZGUrMHgz NGY1LzB4NDRkMApbICA0MzIuNzIzODMwXSAgICAgICAgZXh0NF9jcmVhdGUrMHgxOTUvMHg0YTAK WyAgNDMyLjcyNDQ5OV0gICAgICAgIGxvb2t1cF9vcGVuKzB4YmUyLzB4MTQwMApbICA0MzIuNzI1 MTg1XSAgICAgICAgcGF0aF9vcGVuYXQrMHhiMzEvMHgxZTUwClsgIDQzMi43MjU4NzZdICAgICAg ICBkb19maWxwX29wZW4rMHgxNzUvMHgyNTAKWyAgNDMyLjcyNjU3Ml0gICAgICAgIGRvX3N5c19v cGVuKzB4MjE5LzB4M2YwClsgIDQzMi43MjczMTJdICAgICAgICBlbnRyeV9TWVNDQUxMXzY0X2Zh c3RwYXRoKzB4MWUvMHg4YgpbICA0MzIuNzI4MTUzXSAKWyAgNDMyLjcyODE1M10gLT4gIzIgKGpi ZDJfaGFuZGxlKXsuKy4rfToKWyAgNDMyLjcyOTAwOF0gICAgICAgIGxvY2tfYWNxdWlyZSsweGFh LzB4MTcwClsgIDQzMi43Mjk2ODBdICAgICAgICBzdGFydF90aGlzX2hhbmRsZSsweDQ3Yi8weDEw MjAKWyAgNDMyLjczMDQ2M10gICAgICAgIGpiZDJfX2pvdXJuYWxfc3RhcnQrMHgzMmUvMHg1YzAK WyAgNDMyLjczMTI3Nl0gICAgICAgIF9fZXh0NF9qb3VybmFsX3N0YXJ0X3NiKzB4YTgvMHgxOTAK WyAgNDMyLjczMjE4M10gICAgICAgIGV4dDRfdHJ1bmNhdGUrMHg0ZGQvMHhkMDAKWyAgNDMyLjcz Mjg2OF0gICAgICAgIGV4dDRfc2V0YXR0cisweGE2Mi8weDFhYzAKWyAgNDMyLjczMzUyOF0gICAg ICAgIG5vdGlmeV9jaGFuZ2UrMHg2NzIvMHhkYjAKWyAgNDMyLjczNDE5OF0gICAgICAgIGRvX3Ry dW5jYXRlKzB4MTExLzB4MWMwClsgIDQzMi43MzQ4MzFdICAgICAgICBwYXRoX29wZW5hdCsweGVl Ni8weDFlNTAKWyAgNDMyLjczNTQ3Nl0gICAgICAgIGRvX2ZpbHBfb3BlbisweDE3NS8weDI1MApb ICA0MzIuNzM2MTQ5XSAgICAgICAgZG9fc3lzX29wZW4rMHgyMTkvMHgzZjAKWyAgNDMyLjczNjc4 NV0gICAgICAgIGVudHJ5X1NZU0NBTExfNjRfZmFzdHBhdGgrMHgxZS8weDhiClsgIDQzMi43Mzc1 NzZdIApbICA0MzIuNzM3NTc2XSAtPiAjMSAoJmVpLT5pX21tYXBfc2VtKXsrKysrfToKWyAgNDMy LjczODUwOF0gICAgICAgIGxvY2tfYWNxdWlyZSsweGFhLzB4MTcwClsgIDQzMi43MzkxNDhdICAg ICAgICBkb3duX3JlYWQrMHgzNy8weGEwClsgIDQzMi43Mzk3MzVdICAgICAgICBleHQ0X2ZpbGVt YXBfZmF1bHQrMHg3Yi8weGIwClsgIDQzMi43NDA0NDZdICAgICAgICBfX2RvX2ZhdWx0KzB4N2Ev MHgyMDAKWyAgNDMyLjc0MTA2MF0gICAgICAgIF9faGFuZGxlX21tX2ZhdWx0KzB4NmUwLzB4MjA0 MApbICA0MzIuNzQxODAxXSAgICAgICAgaGFuZGxlX21tX2ZhdWx0KzB4MTk0LzB4MzIwClsgIDQz Mi43NDI0OTRdICAgICAgICBfX2RvX3BhZ2VfZmF1bHQrMHg0ZTEvMHhhNzAKWyAgNDMyLjc0MzE4 NF0gICAgICAgIHBhZ2VfZmF1bHQrMHgyYy8weDYwClsgIDQzMi43NDM3ODRdICAgICAgICBfX2Ns ZWFyX3VzZXIrMHgzZC8weDYwClsgIDQzMi43NDQ0MDldICAgICAgICBjbGVhcl91c2VyKzB4NzYv MHhhMApbICA0MzIuNzQ1MDA5XSAgICAgICAgbG9hZF9lbGZfYmluYXJ5KzB4MmJmNi8weDJmMTAK WyAgNDMyLjc0NTc1M10gICAgICAgIHNlYXJjaF9iaW5hcnlfaGFuZGxlcisweDEzNi8weDQ1MApb ICA0MzIuNzQ2NTIyXSAgICAgICAgZG9fZXhlY3ZlYXRfY29tbW9uLmlzcmEuMTIrMHgxMjQxLzB4 MTljMApbICA0MzIuNzQ3MzM5XSAgICAgICAgZG9fZXhlY3ZlKzB4MmMvMHg0MApbICA0MzIuNzQ3 ODgxXSAgICAgICAgdHJ5X3RvX3J1bl9pbml0X3Byb2Nlc3MrMHgxMi8weDQwClsgIDQzMi43NDg2 MTFdICAgICAgICBrZXJuZWxfaW5pdCsweGYyLzB4MTgwClsgIDQzMi43NDkxOTBdICAgICAgICBy ZXRfZnJvbV9mb3JrKzB4M2EvMHg1MApbICA0MzIuNzQ5Nzg1XSAKWyAgNDMyLjc0OTc4NV0gLT4g IzAgKCZtbS0+bW1hcF9zZW0peysrKyt9OgpbICA0MzIuNzUwNTc2XSAgICAgICAgX19sb2NrX2Fj cXVpcmUrMHg4ZDEvMHgxMmQwClsgIDQzMi43NTEyMzJdICAgICAgICBsb2NrX2FjcXVpcmUrMHhh YS8weDE3MApbICA0MzIuNzUxODQ4XSAgICAgICAgX19taWdodF9mYXVsdCsweDEyYi8weDFhMApb ICA0MzIuNzUyNDc4XSAgICAgICAgX2NvcHlfdG9fdXNlcisweDI3LzB4YzAKWyAgNDMyLjc1MzA4 MF0gICAgICAgIGVuY3J5cHRlZF9yZWFkKzB4NTRkLzB4N2IwClsgIDQzMi43NTM3MzRdICAgICAg ICBrZXljdGxfcmVhZF9rZXkrMHgxZjIvMHgyNDAKWyAgNDMyLjc1NDM5Nl0gICAgICAgIFN5U19r ZXljdGwrMHgxODQvMHgyYTAKWyAgNDMyLjc1NDk5NV0gICAgICAgIGVudHJ5X1NZU0NBTExfNjRf ZmFzdHBhdGgrMHgxZS8weDhiClsgIDQzMi43NTU3NzhdIApbICA0MzIuNzU1Nzc4XSBvdGhlciBp bmZvIHRoYXQgbWlnaHQgaGVscCB1cyBkZWJ1ZyB0aGlzOgpbICA0MzIuNzU1Nzc4XSAKWyAgNDMy Ljc1Njk3Ml0gQ2hhaW4gZXhpc3RzIG9mOgpbICA0MzIuNzU2OTcyXSAgICZtbS0+bW1hcF9zZW0g LS0+IGpiZDJfaGFuZGxlIC0tPiAmdHlwZS0+bG9ja19jbGFzcyMyClsgIDQzMi43NTY5NzJdIApb ICA0MzIuNzU4NDk4XSAgUG9zc2libGUgdW5zYWZlIGxvY2tpbmcgc2NlbmFyaW86ClsgIDQzMi43 NTg0OThdIApbICA0MzIuNzU5MzAyXSAgICAgICAgQ1BVMCAgICAgICAgICAgICAgICAgICAgQ1BV MQpbICA0MzIuNzU5OTE5XSAgICAgICAgLS0tLSAgICAgICAgICAgICAgICAgICAgLS0tLQpbICA0 MzIuNzYwNTM2XSAgIGxvY2soJnR5cGUtPmxvY2tfY2xhc3MjMik7ClsgIDQzMi43NjExMDBdICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICBsb2NrKGpiZDJfaGFuZGxlKTsKWyAgNDMyLjc2 MTkyNl0gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGxvY2soJnR5cGUtPmxvY2tfY2xh c3MjMik7ClsgIDQzMi43NjI4MzZdICAgbG9jaygmbW0tPm1tYXBfc2VtKTsKWyAgNDMyLjc2MzM0 OF0gClsgIDQzMi43NjMzNDhdICAqKiogREVBRExPQ0sgKioqClsgIDQzMi43NjMzNDhdIApbICA0 MzIuNzY0MjA5XSAxIGxvY2sgaGVsZCBieSBrZXljdGwvODc3OgpbICA0MzIuNzY0NzY1XSAgIzA6 ICAoJnR5cGUtPmxvY2tfY2xhc3MjMil7KysrK30sIGF0OiBbPDAwMDAwMDAwOGNjZmExNTY+XSBr ZXljdGxfcmVhZF9rZXkrMHgxNzQvMHgyNDAKWyAgNDMyLjc2NjA0NV0gClsgIDQzMi43NjYwNDVd IHN0YWNrIGJhY2t0cmFjZToKWyAgNDMyLjc2NjY3MV0gQ1BVOiAxIFBJRDogODc3IENvbW06IGtl eWN0bCBOb3QgdGFpbnRlZCA0LjE1LjAtcmM4LW5leHQtMjAxODAxMTktMDAwMTktZ2FiN2VjNDZi NjkzNiAjMzEKWyAgNDMyLjc2ODAyMV0gSGFyZHdhcmUgbmFtZTogUUVNVSBTdGFuZGFyZCBQQyAo aTQ0MEZYICsgUElJWCwgMTk5NiksIEJJT1MgMS4xMC4yLTEgMDQvMDEvMjAxNApbICA0MzIuNzY5 MjExXSBDYWxsIFRyYWNlOgpbICA0MzIuNzY5NTU5XSAgZHVtcF9zdGFjaysweDhkLzB4ZDUKWyAg NDMyLjc3MDA0MV0gIHByaW50X2NpcmN1bGFyX2J1Zy5pc3JhLjIwKzB4MzIxLzB4NWUwClsgIDQz Mi43NzA3MTJdICA/IHNhdmVfdHJhY2UrMHhkNi8weDI1MApbICA0MzIuNzcxMTg1XSAgY2hlY2tf cHJldl9hZGQuY29uc3Rwcm9wLjI3KzB4YTJhLzB4MTY3MApbICA0MzIuNzcxODY3XSAgPyBkZXBv dF9zYXZlX3N0YWNrKzB4MmQ1LzB4NDkwClsgIDQzMi43NzI1MDRdICA/IGNoZWNrX3VzYWdlKzB4 MTNjMC8weDEzYzAKWyAgNDMyLjc3MzA0N10gID8gdHJhY2VfaGFyZGlycXNfb25fY2FsbGVyKzB4 M2RjLzB4NTUwClsgIDQzMi43NzM3MTJdICA/IF9yYXdfc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSsw eDM5LzB4NjAKWyAgNDMyLjc3NDQwMV0gID8gZGVwb3Rfc2F2ZV9zdGFjaysweDJkNS8weDQ5MApb ICA0MzIuNzc0OTc5XSAgPyBrYXNhbl9rbWFsbG9jKzB4MTNhLzB4MTcwClsgIDQzMi43NzU1MjJd ICA/IHZhbGlkYXRlX2NoYWluLmlzcmEuMjQrMHgxM2VlLzB4MjQxMApbICA0MzIuNzc2MTg3XSAg dmFsaWRhdGVfY2hhaW4uaXNyYS4yNCsweDEzZWUvMHgyNDEwClsgIDQzMi43NzY4MzFdICA/IGNo ZWNrX3ByZXZfYWRkLmNvbnN0cHJvcC4yNysweDE2NzAvMHgxNjcwClsgIDQzMi43Nzc1NDZdICBf X2xvY2tfYWNxdWlyZSsweDhkMS8weDEyZDAKWyAgNDMyLjc3ODA4OF0gIGxvY2tfYWNxdWlyZSsw eGFhLzB4MTcwClsgIDQzMi43Nzg1NzZdICA/IF9fbWlnaHRfZmF1bHQrMHhjZS8weDFhMApbICA0 MzIuNzc5MDk2XSAgX19taWdodF9mYXVsdCsweDEyYi8weDFhMApbICA0MzIuNzc5NjA4XSAgPyBf X21pZ2h0X2ZhdWx0KzB4Y2UvMHgxYTAKWyAgNDMyLjc4MDAyOV0gIF9jb3B5X3RvX3VzZXIrMHgy Ny8weGMwClsgIDQzMi43ODA0MzhdICBlbmNyeXB0ZWRfcmVhZCsweDU0ZC8weDdiMApbICA0MzIu NzgwODU4XSAgPyBrZXlfdGFza19wZXJtaXNzaW9uKzB4MTZlLzB4M2IwClsgIDQzMi43ODEzNDBd ICA/IGVuY3J5cHRlZF9pbnN0YW50aWF0ZSsweDhiMC8weDhiMApbICA0MzIuNzgxODUxXSAgPyBj cmVkc19hcmVfaW52YWxpZCsweDkvMHg1MApbICA0MzIuNzgyMjg3XSAgPyBsb29rdXBfdXNlcl9r ZXkrMHgxOWEvMHhmNTAKWyAgNDMyLjc4MjczNl0gID8gX19sb2NrX2FjcXVpcmUrMHg4ZDEvMHgx MmQwClsgIDQzMi43ODMxODJdICA/IGtleV92YWxpZGF0ZSsweGIwLzB4YjAKWyAgNDMyLjc4MzYw Ml0gID8ga2V5Y3RsX3JlYWRfa2V5KzB4MTc0LzB4MjQwClsgIDQzMi43ODQwNThdICBrZXljdGxf cmVhZF9rZXkrMHgxZjIvMHgyNDAKWyAgNDMyLjc4NDQ4Nl0gIFN5U19rZXljdGwrMHgxODQvMHgy YTAKWyAgNDMyLjc4NDg3NV0gIGVudHJ5X1NZU0NBTExfNjRfZmFzdHBhdGgrMHgxZS8weDhiClsg IDQzMi43ODUzNzRdIFJJUDogMDAzMzoweDdmODEyYjZjODI1OQotLQpUbyB1bnN1YnNjcmliZSBm cm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUga2V5cmluZ3MiIGluCnRo ZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJuZWwub3JnCk1vcmUgbWFq b3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRt bA== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 25 Jan 2018 16:37:48 -0800 From: Eric Biggers Subject: Re: [PATCH v3] fscrypt: add support for the encrypted key type Message-ID: <20180126003748.f2uhgwhulcltyok6@gmail.com> References: <20180118131359.8365-1-git@andred.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180118131359.8365-1-git@andred.net> To: =?iso-8859-1?Q?Andr=E9?= Draszik Cc: linux-kernel@vger.kernel.org, Mimi Zohar , David Howells , James Morris , "Serge E. Hallyn" , "Theodore Y. Ts'o" , Jaegeuk Kim , Kees Cook , Eric Biggers , linux-integrity@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, linux-fscrypt@vger.kernel.org List-ID: On Thu, Jan 18, 2018 at 01:13:59PM +0000, Andr� Draszik wrote: > -static int validate_user_key(struct fscrypt_info *crypt_info, > +static inline struct key *fscrypt_get_encrypted_key(const char *description) > +{ > + if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS)) > + return request_key(&key_type_encrypted, description, NULL); > + return ERR_PTR(-ENOKEY); > +} > + > +static int validate_keyring_key(struct fscrypt_info *crypt_info, > struct fscrypt_context *ctx, u8 *raw_key, > const char *prefix, int min_keysize) > { > char *description; > struct key *keyring_key; > - struct fscrypt_key *master_key; > - const struct user_key_payload *ukp; > + const u8 *master_key; > + u32 master_key_len; > int res; > > description = kasprintf(GFP_NOFS, "%s%*phN", prefix, > @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info, > return -ENOMEM; > > keyring_key = request_key(&key_type_logon, description, NULL); > + if (keyring_key == ERR_PTR(-ENOKEY)) > + keyring_key = fscrypt_get_encrypted_key(description); > kfree(description); > if (IS_ERR(keyring_key)) > return PTR_ERR(keyring_key); > down_read(&keyring_key->sem); > > - if (keyring_key->type != &key_type_logon) { > + if (keyring_key->type == &key_type_logon) { > + const struct user_key_payload *ukp; > + const struct fscrypt_key *fk; > + > + ukp = user_key_payload_locked(keyring_key); > + if (!ukp) { > + /* key was revoked before we acquired its semaphore */ > + res = -EKEYREVOKED; > + goto out; > + } > + if (ukp->datalen != sizeof(struct fscrypt_key)) { > + res = -EINVAL; > + goto out; > + } > + fk = (struct fscrypt_key *)ukp->data; > + master_key = fk->raw; > + master_key_len = fk->size; > + } else if (keyring_key->type == &key_type_encrypted) { > + const struct encrypted_key_payload *ekp; > + > + ekp = keyring_key->payload.data[0]; > + master_key = ekp->decrypted_data; > + master_key_len = ekp->decrypted_datalen; > + } else { > printk_once(KERN_WARNING > - "%s: key type must be logon\n", __func__); > + "%s: key type must be logon or encrypted\n", > + __func__); > res = -ENOKEY; > goto out; > } > - ukp = user_key_payload_locked(keyring_key); > - if (!ukp) { > - /* key was revoked before we acquired its semaphore */ > - res = -EKEYREVOKED; > - goto out; > - } > - if (ukp->datalen != sizeof(struct fscrypt_key)) { > - res = -EINVAL; > - goto out; > - } > - master_key = (struct fscrypt_key *)ukp->data; > BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE); > > - if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE > - || master_key->size % AES_BLOCK_SIZE != 0) { > + if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE > + || master_key_len % AES_BLOCK_SIZE != 0) { > printk_once(KERN_WARNING > - "%s: key size incorrect: %d\n", > - __func__, master_key->size); > + "%s: key size incorrect: %u\n", > + __func__, master_key_len); > res = -ENOKEY; > goto out; > } > - res = derive_key_aes(ctx->nonce, master_key, raw_key); > + res = derive_key_aes(ctx->nonce, master_key, master_key_len, raw_key); > + > out: > up_read(&keyring_key->sem); > key_put(keyring_key); > @@ -302,12 +328,12 @@ int fscrypt_get_encryption_info(struct inode *inode) > if (!raw_key) > goto out; > > - res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX, > - keysize); > + res = validate_keyring_key(crypt_info, &ctx, raw_key, > + FS_KEY_DESC_PREFIX, keysize); > if (res && inode->i_sb->s_cop->key_prefix) { > - int res2 = validate_user_key(crypt_info, &ctx, raw_key, > - inode->i_sb->s_cop->key_prefix, > - keysize); > + int res2 = validate_keyring_key(crypt_info, &ctx, raw_key, > + inode->i_sb->s_cop->key_prefix, > + keysize); > if (res2) { > if (res2 == -ENOKEY) > res = -ENOKEY; > -- > 2.15.1 > > -- > To unsubscribe from this list: send the line "unsubscribe keyrings" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html The code changes here look fine, but unfortunately there is a lock ordering problem exposed by using a key type that implements the key_type ->read() method. The problem is that encrypted_read() can take a page fault during copy_to_user() while holding the key semaphore, which then (with ext4) can trigger the start of a jbd2 transaction. Meanwhile fscrypt_get_encryption_info() can be called from within a jbd2 transaction via fscrypt_inherit_context(), which results in a different locking order. We probably will need to fix this by removing the call to fscrypt_get_encryption_info() from fscrypt_inherit_context(). I've pasted the lockdep report I got below: [ 432.705339] [ 432.705681] ====================================================== [ 432.707015] WARNING: possible circular locking dependency detected [ 432.708155] 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 Not tainted [ 432.709365] ------------------------------------------------------ [ 432.710482] keyctl/877 is trying to acquire lock: [ 432.711286] (&mm->mmap_sem){++++}, at: [<00000000e8bba1c7>] __might_fault+0xce/0x1a0 [ 432.712688] [ 432.712688] but task is already holding lock: [ 432.713684] (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240 [ 432.715176] [ 432.715176] which lock already depends on the new lock. [ 432.715176] [ 432.716601] [ 432.716601] the existing dependency chain (in reverse order) is: [ 432.717901] [ 432.717901] -> #3 (&type->lock_class#2){++++}: [ 432.718924] lock_acquire+0xaa/0x170 [ 432.719632] down_read+0x37/0xa0 [ 432.720298] validate_keyring_key.isra.1+0x97/0x410 [ 432.721218] fscrypt_get_encryption_info+0x724/0x1200 [ 432.722233] fscrypt_inherit_context+0x2c1/0x330 [ 432.723067] __ext4_new_inode+0x34f5/0x44d0 [ 432.723830] ext4_create+0x195/0x4a0 [ 432.724499] lookup_open+0xbe2/0x1400 [ 432.725185] path_openat+0xb31/0x1e50 [ 432.725876] do_filp_open+0x175/0x250 [ 432.726572] do_sys_open+0x219/0x3f0 [ 432.727312] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.728153] [ 432.728153] -> #2 (jbd2_handle){.+.+}: [ 432.729008] lock_acquire+0xaa/0x170 [ 432.729680] start_this_handle+0x47b/0x1020 [ 432.730463] jbd2__journal_start+0x32e/0x5c0 [ 432.731276] __ext4_journal_start_sb+0xa8/0x190 [ 432.732183] ext4_truncate+0x4dd/0xd00 [ 432.732868] ext4_setattr+0xa62/0x1ac0 [ 432.733528] notify_change+0x672/0xdb0 [ 432.734198] do_truncate+0x111/0x1c0 [ 432.734831] path_openat+0xee6/0x1e50 [ 432.735476] do_filp_open+0x175/0x250 [ 432.736149] do_sys_open+0x219/0x3f0 [ 432.736785] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.737576] [ 432.737576] -> #1 (&ei->i_mmap_sem){++++}: [ 432.738508] lock_acquire+0xaa/0x170 [ 432.739148] down_read+0x37/0xa0 [ 432.739735] ext4_filemap_fault+0x7b/0xb0 [ 432.740446] __do_fault+0x7a/0x200 [ 432.741060] __handle_mm_fault+0x6e0/0x2040 [ 432.741801] handle_mm_fault+0x194/0x320 [ 432.742494] __do_page_fault+0x4e1/0xa70 [ 432.743184] page_fault+0x2c/0x60 [ 432.743784] __clear_user+0x3d/0x60 [ 432.744409] clear_user+0x76/0xa0 [ 432.745009] load_elf_binary+0x2bf6/0x2f10 [ 432.745753] search_binary_handler+0x136/0x450 [ 432.746522] do_execveat_common.isra.12+0x1241/0x19c0 [ 432.747339] do_execve+0x2c/0x40 [ 432.747881] try_to_run_init_process+0x12/0x40 [ 432.748611] kernel_init+0xf2/0x180 [ 432.749190] ret_from_fork+0x3a/0x50 [ 432.749785] [ 432.749785] -> #0 (&mm->mmap_sem){++++}: [ 432.750576] __lock_acquire+0x8d1/0x12d0 [ 432.751232] lock_acquire+0xaa/0x170 [ 432.751848] __might_fault+0x12b/0x1a0 [ 432.752478] _copy_to_user+0x27/0xc0 [ 432.753080] encrypted_read+0x54d/0x7b0 [ 432.753734] keyctl_read_key+0x1f2/0x240 [ 432.754396] SyS_keyctl+0x184/0x2a0 [ 432.754995] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.755778] [ 432.755778] other info that might help us debug this: [ 432.755778] [ 432.756972] Chain exists of: [ 432.756972] &mm->mmap_sem --> jbd2_handle --> &type->lock_class#2 [ 432.756972] [ 432.758498] Possible unsafe locking scenario: [ 432.758498] [ 432.759302] CPU0 CPU1 [ 432.759919] ---- ---- [ 432.760536] lock(&type->lock_class#2); [ 432.761100] lock(jbd2_handle); [ 432.761926] lock(&type->lock_class#2); [ 432.762836] lock(&mm->mmap_sem); [ 432.763348] [ 432.763348] *** DEADLOCK *** [ 432.763348] [ 432.764209] 1 lock held by keyctl/877: [ 432.764765] #0: (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240 [ 432.766045] [ 432.766045] stack backtrace: [ 432.766671] CPU: 1 PID: 877 Comm: keyctl Not tainted 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 [ 432.768021] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014 [ 432.769211] Call Trace: [ 432.769559] dump_stack+0x8d/0xd5 [ 432.770041] print_circular_bug.isra.20+0x321/0x5e0 [ 432.770712] ? save_trace+0xd6/0x250 [ 432.771185] check_prev_add.constprop.27+0xa2a/0x1670 [ 432.771867] ? depot_save_stack+0x2d5/0x490 [ 432.772504] ? check_usage+0x13c0/0x13c0 [ 432.773047] ? trace_hardirqs_on_caller+0x3dc/0x550 [ 432.773712] ? _raw_spin_unlock_irqrestore+0x39/0x60 [ 432.774401] ? depot_save_stack+0x2d5/0x490 [ 432.774979] ? kasan_kmalloc+0x13a/0x170 [ 432.775522] ? validate_chain.isra.24+0x13ee/0x2410 [ 432.776187] validate_chain.isra.24+0x13ee/0x2410 [ 432.776831] ? check_prev_add.constprop.27+0x1670/0x1670 [ 432.777546] __lock_acquire+0x8d1/0x12d0 [ 432.778088] lock_acquire+0xaa/0x170 [ 432.778576] ? __might_fault+0xce/0x1a0 [ 432.779096] __might_fault+0x12b/0x1a0 [ 432.779608] ? __might_fault+0xce/0x1a0 [ 432.780029] _copy_to_user+0x27/0xc0 [ 432.780438] encrypted_read+0x54d/0x7b0 [ 432.780858] ? key_task_permission+0x16e/0x3b0 [ 432.781340] ? encrypted_instantiate+0x8b0/0x8b0 [ 432.781851] ? creds_are_invalid+0x9/0x50 [ 432.782287] ? lookup_user_key+0x19a/0xf50 [ 432.782736] ? __lock_acquire+0x8d1/0x12d0 [ 432.783182] ? key_validate+0xb0/0xb0 [ 432.783602] ? keyctl_read_key+0x174/0x240 [ 432.784058] keyctl_read_key+0x1f2/0x240 [ 432.784486] SyS_keyctl+0x184/0x2a0 [ 432.784875] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.785374] RIP: 0033:0x7f812b6c8259 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f67.google.com ([209.85.214.67]:42761 "EHLO mail-it0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751375AbeAZAhw (ORCPT ); Thu, 25 Jan 2018 19:37:52 -0500 Date: Thu, 25 Jan 2018 16:37:48 -0800 From: Eric Biggers To: =?iso-8859-1?Q?Andr=E9?= Draszik Cc: linux-kernel@vger.kernel.org, Mimi Zohar , David Howells , James Morris , "Serge E. Hallyn" , "Theodore Y. Ts'o" , Jaegeuk Kim , Kees Cook , Eric Biggers , linux-integrity@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, linux-fscrypt@vger.kernel.org Subject: Re: [PATCH v3] fscrypt: add support for the encrypted key type Message-ID: <20180126003748.f2uhgwhulcltyok6@gmail.com> References: <20180118131359.8365-1-git@andred.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 In-Reply-To: <20180118131359.8365-1-git@andred.net> Sender: linux-integrity-owner@vger.kernel.org List-ID: On Thu, Jan 18, 2018 at 01:13:59PM +0000, Andre Draszik wrote: > -static int validate_user_key(struct fscrypt_info *crypt_info, > +static inline struct key *fscrypt_get_encrypted_key(const char *description) > +{ > + if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS)) > + return request_key(&key_type_encrypted, description, NULL); > + return ERR_PTR(-ENOKEY); > +} > + > +static int validate_keyring_key(struct fscrypt_info *crypt_info, > struct fscrypt_context *ctx, u8 *raw_key, > const char *prefix, int min_keysize) > { > char *description; > struct key *keyring_key; > - struct fscrypt_key *master_key; > - const struct user_key_payload *ukp; > + const u8 *master_key; > + u32 master_key_len; > int res; > > description = kasprintf(GFP_NOFS, "%s%*phN", prefix, > @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info, > return -ENOMEM; > > keyring_key = request_key(&key_type_logon, description, NULL); > + if (keyring_key == ERR_PTR(-ENOKEY)) > + keyring_key = fscrypt_get_encrypted_key(description); > kfree(description); > if (IS_ERR(keyring_key)) > return PTR_ERR(keyring_key); > down_read(&keyring_key->sem); > > - if (keyring_key->type != &key_type_logon) { > + if (keyring_key->type == &key_type_logon) { > + const struct user_key_payload *ukp; > + const struct fscrypt_key *fk; > + > + ukp = user_key_payload_locked(keyring_key); > + if (!ukp) { > + /* key was revoked before we acquired its semaphore */ > + res = -EKEYREVOKED; > + goto out; > + } > + if (ukp->datalen != sizeof(struct fscrypt_key)) { > + res = -EINVAL; > + goto out; > + } > + fk = (struct fscrypt_key *)ukp->data; > + master_key = fk->raw; > + master_key_len = fk->size; > + } else if (keyring_key->type == &key_type_encrypted) { > + const struct encrypted_key_payload *ekp; > + > + ekp = keyring_key->payload.data[0]; > + master_key = ekp->decrypted_data; > + master_key_len = ekp->decrypted_datalen; > + } else { > printk_once(KERN_WARNING > - "%s: key type must be logon\n", __func__); > + "%s: key type must be logon or encrypted\n", > + __func__); > res = -ENOKEY; > goto out; > } > - ukp = user_key_payload_locked(keyring_key); > - if (!ukp) { > - /* key was revoked before we acquired its semaphore */ > - res = -EKEYREVOKED; > - goto out; > - } > - if (ukp->datalen != sizeof(struct fscrypt_key)) { > - res = -EINVAL; > - goto out; > - } > - master_key = (struct fscrypt_key *)ukp->data; > BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE); > > - if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE > - || master_key->size % AES_BLOCK_SIZE != 0) { > + if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE > + || master_key_len % AES_BLOCK_SIZE != 0) { > printk_once(KERN_WARNING > - "%s: key size incorrect: %d\n", > - __func__, master_key->size); > + "%s: key size incorrect: %u\n", > + __func__, master_key_len); > res = -ENOKEY; > goto out; > } > - res = derive_key_aes(ctx->nonce, master_key, raw_key); > + res = derive_key_aes(ctx->nonce, master_key, master_key_len, raw_key); > + > out: > up_read(&keyring_key->sem); > key_put(keyring_key); > @@ -302,12 +328,12 @@ int fscrypt_get_encryption_info(struct inode *inode) > if (!raw_key) > goto out; > > - res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX, > - keysize); > + res = validate_keyring_key(crypt_info, &ctx, raw_key, > + FS_KEY_DESC_PREFIX, keysize); > if (res && inode->i_sb->s_cop->key_prefix) { > - int res2 = validate_user_key(crypt_info, &ctx, raw_key, > - inode->i_sb->s_cop->key_prefix, > - keysize); > + int res2 = validate_keyring_key(crypt_info, &ctx, raw_key, > + inode->i_sb->s_cop->key_prefix, > + keysize); > if (res2) { > if (res2 == -ENOKEY) > res = -ENOKEY; > -- > 2.15.1 > > -- > To unsubscribe from this list: send the line "unsubscribe keyrings" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html The code changes here look fine, but unfortunately there is a lock ordering problem exposed by using a key type that implements the key_type ->read() method. The problem is that encrypted_read() can take a page fault during copy_to_user() while holding the key semaphore, which then (with ext4) can trigger the start of a jbd2 transaction. Meanwhile fscrypt_get_encryption_info() can be called from within a jbd2 transaction via fscrypt_inherit_context(), which results in a different locking order. We probably will need to fix this by removing the call to fscrypt_get_encryption_info() from fscrypt_inherit_context(). I've pasted the lockdep report I got below: [ 432.705339] [ 432.705681] ====================================================== [ 432.707015] WARNING: possible circular locking dependency detected [ 432.708155] 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 Not tainted [ 432.709365] ------------------------------------------------------ [ 432.710482] keyctl/877 is trying to acquire lock: [ 432.711286] (&mm->mmap_sem){++++}, at: [<00000000e8bba1c7>] __might_fault+0xce/0x1a0 [ 432.712688] [ 432.712688] but task is already holding lock: [ 432.713684] (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240 [ 432.715176] [ 432.715176] which lock already depends on the new lock. [ 432.715176] [ 432.716601] [ 432.716601] the existing dependency chain (in reverse order) is: [ 432.717901] [ 432.717901] -> #3 (&type->lock_class#2){++++}: [ 432.718924] lock_acquire+0xaa/0x170 [ 432.719632] down_read+0x37/0xa0 [ 432.720298] validate_keyring_key.isra.1+0x97/0x410 [ 432.721218] fscrypt_get_encryption_info+0x724/0x1200 [ 432.722233] fscrypt_inherit_context+0x2c1/0x330 [ 432.723067] __ext4_new_inode+0x34f5/0x44d0 [ 432.723830] ext4_create+0x195/0x4a0 [ 432.724499] lookup_open+0xbe2/0x1400 [ 432.725185] path_openat+0xb31/0x1e50 [ 432.725876] do_filp_open+0x175/0x250 [ 432.726572] do_sys_open+0x219/0x3f0 [ 432.727312] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.728153] [ 432.728153] -> #2 (jbd2_handle){.+.+}: [ 432.729008] lock_acquire+0xaa/0x170 [ 432.729680] start_this_handle+0x47b/0x1020 [ 432.730463] jbd2__journal_start+0x32e/0x5c0 [ 432.731276] __ext4_journal_start_sb+0xa8/0x190 [ 432.732183] ext4_truncate+0x4dd/0xd00 [ 432.732868] ext4_setattr+0xa62/0x1ac0 [ 432.733528] notify_change+0x672/0xdb0 [ 432.734198] do_truncate+0x111/0x1c0 [ 432.734831] path_openat+0xee6/0x1e50 [ 432.735476] do_filp_open+0x175/0x250 [ 432.736149] do_sys_open+0x219/0x3f0 [ 432.736785] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.737576] [ 432.737576] -> #1 (&ei->i_mmap_sem){++++}: [ 432.738508] lock_acquire+0xaa/0x170 [ 432.739148] down_read+0x37/0xa0 [ 432.739735] ext4_filemap_fault+0x7b/0xb0 [ 432.740446] __do_fault+0x7a/0x200 [ 432.741060] __handle_mm_fault+0x6e0/0x2040 [ 432.741801] handle_mm_fault+0x194/0x320 [ 432.742494] __do_page_fault+0x4e1/0xa70 [ 432.743184] page_fault+0x2c/0x60 [ 432.743784] __clear_user+0x3d/0x60 [ 432.744409] clear_user+0x76/0xa0 [ 432.745009] load_elf_binary+0x2bf6/0x2f10 [ 432.745753] search_binary_handler+0x136/0x450 [ 432.746522] do_execveat_common.isra.12+0x1241/0x19c0 [ 432.747339] do_execve+0x2c/0x40 [ 432.747881] try_to_run_init_process+0x12/0x40 [ 432.748611] kernel_init+0xf2/0x180 [ 432.749190] ret_from_fork+0x3a/0x50 [ 432.749785] [ 432.749785] -> #0 (&mm->mmap_sem){++++}: [ 432.750576] __lock_acquire+0x8d1/0x12d0 [ 432.751232] lock_acquire+0xaa/0x170 [ 432.751848] __might_fault+0x12b/0x1a0 [ 432.752478] _copy_to_user+0x27/0xc0 [ 432.753080] encrypted_read+0x54d/0x7b0 [ 432.753734] keyctl_read_key+0x1f2/0x240 [ 432.754396] SyS_keyctl+0x184/0x2a0 [ 432.754995] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.755778] [ 432.755778] other info that might help us debug this: [ 432.755778] [ 432.756972] Chain exists of: [ 432.756972] &mm->mmap_sem --> jbd2_handle --> &type->lock_class#2 [ 432.756972] [ 432.758498] Possible unsafe locking scenario: [ 432.758498] [ 432.759302] CPU0 CPU1 [ 432.759919] ---- ---- [ 432.760536] lock(&type->lock_class#2); [ 432.761100] lock(jbd2_handle); [ 432.761926] lock(&type->lock_class#2); [ 432.762836] lock(&mm->mmap_sem); [ 432.763348] [ 432.763348] *** DEADLOCK *** [ 432.763348] [ 432.764209] 1 lock held by keyctl/877: [ 432.764765] #0: (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240 [ 432.766045] [ 432.766045] stack backtrace: [ 432.766671] CPU: 1 PID: 877 Comm: keyctl Not tainted 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 [ 432.768021] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014 [ 432.769211] Call Trace: [ 432.769559] dump_stack+0x8d/0xd5 [ 432.770041] print_circular_bug.isra.20+0x321/0x5e0 [ 432.770712] ? save_trace+0xd6/0x250 [ 432.771185] check_prev_add.constprop.27+0xa2a/0x1670 [ 432.771867] ? depot_save_stack+0x2d5/0x490 [ 432.772504] ? check_usage+0x13c0/0x13c0 [ 432.773047] ? trace_hardirqs_on_caller+0x3dc/0x550 [ 432.773712] ? _raw_spin_unlock_irqrestore+0x39/0x60 [ 432.774401] ? depot_save_stack+0x2d5/0x490 [ 432.774979] ? kasan_kmalloc+0x13a/0x170 [ 432.775522] ? validate_chain.isra.24+0x13ee/0x2410 [ 432.776187] validate_chain.isra.24+0x13ee/0x2410 [ 432.776831] ? check_prev_add.constprop.27+0x1670/0x1670 [ 432.777546] __lock_acquire+0x8d1/0x12d0 [ 432.778088] lock_acquire+0xaa/0x170 [ 432.778576] ? __might_fault+0xce/0x1a0 [ 432.779096] __might_fault+0x12b/0x1a0 [ 432.779608] ? __might_fault+0xce/0x1a0 [ 432.780029] _copy_to_user+0x27/0xc0 [ 432.780438] encrypted_read+0x54d/0x7b0 [ 432.780858] ? key_task_permission+0x16e/0x3b0 [ 432.781340] ? encrypted_instantiate+0x8b0/0x8b0 [ 432.781851] ? creds_are_invalid+0x9/0x50 [ 432.782287] ? lookup_user_key+0x19a/0xf50 [ 432.782736] ? __lock_acquire+0x8d1/0x12d0 [ 432.783182] ? key_validate+0xb0/0xb0 [ 432.783602] ? keyctl_read_key+0x174/0x240 [ 432.784058] keyctl_read_key+0x1f2/0x240 [ 432.784486] SyS_keyctl+0x184/0x2a0 [ 432.784875] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.785374] RIP: 0033:0x7f812b6c8259 From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiggers3@gmail.com (Eric Biggers) Date: Thu, 25 Jan 2018 16:37:48 -0800 Subject: [PATCH v3] fscrypt: add support for the encrypted key type In-Reply-To: <20180118131359.8365-1-git@andred.net> References: <20180118131359.8365-1-git@andred.net> Message-ID: <20180126003748.f2uhgwhulcltyok6@gmail.com> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Thu, Jan 18, 2018 at 01:13:59PM +0000, Andr? Draszik wrote: > -static int validate_user_key(struct fscrypt_info *crypt_info, > +static inline struct key *fscrypt_get_encrypted_key(const char *description) > +{ > + if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS)) > + return request_key(&key_type_encrypted, description, NULL); > + return ERR_PTR(-ENOKEY); > +} > + > +static int validate_keyring_key(struct fscrypt_info *crypt_info, > struct fscrypt_context *ctx, u8 *raw_key, > const char *prefix, int min_keysize) > { > char *description; > struct key *keyring_key; > - struct fscrypt_key *master_key; > - const struct user_key_payload *ukp; > + const u8 *master_key; > + u32 master_key_len; > int res; > > description = kasprintf(GFP_NOFS, "%s%*phN", prefix, > @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info, > return -ENOMEM; > > keyring_key = request_key(&key_type_logon, description, NULL); > + if (keyring_key == ERR_PTR(-ENOKEY)) > + keyring_key = fscrypt_get_encrypted_key(description); > kfree(description); > if (IS_ERR(keyring_key)) > return PTR_ERR(keyring_key); > down_read(&keyring_key->sem); > > - if (keyring_key->type != &key_type_logon) { > + if (keyring_key->type == &key_type_logon) { > + const struct user_key_payload *ukp; > + const struct fscrypt_key *fk; > + > + ukp = user_key_payload_locked(keyring_key); > + if (!ukp) { > + /* key was revoked before we acquired its semaphore */ > + res = -EKEYREVOKED; > + goto out; > + } > + if (ukp->datalen != sizeof(struct fscrypt_key)) { > + res = -EINVAL; > + goto out; > + } > + fk = (struct fscrypt_key *)ukp->data; > + master_key = fk->raw; > + master_key_len = fk->size; > + } else if (keyring_key->type == &key_type_encrypted) { > + const struct encrypted_key_payload *ekp; > + > + ekp = keyring_key->payload.data[0]; > + master_key = ekp->decrypted_data; > + master_key_len = ekp->decrypted_datalen; > + } else { > printk_once(KERN_WARNING > - "%s: key type must be logon\n", __func__); > + "%s: key type must be logon or encrypted\n", > + __func__); > res = -ENOKEY; > goto out; > } > - ukp = user_key_payload_locked(keyring_key); > - if (!ukp) { > - /* key was revoked before we acquired its semaphore */ > - res = -EKEYREVOKED; > - goto out; > - } > - if (ukp->datalen != sizeof(struct fscrypt_key)) { > - res = -EINVAL; > - goto out; > - } > - master_key = (struct fscrypt_key *)ukp->data; > BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE); > > - if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE > - || master_key->size % AES_BLOCK_SIZE != 0) { > + if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE > + || master_key_len % AES_BLOCK_SIZE != 0) { > printk_once(KERN_WARNING > - "%s: key size incorrect: %d\n", > - __func__, master_key->size); > + "%s: key size incorrect: %u\n", > + __func__, master_key_len); > res = -ENOKEY; > goto out; > } > - res = derive_key_aes(ctx->nonce, master_key, raw_key); > + res = derive_key_aes(ctx->nonce, master_key, master_key_len, raw_key); > + > out: > up_read(&keyring_key->sem); > key_put(keyring_key); > @@ -302,12 +328,12 @@ int fscrypt_get_encryption_info(struct inode *inode) > if (!raw_key) > goto out; > > - res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX, > - keysize); > + res = validate_keyring_key(crypt_info, &ctx, raw_key, > + FS_KEY_DESC_PREFIX, keysize); > if (res && inode->i_sb->s_cop->key_prefix) { > - int res2 = validate_user_key(crypt_info, &ctx, raw_key, > - inode->i_sb->s_cop->key_prefix, > - keysize); > + int res2 = validate_keyring_key(crypt_info, &ctx, raw_key, > + inode->i_sb->s_cop->key_prefix, > + keysize); > if (res2) { > if (res2 == -ENOKEY) > res = -ENOKEY; > -- > 2.15.1 > > -- > To unsubscribe from this list: send the line "unsubscribe keyrings" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html The code changes here look fine, but unfortunately there is a lock ordering problem exposed by using a key type that implements the key_type ->read() method. The problem is that encrypted_read() can take a page fault during copy_to_user() while holding the key semaphore, which then (with ext4) can trigger the start of a jbd2 transaction. Meanwhile fscrypt_get_encryption_info() can be called from within a jbd2 transaction via fscrypt_inherit_context(), which results in a different locking order. We probably will need to fix this by removing the call to fscrypt_get_encryption_info() from fscrypt_inherit_context(). I've pasted the lockdep report I got below: [ 432.705339] [ 432.705681] ====================================================== [ 432.707015] WARNING: possible circular locking dependency detected [ 432.708155] 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 Not tainted [ 432.709365] ------------------------------------------------------ [ 432.710482] keyctl/877 is trying to acquire lock: [ 432.711286] (&mm->mmap_sem){++++}, at: [<00000000e8bba1c7>] __might_fault+0xce/0x1a0 [ 432.712688] [ 432.712688] but task is already holding lock: [ 432.713684] (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240 [ 432.715176] [ 432.715176] which lock already depends on the new lock. [ 432.715176] [ 432.716601] [ 432.716601] the existing dependency chain (in reverse order) is: [ 432.717901] [ 432.717901] -> #3 (&type->lock_class#2){++++}: [ 432.718924] lock_acquire+0xaa/0x170 [ 432.719632] down_read+0x37/0xa0 [ 432.720298] validate_keyring_key.isra.1+0x97/0x410 [ 432.721218] fscrypt_get_encryption_info+0x724/0x1200 [ 432.722233] fscrypt_inherit_context+0x2c1/0x330 [ 432.723067] __ext4_new_inode+0x34f5/0x44d0 [ 432.723830] ext4_create+0x195/0x4a0 [ 432.724499] lookup_open+0xbe2/0x1400 [ 432.725185] path_openat+0xb31/0x1e50 [ 432.725876] do_filp_open+0x175/0x250 [ 432.726572] do_sys_open+0x219/0x3f0 [ 432.727312] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.728153] [ 432.728153] -> #2 (jbd2_handle){.+.+}: [ 432.729008] lock_acquire+0xaa/0x170 [ 432.729680] start_this_handle+0x47b/0x1020 [ 432.730463] jbd2__journal_start+0x32e/0x5c0 [ 432.731276] __ext4_journal_start_sb+0xa8/0x190 [ 432.732183] ext4_truncate+0x4dd/0xd00 [ 432.732868] ext4_setattr+0xa62/0x1ac0 [ 432.733528] notify_change+0x672/0xdb0 [ 432.734198] do_truncate+0x111/0x1c0 [ 432.734831] path_openat+0xee6/0x1e50 [ 432.735476] do_filp_open+0x175/0x250 [ 432.736149] do_sys_open+0x219/0x3f0 [ 432.736785] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.737576] [ 432.737576] -> #1 (&ei->i_mmap_sem){++++}: [ 432.738508] lock_acquire+0xaa/0x170 [ 432.739148] down_read+0x37/0xa0 [ 432.739735] ext4_filemap_fault+0x7b/0xb0 [ 432.740446] __do_fault+0x7a/0x200 [ 432.741060] __handle_mm_fault+0x6e0/0x2040 [ 432.741801] handle_mm_fault+0x194/0x320 [ 432.742494] __do_page_fault+0x4e1/0xa70 [ 432.743184] page_fault+0x2c/0x60 [ 432.743784] __clear_user+0x3d/0x60 [ 432.744409] clear_user+0x76/0xa0 [ 432.745009] load_elf_binary+0x2bf6/0x2f10 [ 432.745753] search_binary_handler+0x136/0x450 [ 432.746522] do_execveat_common.isra.12+0x1241/0x19c0 [ 432.747339] do_execve+0x2c/0x40 [ 432.747881] try_to_run_init_process+0x12/0x40 [ 432.748611] kernel_init+0xf2/0x180 [ 432.749190] ret_from_fork+0x3a/0x50 [ 432.749785] [ 432.749785] -> #0 (&mm->mmap_sem){++++}: [ 432.750576] __lock_acquire+0x8d1/0x12d0 [ 432.751232] lock_acquire+0xaa/0x170 [ 432.751848] __might_fault+0x12b/0x1a0 [ 432.752478] _copy_to_user+0x27/0xc0 [ 432.753080] encrypted_read+0x54d/0x7b0 [ 432.753734] keyctl_read_key+0x1f2/0x240 [ 432.754396] SyS_keyctl+0x184/0x2a0 [ 432.754995] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.755778] [ 432.755778] other info that might help us debug this: [ 432.755778] [ 432.756972] Chain exists of: [ 432.756972] &mm->mmap_sem --> jbd2_handle --> &type->lock_class#2 [ 432.756972] [ 432.758498] Possible unsafe locking scenario: [ 432.758498] [ 432.759302] CPU0 CPU1 [ 432.759919] ---- ---- [ 432.760536] lock(&type->lock_class#2); [ 432.761100] lock(jbd2_handle); [ 432.761926] lock(&type->lock_class#2); [ 432.762836] lock(&mm->mmap_sem); [ 432.763348] [ 432.763348] *** DEADLOCK *** [ 432.763348] [ 432.764209] 1 lock held by keyctl/877: [ 432.764765] #0: (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240 [ 432.766045] [ 432.766045] stack backtrace: [ 432.766671] CPU: 1 PID: 877 Comm: keyctl Not tainted 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 [ 432.768021] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014 [ 432.769211] Call Trace: [ 432.769559] dump_stack+0x8d/0xd5 [ 432.770041] print_circular_bug.isra.20+0x321/0x5e0 [ 432.770712] ? save_trace+0xd6/0x250 [ 432.771185] check_prev_add.constprop.27+0xa2a/0x1670 [ 432.771867] ? depot_save_stack+0x2d5/0x490 [ 432.772504] ? check_usage+0x13c0/0x13c0 [ 432.773047] ? trace_hardirqs_on_caller+0x3dc/0x550 [ 432.773712] ? _raw_spin_unlock_irqrestore+0x39/0x60 [ 432.774401] ? depot_save_stack+0x2d5/0x490 [ 432.774979] ? kasan_kmalloc+0x13a/0x170 [ 432.775522] ? validate_chain.isra.24+0x13ee/0x2410 [ 432.776187] validate_chain.isra.24+0x13ee/0x2410 [ 432.776831] ? check_prev_add.constprop.27+0x1670/0x1670 [ 432.777546] __lock_acquire+0x8d1/0x12d0 [ 432.778088] lock_acquire+0xaa/0x170 [ 432.778576] ? __might_fault+0xce/0x1a0 [ 432.779096] __might_fault+0x12b/0x1a0 [ 432.779608] ? __might_fault+0xce/0x1a0 [ 432.780029] _copy_to_user+0x27/0xc0 [ 432.780438] encrypted_read+0x54d/0x7b0 [ 432.780858] ? key_task_permission+0x16e/0x3b0 [ 432.781340] ? encrypted_instantiate+0x8b0/0x8b0 [ 432.781851] ? creds_are_invalid+0x9/0x50 [ 432.782287] ? lookup_user_key+0x19a/0xf50 [ 432.782736] ? __lock_acquire+0x8d1/0x12d0 [ 432.783182] ? key_validate+0xb0/0xb0 [ 432.783602] ? keyctl_read_key+0x174/0x240 [ 432.784058] keyctl_read_key+0x1f2/0x240 [ 432.784486] SyS_keyctl+0x184/0x2a0 [ 432.784875] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.785374] RIP: 0033:0x7f812b6c8259 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751908AbeAZAh4 (ORCPT ); Thu, 25 Jan 2018 19:37:56 -0500 Received: from mail-it0-f67.google.com ([209.85.214.67]:42761 "EHLO mail-it0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751375AbeAZAhw (ORCPT ); Thu, 25 Jan 2018 19:37:52 -0500 X-Google-Smtp-Source: AH8x226LtHGD4Gb9lhlG6APYR29he2+VBbvMGqjsNe47KLN3eNozb9PHqHI10riSASTW4Uy6S+FFfw== Date: Thu, 25 Jan 2018 16:37:48 -0800 From: Eric Biggers To: =?iso-8859-1?Q?Andr=E9?= Draszik Cc: linux-kernel@vger.kernel.org, Mimi Zohar , David Howells , James Morris , "Serge E. Hallyn" , "Theodore Y. Ts'o" , Jaegeuk Kim , Kees Cook , Eric Biggers , linux-integrity@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, linux-fscrypt@vger.kernel.org Subject: Re: [PATCH v3] fscrypt: add support for the encrypted key type Message-ID: <20180126003748.f2uhgwhulcltyok6@gmail.com> References: <20180118131359.8365-1-git@andred.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180118131359.8365-1-git@andred.net> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 18, 2018 at 01:13:59PM +0000, André Draszik wrote: > -static int validate_user_key(struct fscrypt_info *crypt_info, > +static inline struct key *fscrypt_get_encrypted_key(const char *description) > +{ > + if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS)) > + return request_key(&key_type_encrypted, description, NULL); > + return ERR_PTR(-ENOKEY); > +} > + > +static int validate_keyring_key(struct fscrypt_info *crypt_info, > struct fscrypt_context *ctx, u8 *raw_key, > const char *prefix, int min_keysize) > { > char *description; > struct key *keyring_key; > - struct fscrypt_key *master_key; > - const struct user_key_payload *ukp; > + const u8 *master_key; > + u32 master_key_len; > int res; > > description = kasprintf(GFP_NOFS, "%s%*phN", prefix, > @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info, > return -ENOMEM; > > keyring_key = request_key(&key_type_logon, description, NULL); > + if (keyring_key == ERR_PTR(-ENOKEY)) > + keyring_key = fscrypt_get_encrypted_key(description); > kfree(description); > if (IS_ERR(keyring_key)) > return PTR_ERR(keyring_key); > down_read(&keyring_key->sem); > > - if (keyring_key->type != &key_type_logon) { > + if (keyring_key->type == &key_type_logon) { > + const struct user_key_payload *ukp; > + const struct fscrypt_key *fk; > + > + ukp = user_key_payload_locked(keyring_key); > + if (!ukp) { > + /* key was revoked before we acquired its semaphore */ > + res = -EKEYREVOKED; > + goto out; > + } > + if (ukp->datalen != sizeof(struct fscrypt_key)) { > + res = -EINVAL; > + goto out; > + } > + fk = (struct fscrypt_key *)ukp->data; > + master_key = fk->raw; > + master_key_len = fk->size; > + } else if (keyring_key->type == &key_type_encrypted) { > + const struct encrypted_key_payload *ekp; > + > + ekp = keyring_key->payload.data[0]; > + master_key = ekp->decrypted_data; > + master_key_len = ekp->decrypted_datalen; > + } else { > printk_once(KERN_WARNING > - "%s: key type must be logon\n", __func__); > + "%s: key type must be logon or encrypted\n", > + __func__); > res = -ENOKEY; > goto out; > } > - ukp = user_key_payload_locked(keyring_key); > - if (!ukp) { > - /* key was revoked before we acquired its semaphore */ > - res = -EKEYREVOKED; > - goto out; > - } > - if (ukp->datalen != sizeof(struct fscrypt_key)) { > - res = -EINVAL; > - goto out; > - } > - master_key = (struct fscrypt_key *)ukp->data; > BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE); > > - if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE > - || master_key->size % AES_BLOCK_SIZE != 0) { > + if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE > + || master_key_len % AES_BLOCK_SIZE != 0) { > printk_once(KERN_WARNING > - "%s: key size incorrect: %d\n", > - __func__, master_key->size); > + "%s: key size incorrect: %u\n", > + __func__, master_key_len); > res = -ENOKEY; > goto out; > } > - res = derive_key_aes(ctx->nonce, master_key, raw_key); > + res = derive_key_aes(ctx->nonce, master_key, master_key_len, raw_key); > + > out: > up_read(&keyring_key->sem); > key_put(keyring_key); > @@ -302,12 +328,12 @@ int fscrypt_get_encryption_info(struct inode *inode) > if (!raw_key) > goto out; > > - res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX, > - keysize); > + res = validate_keyring_key(crypt_info, &ctx, raw_key, > + FS_KEY_DESC_PREFIX, keysize); > if (res && inode->i_sb->s_cop->key_prefix) { > - int res2 = validate_user_key(crypt_info, &ctx, raw_key, > - inode->i_sb->s_cop->key_prefix, > - keysize); > + int res2 = validate_keyring_key(crypt_info, &ctx, raw_key, > + inode->i_sb->s_cop->key_prefix, > + keysize); > if (res2) { > if (res2 == -ENOKEY) > res = -ENOKEY; > -- > 2.15.1 > > -- > To unsubscribe from this list: send the line "unsubscribe keyrings" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html The code changes here look fine, but unfortunately there is a lock ordering problem exposed by using a key type that implements the key_type ->read() method. The problem is that encrypted_read() can take a page fault during copy_to_user() while holding the key semaphore, which then (with ext4) can trigger the start of a jbd2 transaction. Meanwhile fscrypt_get_encryption_info() can be called from within a jbd2 transaction via fscrypt_inherit_context(), which results in a different locking order. We probably will need to fix this by removing the call to fscrypt_get_encryption_info() from fscrypt_inherit_context(). I've pasted the lockdep report I got below: [ 432.705339] [ 432.705681] ====================================================== [ 432.707015] WARNING: possible circular locking dependency detected [ 432.708155] 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 Not tainted [ 432.709365] ------------------------------------------------------ [ 432.710482] keyctl/877 is trying to acquire lock: [ 432.711286] (&mm->mmap_sem){++++}, at: [<00000000e8bba1c7>] __might_fault+0xce/0x1a0 [ 432.712688] [ 432.712688] but task is already holding lock: [ 432.713684] (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240 [ 432.715176] [ 432.715176] which lock already depends on the new lock. [ 432.715176] [ 432.716601] [ 432.716601] the existing dependency chain (in reverse order) is: [ 432.717901] [ 432.717901] -> #3 (&type->lock_class#2){++++}: [ 432.718924] lock_acquire+0xaa/0x170 [ 432.719632] down_read+0x37/0xa0 [ 432.720298] validate_keyring_key.isra.1+0x97/0x410 [ 432.721218] fscrypt_get_encryption_info+0x724/0x1200 [ 432.722233] fscrypt_inherit_context+0x2c1/0x330 [ 432.723067] __ext4_new_inode+0x34f5/0x44d0 [ 432.723830] ext4_create+0x195/0x4a0 [ 432.724499] lookup_open+0xbe2/0x1400 [ 432.725185] path_openat+0xb31/0x1e50 [ 432.725876] do_filp_open+0x175/0x250 [ 432.726572] do_sys_open+0x219/0x3f0 [ 432.727312] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.728153] [ 432.728153] -> #2 (jbd2_handle){.+.+}: [ 432.729008] lock_acquire+0xaa/0x170 [ 432.729680] start_this_handle+0x47b/0x1020 [ 432.730463] jbd2__journal_start+0x32e/0x5c0 [ 432.731276] __ext4_journal_start_sb+0xa8/0x190 [ 432.732183] ext4_truncate+0x4dd/0xd00 [ 432.732868] ext4_setattr+0xa62/0x1ac0 [ 432.733528] notify_change+0x672/0xdb0 [ 432.734198] do_truncate+0x111/0x1c0 [ 432.734831] path_openat+0xee6/0x1e50 [ 432.735476] do_filp_open+0x175/0x250 [ 432.736149] do_sys_open+0x219/0x3f0 [ 432.736785] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.737576] [ 432.737576] -> #1 (&ei->i_mmap_sem){++++}: [ 432.738508] lock_acquire+0xaa/0x170 [ 432.739148] down_read+0x37/0xa0 [ 432.739735] ext4_filemap_fault+0x7b/0xb0 [ 432.740446] __do_fault+0x7a/0x200 [ 432.741060] __handle_mm_fault+0x6e0/0x2040 [ 432.741801] handle_mm_fault+0x194/0x320 [ 432.742494] __do_page_fault+0x4e1/0xa70 [ 432.743184] page_fault+0x2c/0x60 [ 432.743784] __clear_user+0x3d/0x60 [ 432.744409] clear_user+0x76/0xa0 [ 432.745009] load_elf_binary+0x2bf6/0x2f10 [ 432.745753] search_binary_handler+0x136/0x450 [ 432.746522] do_execveat_common.isra.12+0x1241/0x19c0 [ 432.747339] do_execve+0x2c/0x40 [ 432.747881] try_to_run_init_process+0x12/0x40 [ 432.748611] kernel_init+0xf2/0x180 [ 432.749190] ret_from_fork+0x3a/0x50 [ 432.749785] [ 432.749785] -> #0 (&mm->mmap_sem){++++}: [ 432.750576] __lock_acquire+0x8d1/0x12d0 [ 432.751232] lock_acquire+0xaa/0x170 [ 432.751848] __might_fault+0x12b/0x1a0 [ 432.752478] _copy_to_user+0x27/0xc0 [ 432.753080] encrypted_read+0x54d/0x7b0 [ 432.753734] keyctl_read_key+0x1f2/0x240 [ 432.754396] SyS_keyctl+0x184/0x2a0 [ 432.754995] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.755778] [ 432.755778] other info that might help us debug this: [ 432.755778] [ 432.756972] Chain exists of: [ 432.756972] &mm->mmap_sem --> jbd2_handle --> &type->lock_class#2 [ 432.756972] [ 432.758498] Possible unsafe locking scenario: [ 432.758498] [ 432.759302] CPU0 CPU1 [ 432.759919] ---- ---- [ 432.760536] lock(&type->lock_class#2); [ 432.761100] lock(jbd2_handle); [ 432.761926] lock(&type->lock_class#2); [ 432.762836] lock(&mm->mmap_sem); [ 432.763348] [ 432.763348] *** DEADLOCK *** [ 432.763348] [ 432.764209] 1 lock held by keyctl/877: [ 432.764765] #0: (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240 [ 432.766045] [ 432.766045] stack backtrace: [ 432.766671] CPU: 1 PID: 877 Comm: keyctl Not tainted 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 [ 432.768021] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014 [ 432.769211] Call Trace: [ 432.769559] dump_stack+0x8d/0xd5 [ 432.770041] print_circular_bug.isra.20+0x321/0x5e0 [ 432.770712] ? save_trace+0xd6/0x250 [ 432.771185] check_prev_add.constprop.27+0xa2a/0x1670 [ 432.771867] ? depot_save_stack+0x2d5/0x490 [ 432.772504] ? check_usage+0x13c0/0x13c0 [ 432.773047] ? trace_hardirqs_on_caller+0x3dc/0x550 [ 432.773712] ? _raw_spin_unlock_irqrestore+0x39/0x60 [ 432.774401] ? depot_save_stack+0x2d5/0x490 [ 432.774979] ? kasan_kmalloc+0x13a/0x170 [ 432.775522] ? validate_chain.isra.24+0x13ee/0x2410 [ 432.776187] validate_chain.isra.24+0x13ee/0x2410 [ 432.776831] ? check_prev_add.constprop.27+0x1670/0x1670 [ 432.777546] __lock_acquire+0x8d1/0x12d0 [ 432.778088] lock_acquire+0xaa/0x170 [ 432.778576] ? __might_fault+0xce/0x1a0 [ 432.779096] __might_fault+0x12b/0x1a0 [ 432.779608] ? __might_fault+0xce/0x1a0 [ 432.780029] _copy_to_user+0x27/0xc0 [ 432.780438] encrypted_read+0x54d/0x7b0 [ 432.780858] ? key_task_permission+0x16e/0x3b0 [ 432.781340] ? encrypted_instantiate+0x8b0/0x8b0 [ 432.781851] ? creds_are_invalid+0x9/0x50 [ 432.782287] ? lookup_user_key+0x19a/0xf50 [ 432.782736] ? __lock_acquire+0x8d1/0x12d0 [ 432.783182] ? key_validate+0xb0/0xb0 [ 432.783602] ? keyctl_read_key+0x174/0x240 [ 432.784058] keyctl_read_key+0x1f2/0x240 [ 432.784486] SyS_keyctl+0x184/0x2a0 [ 432.784875] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.785374] RIP: 0033:0x7f812b6c8259