| Message ID | 20181019230153.28201-1-dbaryshkov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [1/2] crypto: fix cfb mode decryption | expand |
(+ James) On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > crypto_cfb_decrypt_segment() incorrectly XOR'ed generated keystream with > IV, rather than with data stream, resulting in incorrect decryption. > Test vectors will be added in the next patch. > > Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> > Cc: stable@vger.kernel.org > --- > crypto/cfb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/crypto/cfb.c b/crypto/cfb.c > index a0d68c09e1b9..fd4e8500e121 100644 > --- a/crypto/cfb.c > +++ b/crypto/cfb.c > @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct skcipher_walk *walk, > > do { > crypto_cfb_encrypt_one(tfm, iv, dst); > - crypto_xor(dst, iv, bsize); > + crypto_xor(dst, src, bsize); > iv = src; > > src += bsize; > -- > 2.19.1 >
On Sun, 2018-10-21 at 09:05 +0200, Ard Biesheuvel wrote: > (+ James) Thanks! > On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov > <dbaryshkov@gmail.com> wrote: > > crypto_cfb_decrypt_segment() incorrectly XOR'ed generated keystream > > with > > IV, rather than with data stream, resulting in incorrect > > decryption. > > Test vectors will be added in the next patch. > > > > Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> > > Cc: stable@vger.kernel.org > > --- > > crypto/cfb.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/crypto/cfb.c b/crypto/cfb.c > > index a0d68c09e1b9..fd4e8500e121 100644 > > --- a/crypto/cfb.c > > +++ b/crypto/cfb.c > > @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct > > skcipher_walk *walk, > > > > do { > > crypto_cfb_encrypt_one(tfm, iv, dst); > > - crypto_xor(dst, iv, bsize); > > + crypto_xor(dst, src, bsize); This does look right. I think the reason the TPM code works is that it always does encrypt/decrypt in-place, which is a separate piece of the code which appears to be correct. James
On 21 October 2018 at 10:07, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > On Sun, 2018-10-21 at 09:05 +0200, Ard Biesheuvel wrote: >> (+ James) > > Thanks! > >> On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov >> <dbaryshkov@gmail.com> wrote: >> > crypto_cfb_decrypt_segment() incorrectly XOR'ed generated keystream >> > with >> > IV, rather than with data stream, resulting in incorrect >> > decryption. >> > Test vectors will be added in the next patch. >> > >> > Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> >> > Cc: stable@vger.kernel.org >> > --- >> > crypto/cfb.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/crypto/cfb.c b/crypto/cfb.c >> > index a0d68c09e1b9..fd4e8500e121 100644 >> > --- a/crypto/cfb.c >> > +++ b/crypto/cfb.c >> > @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct >> > skcipher_walk *walk, >> > >> > do { >> > crypto_cfb_encrypt_one(tfm, iv, dst); >> > - crypto_xor(dst, iv, bsize); >> > + crypto_xor(dst, src, bsize); > > This does look right. I think the reason the TPM code works is that it > always does encrypt/decrypt in-place, which is a separate piece of the > code which appears to be correct. > Yeah I figured that. So where is the TPM code that actually uses this code?
On October 21, 2018 9:58:04 AM GMT, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >On 21 October 2018 at 10:07, James Bottomley ><James.Bottomley@hansenpartnership.com> wrote: >> On Sun, 2018-10-21 at 09:05 +0200, Ard Biesheuvel wrote: >>> (+ James) >> >> Thanks! >> >>> On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov >>> <dbaryshkov@gmail.com> wrote: >>> > crypto_cfb_decrypt_segment() incorrectly XOR'ed generated >keystream >>> > with >>> > IV, rather than with data stream, resulting in incorrect >>> > decryption. >>> > Test vectors will be added in the next patch. >>> > >>> > Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> >>> > Cc: stable@vger.kernel.org >>> > --- >>> > crypto/cfb.c | 2 +- >>> > 1 file changed, 1 insertion(+), 1 deletion(-) >>> > >>> > diff --git a/crypto/cfb.c b/crypto/cfb.c >>> > index a0d68c09e1b9..fd4e8500e121 100644 >>> > --- a/crypto/cfb.c >>> > +++ b/crypto/cfb.c >>> > @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct >>> > skcipher_walk *walk, >>> > >>> > do { >>> > crypto_cfb_encrypt_one(tfm, iv, dst); >>> > - crypto_xor(dst, iv, bsize); >>> > + crypto_xor(dst, src, bsize); >> >> This does look right. I think the reason the TPM code works is that >it >> always does encrypt/decrypt in-place, which is a separate piece of >the >> code which appears to be correct. >> > >Yeah I figured that. > >So where is the TPM code that actually uses this code? It was posted to the integrity list a while ago. I'm planning a repost shortly. James
On 21 October 2018 at 11:00, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > On October 21, 2018 9:58:04 AM GMT, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>On 21 October 2018 at 10:07, James Bottomley >><James.Bottomley@hansenpartnership.com> wrote: >>> On Sun, 2018-10-21 at 09:05 +0200, Ard Biesheuvel wrote: >>>> (+ James) >>> >>> Thanks! >>> >>>> On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov >>>> <dbaryshkov@gmail.com> wrote: >>>> > crypto_cfb_decrypt_segment() incorrectly XOR'ed generated >>keystream >>>> > with >>>> > IV, rather than with data stream, resulting in incorrect >>>> > decryption. >>>> > Test vectors will be added in the next patch. >>>> > >>>> > Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> >>>> > Cc: stable@vger.kernel.org >>>> > --- >>>> > crypto/cfb.c | 2 +- >>>> > 1 file changed, 1 insertion(+), 1 deletion(-) >>>> > >>>> > diff --git a/crypto/cfb.c b/crypto/cfb.c >>>> > index a0d68c09e1b9..fd4e8500e121 100644 >>>> > --- a/crypto/cfb.c >>>> > +++ b/crypto/cfb.c >>>> > @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct >>>> > skcipher_walk *walk, >>>> > >>>> > do { >>>> > crypto_cfb_encrypt_one(tfm, iv, dst); >>>> > - crypto_xor(dst, iv, bsize); >>>> > + crypto_xor(dst, src, bsize); >>> >>> This does look right. I think the reason the TPM code works is that >>it >>> always does encrypt/decrypt in-place, which is a separate piece of >>the >>> code which appears to be correct. >>> >> >>Yeah I figured that. >> >>So where is the TPM code that actually uses this code? > > It was posted to the integrity list a while ago. I'm planning a repost shortly. > OK, found it. Mind cc'ing me on that repost?
Hello, вс, 21 окт. 2018 г. в 11:07, James Bottomley <James.Bottomley@hansenpartnership.com>: > > On Sun, 2018-10-21 at 09:05 +0200, Ard Biesheuvel wrote: > > (+ James) > > Thanks! > > > On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov > > <dbaryshkov@gmail.com> wrote: > > > crypto_cfb_decrypt_segment() incorrectly XOR'ed generated keystream > > > with > > > IV, rather than with data stream, resulting in incorrect > > > decryption. > > > Test vectors will be added in the next patch. > > > > > > Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> > > > Cc: stable@vger.kernel.org > > > --- > > > crypto/cfb.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/crypto/cfb.c b/crypto/cfb.c > > > index a0d68c09e1b9..fd4e8500e121 100644 > > > --- a/crypto/cfb.c > > > +++ b/crypto/cfb.c > > > @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct > > > skcipher_walk *walk, > > > > > > do { > > > crypto_cfb_encrypt_one(tfm, iv, dst); > > > - crypto_xor(dst, iv, bsize); > > > + crypto_xor(dst, src, bsize); > > This does look right. I think the reason the TPM code works is that it > always does encrypt/decrypt in-place, which is a separate piece of the > code which appears to be correct. Since 4.20 pull went into Linus'es tree, any change of getting these two patches in crypto tree?
On Thu, Nov 01, 2018 at 11:32:37AM +0300, Dmitry Eremin-Solenikov wrote: > > Since 4.20 pull went into Linus'es tree, any change of getting these two patches > in crypto tree? These aren't critical enough for the current mainline so they will go in at the next merge window. Cheers,
чт, 1 нояб. 2018 г. в 11:41, Herbert Xu <herbert@gondor.apana.org.au>: > > On Thu, Nov 01, 2018 at 11:32:37AM +0300, Dmitry Eremin-Solenikov wrote: > > > > Since 4.20 pull went into Linus'es tree, any change of getting these two patches > > in crypto tree? > > These aren't critical enough for the current mainline so they will > go in at the next merge window. Thank you.
On Sat, Oct 20, 2018 at 02:01:52AM +0300, Dmitry Eremin-Solenikov wrote: > crypto_cfb_decrypt_segment() incorrectly XOR'ed generated keystream with > IV, rather than with data stream, resulting in incorrect decryption. > Test vectors will be added in the next patch. > > Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> > Cc: stable@vger.kernel.org > --- > crypto/cfb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) All applied. Thanks.
diff --git a/crypto/cfb.c b/crypto/cfb.c index a0d68c09e1b9..fd4e8500e121 100644 --- a/crypto/cfb.c +++ b/crypto/cfb.c @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct skcipher_walk *walk, do { crypto_cfb_encrypt_one(tfm, iv, dst); - crypto_xor(dst, iv, bsize); + crypto_xor(dst, src, bsize); iv = src; src += bsize;
crypto_cfb_decrypt_segment() incorrectly XOR'ed generated keystream with IV, rather than with data stream, resulting in incorrect decryption. Test vectors will be added in the next patch. Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> Cc: stable@vger.kernel.org --- crypto/cfb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)