| Message ID | 2587101.YvN0IxRmOH@tachyon.chronox.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
Am Sonntag, 18. Januar 2015, 23:56:03 schrieb Stephan Mueller: Hi Tadeusz, > The cipher registered as __driver-gcm-aes-aesni is never intended > to be used directly by any caller. Instead it is a service mechanism to > rfc4106-gcm-aesni. > > The kernel crypto API unconditionally calls the registered setkey > function. In case a caller erroneously uses __driver-gcm-aes-aesni a > call to crypto_aead_setkey will cause a NULL pointer dereference without > this patch. I tested that patch and can confirm that this patch fixes the kernel crash triggered through the AF_ALG interface for AEAD ciphers that is currently under development reported earlier. > > CC: Tadeusz Struk <tadeusz.struk@intel.com> > Signed-off-by: Stephan Mueller <smueller@chronox.de> > --- > arch/x86/crypto/aesni-intel_glue.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/arch/x86/crypto/aesni-intel_glue.c > b/arch/x86/crypto/aesni-intel_glue.c index 947c6bf..a278ef9 100644 > --- a/arch/x86/crypto/aesni-intel_glue.c > +++ b/arch/x86/crypto/aesni-intel_glue.c > @@ -1012,6 +1012,16 @@ static int rfc4106_decrypt(struct aead_request *req) > } > } > > +static int __driver_rfc4106_set_key(struct crypto_aead *parent, > + const u8 *key, unsigned int key_len) > +{ > + /* > + * __driver-gcm-aes-aesni is only a backend for rfc4106-gcm-aesni > + * and is never intended to be used as a regular cipher. > + */ > + return -EOPNOTSUPP; > +} > + > static int __driver_rfc4106_encrypt(struct aead_request *req) > { > u8 one_entry_in_sg = 0; > @@ -1366,6 +1376,7 @@ static struct crypto_alg aesni_algs[] = { { > .cra_module = THIS_MODULE, > .cra_u = { > .aead = { > + .setkey = __driver_rfc4106_set_key, > .encrypt = __driver_rfc4106_encrypt, > .decrypt = __driver_rfc4106_decrypt, > },
On 01/18/2015 02:56 PM, Stephan Mueller wrote: > The cipher registered as __driver-gcm-aes-aesni is never intended > to be used directly by any caller. Instead it is a service mechanism to > rfc4106-gcm-aesni. > > The kernel crypto API unconditionally calls the registered setkey > function. In case a caller erroneously uses __driver-gcm-aes-aesni a > call to crypto_aead_setkey will cause a NULL pointer dereference without > this patch. Acked-by: Tadeusz Struk <tadeusz.struk@intel.com> -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 18, 2015 at 11:56:03PM +0100, Stephan Mueller wrote: > The cipher registered as __driver-gcm-aes-aesni is never intended > to be used directly by any caller. Instead it is a service mechanism to > rfc4106-gcm-aesni. > > The kernel crypto API unconditionally calls the registered setkey > function. In case a caller erroneously uses __driver-gcm-aes-aesni a > call to crypto_aead_setkey will cause a NULL pointer dereference without > this patch. > > CC: Tadeusz Struk <tadeusz.struk@intel.com> > Signed-off-by: Stephan Mueller <smueller@chronox.de> Rather than adding a bogus setkey function, please fix this mess properly by moving the top-level setkey function into the __driver one where it should be. Compare with how we handle it in the ablk_helper which is pretty much the same thing. Thanks,
Am Dienstag, 20. Januar 2015, 14:17:04 schrieb Herbert Xu: Hi Herbert, >On Sun, Jan 18, 2015 at 11:56:03PM +0100, Stephan Mueller wrote: >> The cipher registered as __driver-gcm-aes-aesni is never intended >> to be used directly by any caller. Instead it is a service mechanism >> to rfc4106-gcm-aesni. >> >> The kernel crypto API unconditionally calls the registered setkey >> function. In case a caller erroneously uses __driver-gcm-aes-aesni a >> call to crypto_aead_setkey will cause a NULL pointer dereference >> without this patch. >> >> CC: Tadeusz Struk <tadeusz.struk@intel.com> >> Signed-off-by: Stephan Mueller <smueller@chronox.de> > >Rather than adding a bogus setkey function, please fix this mess >properly by moving the top-level setkey function into the __driver >one where it should be. Compare with how we handle it in the >ablk_helper which is pretty much the same thing. That is a good suggestion. And the modification is quite limited as the existing rfc4106_set_key could be used for the __driver with only slight modifications. In that case, however, we should apply the same to rfc4106_set_authsize. This in turn would then turn the __driver implementation into a full GCM implementation. That would mean that we should rename it from __driver into gcm(aes) / gcm-aesni. > >Thanks, Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 20, 2015 at 04:35:41AM +0100, Stephan Mueller wrote: > > This in turn would then turn the __driver implementation into a full GCM > implementation. That would mean that we should rename it from __driver > into gcm(aes) / gcm-aesni. No you shouldn't because it'll fail in interrupt context where you cannot use those special instructions. The whole point of this setup is to use accelerated instructions where possible, and otherwise fall back to a separate thread where we can do so safely. Cheers,
Am Dienstag, 20. Januar 2015, 14:37:05 schrieb Herbert Xu: Hi Herbert, >On Tue, Jan 20, 2015 at 04:35:41AM +0100, Stephan Mueller wrote: >> This in turn would then turn the __driver implementation into a full >> GCM implementation. That would mean that we should rename it from >> __driver into gcm(aes) / gcm-aesni. > >No you shouldn't because it'll fail in interrupt context where >you cannot use those special instructions. How would the fail manifest itself? If algif_aead would be present, user space could use the __driver implementation regardless of a setkey or authsize callback by simply calling encrypt/decrypt. Would the error be limited to that caller only? > >The whole point of this setup is to use accelerated instructions >where possible, and otherwise fall back to a separate thread >where we can do so safely. Thanks for clarification. > >Cheers, Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 20, 2015 at 04:54:44AM +0100, Stephan Mueller wrote: > > How would the fail manifest itself? If algif_aead would be present, user > space could use the __driver implementation regardless of a setkey or > authsize callback by simply calling encrypt/decrypt. Would the error be > limited to that caller only? For user-space it'll never fail. However, for kernel users such as IPsec it'll fail if the interrupt occurs while in a thread that's already using the FPU. But you're right, we probably shouldn't allow these algorithms to be directly exported to user-space at all, even when they do possess a setkey function. In fact, we should ban them from other places where they might be used too, such as through IPsec. I'll try to write something up to do this Thanks,
Am Dienstag, 20. Januar 2015, 14:17:04 schrieb Herbert Xu: Hi Tadeusz, > On Sun, Jan 18, 2015 at 11:56:03PM +0100, Stephan Mueller wrote: > > The cipher registered as __driver-gcm-aes-aesni is never intended > > to be used directly by any caller. Instead it is a service mechanism to > > rfc4106-gcm-aesni. > > > > The kernel crypto API unconditionally calls the registered setkey > > function. In case a caller erroneously uses __driver-gcm-aes-aesni a > > call to crypto_aead_setkey will cause a NULL pointer dereference without > > this patch. > > > > CC: Tadeusz Struk <tadeusz.struk@intel.com> > > Signed-off-by: Stephan Mueller <smueller@chronox.de> > > Rather than adding a bogus setkey function, please fix this mess > properly by moving the top-level setkey function into the __driver > one where it should be. Compare with how we handle it in the > ablk_helper which is pretty much the same thing. Tadeusz, are you working on that update or shall I have a look?
On 01/20/2015 05:25 PM, Stephan Mueller wrote: >> Rather than adding a bogus setkey function, please fix this mess >> properly by moving the top-level setkey function into the __driver >> one where it should be. Compare with how we handle it in the >> ablk_helper which is pretty much the same thing. > > Tadeusz, are you working on that update or shall I have a look? > Hi, No, I thought that the agreement was that we don't want to allow user space to use these helpers directly, right? Am I missing something? -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Donnerstag, 22. Januar 2015, 10:23:57 schrieb Tadeusz Struk: Hi Tadeusz, >On 01/20/2015 05:25 PM, Stephan Mueller wrote: >>> Rather than adding a bogus setkey function, please fix this mess >>> properly by moving the top-level setkey function into the __driver >>> one where it should be. Compare with how we handle it in the >>> ablk_helper which is pretty much the same thing. >> >> Tadeusz, are you working on that update or shall I have a look? > >Hi, >No, I thought that the agreement was that we don't want to allow user >space to use these helpers directly, right? Am I missing something? That would be correct. But if I understood Herbert correctly, he is creating a patch that disables these service ciphers for general usage. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/22/2015 01:20 PM, Stephan Mueller wrote: > That would be correct. But if I understood Herbert correctly, he is > creating a patch that disables these service ciphers for general usage. Yes, and this should also implicitly fix the problem with user space. Thanks, Tadeusz -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 22, 2015 at 10:23:57AM -0800, Tadeusz Struk wrote: > > No, I thought that the agreement was that we don't want to allow user > space to use these helpers directly, right? Am I missing something? Yes but we should also fix this so that it's a proper aead algorithm. Cheers,
On 01/22/2015 02:23 PM, Herbert Xu wrote: > Yes but we should also fix this so that it's a proper aead > algorithm. Ok, I'll do that. Thanks, Tadeusz -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c index 947c6bf..a278ef9 100644 --- a/arch/x86/crypto/aesni-intel_glue.c +++ b/arch/x86/crypto/aesni-intel_glue.c @@ -1012,6 +1012,16 @@ static int rfc4106_decrypt(struct aead_request *req) } } +static int __driver_rfc4106_set_key(struct crypto_aead *parent, + const u8 *key, unsigned int key_len) +{ + /* + * __driver-gcm-aes-aesni is only a backend for rfc4106-gcm-aesni + * and is never intended to be used as a regular cipher. + */ + return -EOPNOTSUPP; +} + static int __driver_rfc4106_encrypt(struct aead_request *req) { u8 one_entry_in_sg = 0; @@ -1366,6 +1376,7 @@ static struct crypto_alg aesni_algs[] = { { .cra_module = THIS_MODULE, .cra_u = { .aead = { + .setkey = __driver_rfc4106_set_key, .encrypt = __driver_rfc4106_encrypt, .decrypt = __driver_rfc4106_decrypt, },
The cipher registered as __driver-gcm-aes-aesni is never intended to be used directly by any caller. Instead it is a service mechanism to rfc4106-gcm-aesni. The kernel crypto API unconditionally calls the registered setkey function. In case a caller erroneously uses __driver-gcm-aes-aesni a call to crypto_aead_setkey will cause a NULL pointer dereference without this patch. CC: Tadeusz Struk <tadeusz.struk@intel.com> Signed-off-by: Stephan Mueller <smueller@chronox.de> --- arch/x86/crypto/aesni-intel_glue.c | 11 +++++++++++ 1 file changed, 11 insertions(+)