| Message ID | 2100556.thFK4ZhSZX@myon.chronox.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
Am Mittwoch, 28. Oktober 2015, 01:09:58 schrieb Stephan Mueller: Hi Harsh, > > > However, any error in user space should not crash the kernel. So, a fix > should be done. But I think your code is not correct as it solidifies a > broken user space code. After thinking a bit again, I think your approach is correct after all. I was able to reproduce the crash by simply adding more accept calls to my test code. And I can confirm that your patch works, for hashes. *BUT* it does NOT work for HMAC as the key is set on the TFM and the subsequent accepts do not transport the key. Albeit your code prevents the kernel from crashing, the HMAC calculation will be done with an empty key as the setkey operation does not reach the TFM handle in the subordinate accept() call. So, I would think that the second accept is simply broken, for HMAC at least. Herbert, what is the purpose of that subordinate accept that is implemented with hash_accept? As this is broken for HMACs, should it be removed entirely?
Hi Stephan, I tried your patch on my machine. Kernel is not crashing. The openssl break with this. Can you share HMAC program which you are suspecting it will not work or do you already have some test written in libkcapi/test.sh which will fail. Regards Harsh Jain On Wed, Oct 28, 2015 at 6:25 AM, Stephan Mueller <smueller@chronox.de> wrote: > Am Mittwoch, 28. Oktober 2015, 01:09:58 schrieb Stephan Mueller: > > Hi Harsh, > >> >> >> However, any error in user space should not crash the kernel. So, a fix >> should be done. But I think your code is not correct as it solidifies a >> broken user space code. > > After thinking a bit again, I think your approach is correct after all. I was > able to reproduce the crash by simply adding more accept calls to my test > code. And I can confirm that your patch works, for hashes. > > *BUT* it does NOT work for HMAC as the key is set on the TFM and the > subsequent accepts do not transport the key. Albeit your code prevents the > kernel from crashing, the HMAC calculation will be done with an empty key as > the setkey operation does not reach the TFM handle in the subordinate accept() > call. > > So, I would think that the second accept is simply broken, for HMAC at least. > > Herbert, what is the purpose of that subordinate accept that is implemented > with hash_accept? As this is broken for HMACs, should it be removed entirely? > > -- > 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
Am Mittwoch, 28. Oktober 2015, 16:24:34 schrieb Harsh Jain: Hi Harsh, >Hi Stephan, > >I tried your patch on my machine. Kernel is not crashing. The openssl >break with this. Can you share HMAC program which you are suspecting >it will not work or do you already have some test written in >libkcapi/test.sh which will fail. See comments above test/kcapi-main.c:cavs_hash * HMAC command line invocation: * $ ./kcapi -x 3 -c "hmac(sha1)" -k 6e77ebd479da794707bc6cde3694f552ea892dab -p 31b62a797adbff6b8a358d2b5206e01fee079de8cdfc4695138bba163b4efbf30127343e7fd4fbc696c3d38d8f27f57c024b5056f726ceeb4c31d98e57751ec8cbe8904ee0f9b031ae6a0c55da5e062475b3d7832191d4057643ef5fa446801d59a04693e573a8159cd2416b7bd39c7f0fe63c599365e04d596c05736beaab58 * 7f204ea665666f5bd2b370e546d1b408005e4d85 To do that, apply your patch and then 1. open lib/kcapi-kernel-if.c and change line 567 from handle->opfd = accept(handle->tfmfd, NULL, 0); to handle->opfd = accept(handle->tfmfd, NULL, 0); handle->opfd = accept(handle->opfd, NULL, 0); handle->opfd = accept(handle->opfd, NULL, 0); handle->opfd = accept(handle->opfd, NULL, 0); handle->opfd = accept(handle->opfd, NULL, 0); You will see that the hash commands will pass, the HMAC fails Without your patch, the kernel crashes (same as with your OpenSSL code). The reason is that setkey is applied on the TFM that is not conveyed to the subsequent TFMs generated with new accepts. > > >Regards >Harsh Jain > >On Wed, Oct 28, 2015 at 6:25 AM, Stephan Mueller <smueller@chronox.de> wrote: >> Am Mittwoch, 28. Oktober 2015, 01:09:58 schrieb Stephan Mueller: >> >> Hi Harsh, >> >>> However, any error in user space should not crash the kernel. So, a fix >>> should be done. But I think your code is not correct as it solidifies a >>> broken user space code. >> >> After thinking a bit again, I think your approach is correct after all. I >> was able to reproduce the crash by simply adding more accept calls to my >> test code. And I can confirm that your patch works, for hashes. >> >> *BUT* it does NOT work for HMAC as the key is set on the TFM and the >> subsequent accepts do not transport the key. Albeit your code prevents the >> kernel from crashing, the HMAC calculation will be done with an empty key >> as >> the setkey operation does not reach the TFM handle in the subordinate >> accept() call. >> >> So, I would think that the second accept is simply broken, for HMAC at >> least. >> >> Herbert, what is the purpose of that subordinate accept that is implemented >> with hash_accept? As this is broken for HMACs, should it be removed >> entirely? >> >> -- >> 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 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
Hi Stephan, If we add sendmsg() in between 2 accept calls then the setkey problem will happen? handle->opfd = accept(handle->tfmfd, NULL, 0); sendmsg() handle->opfd = accept(handle->opfd, NULL, 0); sendmsg() handle->opfd = accept(handle->opfd, NULL, 0); If yes, Then may be it is expected behavior and user is supposed to set the key explicitly with some other system call.Why I am saying this is. I remember somewhere in kernel code I read some comment related to setkey operations. In that case my patch should work. 1 doubt I have related to patch is do I need to set "ctx->more" =1 after initialisation. Correct me If I am wrong. Thanks for your support. regards Harsh Jain On Wed, Oct 28, 2015 at 4:53 PM, Stephan Mueller <smueller@chronox.de> wrote: > Am Mittwoch, 28. Oktober 2015, 16:24:34 schrieb Harsh Jain: > > Hi Harsh, > >>Hi Stephan, >> >>I tried your patch on my machine. Kernel is not crashing. The openssl >>break with this. Can you share HMAC program which you are suspecting >>it will not work or do you already have some test written in >>libkcapi/test.sh which will fail. > > See comments above test/kcapi-main.c:cavs_hash > > * HMAC command line invocation: > * $ ./kcapi -x 3 -c "hmac(sha1)" -k 6e77ebd479da794707bc6cde3694f552ea892dab > -p > 31b62a797adbff6b8a358d2b5206e01fee079de8cdfc4695138bba163b4efbf30127343e7fd4fbc696c3d38d8f27f57c024b5056f726ceeb4c31d98e57751ec8cbe8904ee0f9b031ae6a0c55da5e062475b3d7832191d4057643ef5fa446801d59a04693e573a8159cd2416b7bd39c7f0fe63c599365e04d596c05736beaab58 > * 7f204ea665666f5bd2b370e546d1b408005e4d85 > > To do that, apply your patch and then > > 1. open lib/kcapi-kernel-if.c and change line 567 from > > handle->opfd = accept(handle->tfmfd, NULL, 0); > > > to > > handle->opfd = accept(handle->tfmfd, NULL, 0); > handle->opfd = accept(handle->opfd, NULL, 0); > handle->opfd = accept(handle->opfd, NULL, 0); > handle->opfd = accept(handle->opfd, NULL, 0); > handle->opfd = accept(handle->opfd, NULL, 0); > > You will see that the hash commands will pass, the HMAC fails > > Without your patch, the kernel crashes (same as with your OpenSSL code). > > The reason is that setkey is applied on the TFM that is not conveyed to the > subsequent TFMs generated with new accepts. >> >> >>Regards >>Harsh Jain >> >>On Wed, Oct 28, 2015 at 6:25 AM, Stephan Mueller <smueller@chronox.de> wrote: >>> Am Mittwoch, 28. Oktober 2015, 01:09:58 schrieb Stephan Mueller: >>> >>> Hi Harsh, >>> >>>> However, any error in user space should not crash the kernel. So, a fix >>>> should be done. But I think your code is not correct as it solidifies a >>>> broken user space code. >>> >>> After thinking a bit again, I think your approach is correct after all. I >>> was able to reproduce the crash by simply adding more accept calls to my >>> test code. And I can confirm that your patch works, for hashes. >>> >>> *BUT* it does NOT work for HMAC as the key is set on the TFM and the >>> subsequent accepts do not transport the key. Albeit your code prevents the >>> kernel from crashing, the HMAC calculation will be done with an empty key >>> as >>> the setkey operation does not reach the TFM handle in the subordinate >>> accept() call. >>> >>> So, I would think that the second accept is simply broken, for HMAC at >>> least. >>> >>> Herbert, what is the purpose of that subordinate accept that is implemented >>> with hash_accept? As this is broken for HMACs, should it be removed >>> entirely? >>> >>> -- >>> 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 > > > 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
Am Freitag, 30. Oktober 2015, 14:02:27 schrieb Harsh Jain: Hi Harsh, >Hi Stephan, > >If we add sendmsg() in between 2 accept calls then the setkey problem >will happen? > >handle->opfd = accept(handle->tfmfd, NULL, 0); >sendmsg() >handle->opfd = accept(handle->opfd, NULL, 0); >sendmsg() >handle->opfd = accept(handle->opfd, NULL, 0); Without testing, I would very much expect that, because the setkey does not apply to the subordinate tfm. > >If yes, Then may be it is expected behavior and user is supposed to >set the key explicitly with some other system call.Why I am saying >this is. I remember somewhere in kernel code I read some comment >related to setkey operations. I would like to wait for Herbert to chime in here on how he thinks this would work. > >In that case my patch should work. 1 doubt I have related to patch is >do I need to set "ctx->more" =1 after initialisation. > >Correct me If I am wrong. > > >Thanks for your support. > > >regards >Harsh Jain > >On Wed, Oct 28, 2015 at 4:53 PM, Stephan Mueller <smueller@chronox.de> wrote: >> Am Mittwoch, 28. Oktober 2015, 16:24:34 schrieb Harsh Jain: >> >> Hi Harsh, >> >>>Hi Stephan, >>> >>>I tried your patch on my machine. Kernel is not crashing. The openssl >>>break with this. Can you share HMAC program which you are suspecting >>>it will not work or do you already have some test written in >>>libkcapi/test.sh which will fail. >>> >> See comments above test/kcapi-main.c:cavs_hash >> >> * HMAC command line invocation: >> * $ ./kcapi -x 3 -c "hmac(sha1)" -k >> 6e77ebd479da794707bc6cde3694f552ea892dab >> >> -p >> 31b62a797adbff6b8a358d2b5206e01fee079de8cdfc4695138bba163b4efbf30127343e7fd >> 4fbc696c3d38d8f27f57c024b5056f726ceeb4c31d98e57751ec8cbe8904ee0f9b031ae6a0c >> 55da5e062475b3d7832191d4057643ef5fa446801d59a04693e573a8159cd2416b7bd39c7f0 >> fe63c599365e04d596c05736beaab58> >> * 7f204ea665666f5bd2b370e546d1b408005e4d85 >> >> To do that, apply your patch and then >> >> 1. open lib/kcapi-kernel-if.c and change line 567 from >> >> handle->opfd = accept(handle->tfmfd, NULL, 0); >> >> >> to >> >> handle->opfd = accept(handle->tfmfd, NULL, 0); >> handle->opfd = accept(handle->opfd, NULL, 0); >> handle->opfd = accept(handle->opfd, NULL, 0); >> handle->opfd = accept(handle->opfd, NULL, 0); >> handle->opfd = accept(handle->opfd, NULL, 0); >> >> You will see that the hash commands will pass, the HMAC fails >> >> Without your patch, the kernel crashes (same as with your OpenSSL code). >> >> The reason is that setkey is applied on the TFM that is not conveyed to the >> subsequent TFMs generated with new accepts. >> >>>Regards >>>Harsh Jain >>> >>>On Wed, Oct 28, 2015 at 6:25 AM, Stephan Mueller <smueller@chronox.de> wrote: >>>> Am Mittwoch, 28. Oktober 2015, 01:09:58 schrieb Stephan Mueller: >>>> >>>> Hi Harsh, >>>> >>>>> However, any error in user space should not crash the kernel. So, a fix >>>>> should be done. But I think your code is not correct as it solidifies a >>>>> broken user space code. >>>> >>>> After thinking a bit again, I think your approach is correct after all. I >>>> was able to reproduce the crash by simply adding more accept calls to my >>>> test code. And I can confirm that your patch works, for hashes. >>>> >>>> *BUT* it does NOT work for HMAC as the key is set on the TFM and the >>>> subsequent accepts do not transport the key. Albeit your code prevents >>>> the >>>> kernel from crashing, the HMAC calculation will be done with an empty key >>>> as >>>> the setkey operation does not reach the TFM handle in the subordinate >>>> accept() call. >>>> >>>> So, I would think that the second accept is simply broken, for HMAC at >>>> least. >>>> >>>> Herbert, what is the purpose of that subordinate accept that is >>>> implemented >>>> with hash_accept? As this is broken for HMACs, should it be removed >>>> entirely? >>>> >>>> -- >>>> 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 >>> >> Ciao >> Stephan 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
Hi, I tried patch on my setup and its working fine. Thanks Stephan, Herbert for your support. Regards Harsh Jain On Fri, Oct 30, 2015 at 4:40 PM, Stephan Mueller <smueller@chronox.de> wrote: > Am Freitag, 30. Oktober 2015, 14:02:27 schrieb Harsh Jain: > > Hi Harsh, > >>Hi Stephan, >> >>If we add sendmsg() in between 2 accept calls then the setkey problem >>will happen? >> >>handle->opfd = accept(handle->tfmfd, NULL, 0); >>sendmsg() >>handle->opfd = accept(handle->opfd, NULL, 0); >>sendmsg() >>handle->opfd = accept(handle->opfd, NULL, 0); > > Without testing, I would very much expect that, because the setkey does not > apply to the subordinate tfm. >> >>If yes, Then may be it is expected behavior and user is supposed to >>set the key explicitly with some other system call.Why I am saying >>this is. I remember somewhere in kernel code I read some comment >>related to setkey operations. > > I would like to wait for Herbert to chime in here on how he thinks this would > work. >> >>In that case my patch should work. 1 doubt I have related to patch is >>do I need to set "ctx->more" =1 after initialisation. >> >>Correct me If I am wrong. >> >> >>Thanks for your support. >> >> >>regards >>Harsh Jain >> >>On Wed, Oct 28, 2015 at 4:53 PM, Stephan Mueller <smueller@chronox.de> wrote: >>> Am Mittwoch, 28. Oktober 2015, 16:24:34 schrieb Harsh Jain: >>> >>> Hi Harsh, >>> >>>>Hi Stephan, >>>> >>>>I tried your patch on my machine. Kernel is not crashing. The openssl >>>>break with this. Can you share HMAC program which you are suspecting >>>>it will not work or do you already have some test written in >>>>libkcapi/test.sh which will fail. >>>> >>> See comments above test/kcapi-main.c:cavs_hash >>> >>> * HMAC command line invocation: >>> * $ ./kcapi -x 3 -c "hmac(sha1)" -k >>> 6e77ebd479da794707bc6cde3694f552ea892dab >>> >>> -p >>> 31b62a797adbff6b8a358d2b5206e01fee079de8cdfc4695138bba163b4efbf30127343e7fd >>> 4fbc696c3d38d8f27f57c024b5056f726ceeb4c31d98e57751ec8cbe8904ee0f9b031ae6a0c >>> 55da5e062475b3d7832191d4057643ef5fa446801d59a04693e573a8159cd2416b7bd39c7f0 >>> fe63c599365e04d596c05736beaab58> >>> * 7f204ea665666f5bd2b370e546d1b408005e4d85 >>> >>> To do that, apply your patch and then >>> >>> 1. open lib/kcapi-kernel-if.c and change line 567 from >>> >>> handle->opfd = accept(handle->tfmfd, NULL, 0); >>> >>> >>> to >>> >>> handle->opfd = accept(handle->tfmfd, NULL, 0); >>> handle->opfd = accept(handle->opfd, NULL, 0); >>> handle->opfd = accept(handle->opfd, NULL, 0); >>> handle->opfd = accept(handle->opfd, NULL, 0); >>> handle->opfd = accept(handle->opfd, NULL, 0); >>> >>> You will see that the hash commands will pass, the HMAC fails >>> >>> Without your patch, the kernel crashes (same as with your OpenSSL code). >>> >>> The reason is that setkey is applied on the TFM that is not conveyed to the >>> subsequent TFMs generated with new accepts. >>> >>>>Regards >>>>Harsh Jain >>>> >>>>On Wed, Oct 28, 2015 at 6:25 AM, Stephan Mueller <smueller@chronox.de> > wrote: >>>>> Am Mittwoch, 28. Oktober 2015, 01:09:58 schrieb Stephan Mueller: >>>>> >>>>> Hi Harsh, >>>>> >>>>>> However, any error in user space should not crash the kernel. So, a fix >>>>>> should be done. But I think your code is not correct as it solidifies a >>>>>> broken user space code. >>>>> >>>>> After thinking a bit again, I think your approach is correct after all. I >>>>> was able to reproduce the crash by simply adding more accept calls to my >>>>> test code. And I can confirm that your patch works, for hashes. >>>>> >>>>> *BUT* it does NOT work for HMAC as the key is set on the TFM and the >>>>> subsequent accepts do not transport the key. Albeit your code prevents >>>>> the >>>>> kernel from crashing, the HMAC calculation will be done with an empty key >>>>> as >>>>> the setkey operation does not reach the TFM handle in the subordinate >>>>> accept() call. >>>>> >>>>> So, I would think that the second accept is simply broken, for HMAC at >>>>> least. >>>>> >>>>> Herbert, what is the purpose of that subordinate accept that is >>>>> implemented >>>>> with hash_accept? As this is broken for HMACs, should it be removed >>>>> entirely? >>>>> >>>>> -- >>>>> 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 >>>> >>> Ciao >>> Stephan > > > 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
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c index 1396ad0..785df23 100644 --- a/crypto/algif_hash.c +++ b/crypto/algif_hash.c @@ -183,6 +183,9 @@ static int hash_accept(struct socket *sock, struct socket *newsock, int flags) struct hash_ctx *ctx2; int err; + if (!ctx->more) + return -EINVAL; + err = crypto_ahash_export(req, state); if (err)