Linux-Crypto Archive on lore.kernel.org
 help / color / Atom feed
From: "Ondrej Mosnáček" <omosnacek+linux-crypto@gmail.com>
To: torvalds@linux-foundation.org
Cc: "Herbert Xu" <herbert@gondor.apana.org.au>,
	"David S. Miller" <davem@davemloft.net>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	linux-crypto@vger.kernel.org, "Milan Brož" <gmazyland@gmail.com>
Subject: Re: Crypto Fixes for 4.18
Date: Mon, 9 Jul 2018 11:47:42 +0200
Message-ID: <CAAUqJDsHzgGZTpYCAxAy=b3P0bq7dGrw6St68SkwjqwHpusN+A@mail.gmail.com> (raw)
In-Reply-To: <CA+55aFwe3HrgMBUhyHC7kAOCvp31S4kpAqDSFQ+BQY6r=WBiiA@mail.gmail.com>

Hi Linus,

ne 8. 7. 2018 o 20:32 Linus Torvalds <torvalds@linux-foundation.org> napísal(a):
>
> On Sun, Jul 8, 2018 at 9:20 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > - Add missing RETs in x86 aegis/morus.
>
> Side note - I queried earlier during the discussion about this: how
> was this code taken despite having clearly never tested on _anything_?
>
> That's a serious question. Code that simply has never had any testing
> AT ALL should not have gotten in.

I did test the code using the included test vectors (and I found and
resolved lots of issues before submitting the patches thanks to that).
A good deal of the test vectors actually do trigger the code path that
calls the buggy function, so somehow it must have been working despite
the bug (see below).

> The use of 'int3' in padding showed the issue, but I don't believe the
> code could possibly have worked with the nops and fallthroughs.

I just looked at the disassembly of the function and its surroundings
(as compiled by my testing environment) and it seems that by a curious
but logical coincidence, the code actually *did* work and without any
side effects (other than executing a few useless instructions before
returning).

This is what the C signatures of the relevant functions look like (for
aegis128, the other cases are analogical):

asmlinkage void crypto_aegis128_aesni_enc_tail(
        void *state, unsigned int length, const void *src, void *dst);

asmlinkage void crypto_aegis128_aesni_dec(
        void *state, unsigned int length, const void *src, void *dst);

Notice that these two functions have identical signatures, this will
be important later. Now, the disassembly for
crypto_aegis128_aesni_enc_tail looks roughly like this:

0000000000000950 <crypto_aegis128_aesni_enc_tail>:
 [some code...]
 9c3:   0f 1f 00                nopl   (%rax)
 9c6:   66 2e 0f 1f 84 00 00    nopw   %cs:0x0(%rax,%rax,1)
 9cd:   00 00 00

00000000000009d0 <crypto_aegis128_aesni_dec>:
 9d0:   48 83 fe 10             cmp    $0x10,%rsi
 9d4:   0f 82 c3 03 00 00       jb     d9d <crypto_aegis128_aesni_dec+0x3cd>
 [some code...]
 d9d:   c3                      retq    # <---
<crypto_aegis128_aesni_dec+0x3cd> is here
 d9e:   66 90                   xchg   %ax,%ax

So... thanks to the NOP padding, the control after the end of the
_enc_tail function walks right into the _dec function. This looks
scary at first glance, but here we are "saved" by the combination of
the following:
1. The second argument of the _enc_tail function (length; passed via
%rsi) is implictly always less than the block size (16 or 32 bytes).
2. The second argument of the _dec function (length; also passed via
%rsi) is checked to be greater than or equal to the block size (16 or
32 bytes); if it is less, then the function does nothing and just
returns.
3. _enc_tail does not modify the value in %rsi.

In conclusion, the bug remained undiscovered not because of lack of
testing, but because by sheer luck it was "working" anyway...

Sorry for introducing this (and other) bugs that had to be fixed
post-merging (I am the one who wrote the code). It is a lot of new
code that is hard to review, as it contains a lot of repetitive
boilerplate and assembly code.

Cheers,
Ondrej

  reply index

Thread overview: 120+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-19 10:27 Crypto Fixes for 4.9 Herbert Xu
2016-11-23  5:36 ` Herbert Xu
2016-12-05  6:37 ` Herbert Xu
2016-12-10  6:01   ` Herbert Xu
2016-12-13 13:24 ` Crypto Update for 4.10 Herbert Xu
2017-02-23 12:51   ` Crypto Update for 4.11 Herbert Xu
2017-05-01 14:26     ` Crypto Update for 4.12 Herbert Xu
2017-07-05 13:01     ` Crypto Update for 4.13 Herbert Xu
2017-07-05 20:02       ` Linus Torvalds
2017-07-06  2:19         ` Herbert Xu
2017-07-14 14:18     ` Crypto Fixes " Herbert Xu
2017-07-28  8:49       ` Herbert Xu
2017-08-09 12:04         ` Herbert Xu
2017-08-14  9:43           ` Herbert Xu
2017-09-01  7:53             ` Herbert Xu
2017-09-22  8:44       ` Crypto Fixes for 4.14 Herbert Xu
2017-10-12 10:51         ` Herbert Xu
2017-10-30  7:20           ` Herbert Xu
2017-11-06  6:37             ` Herbert Xu
2017-11-28 23:09         ` Crypto Fixes for 4.15 Herbert Xu
2017-12-11  7:22           ` Herbert Xu
2017-12-22  6:49           ` Herbert Xu
2018-01-05  7:38             ` Herbert Xu
2018-01-12  6:44               ` Herbert Xu
2018-02-12  3:17               ` Crypto Fixes for 4.16 Herbert Xu
2018-04-28  8:05                 ` Crypto Fixes for 4.17 Herbert Xu
2018-05-30 16:17                   ` Herbert Xu
2018-06-22 14:54                   ` Crypto Fixes for 4.18 Herbert Xu
2018-07-08 16:20                     ` Herbert Xu
2018-07-08 18:31                       ` Linus Torvalds
2018-07-09  9:47                         ` Ondrej Mosnáček [this message]
2018-07-09 15:52                           ` Linus Torvalds
2018-07-19  7:39                     ` Herbert Xu
2018-08-03  5:44                       ` Herbert Xu
2018-08-09  5:47                         ` Herbert Xu
2018-08-29  3:33                     ` Crypto Fixes for 4.19 Herbert Xu
2018-09-19  6:22                       ` Herbert Xu
2018-09-19 13:23                         ` Greg KH
2018-09-19 16:10                           ` process? [Re: Crypto Fixes for 4.19] Randy Dunlap
2018-09-19 16:40                             ` Greg KH
2018-09-19 16:48                               ` Randy Dunlap
2018-09-19 17:00                                 ` Willy Tarreau
2018-10-05  2:08                         ` Crypto Fixes for 4.19 Herbert Xu
2018-10-05 15:37                           ` Greg KH
2018-11-16  6:31                       ` Crypto Fixes for 4.20 Herbert Xu
2018-12-07  6:14                         ` Herbert Xu
2019-01-18 10:40                           ` Crypto Fixes for 5.0 Herbert Xu
2019-02-01  5:42                             ` Herbert Xu
2019-02-01  7:37                               ` Linus Torvalds
2019-02-15  2:47                               ` [GIT] " Herbert Xu
2019-02-15 17:10                                 ` pr-tracker-bot
2019-02-28  5:56                                 ` Herbert Xu
2019-03-02  0:20                                   ` pr-tracker-bot
2019-03-02  2:42                                   ` Herbert Xu
2019-03-02 17:10                                     ` pr-tracker-bot
2019-03-05  8:11                                 ` [GIT] Crypto Update for 5.1 Herbert Xu
2019-03-05 17:40                                   ` pr-tracker-bot
2019-05-06  3:29                                   ` [GIT] Crypto Update for 5.2 Herbert Xu
2019-05-07  3:25                                     ` pr-tracker-bot
2019-07-08 15:08                                     ` [GIT] Crypto Update for 5.3 Herbert Xu
2019-07-09  4:45                                       ` pr-tracker-bot
2019-03-12  4:58                                 ` [GIT] Crypto Fixes for 5.1 Herbert Xu
2019-03-13 17:10                                   ` pr-tracker-bot
2019-04-08  5:48                                   ` Herbert Xu
2019-04-08  6:25                                     ` pr-tracker-bot
2019-04-18  5:17                                     ` Herbert Xu
2019-04-18 15:20                                       ` pr-tracker-bot
2019-04-25  7:26                                       ` Herbert Xu
2019-04-25 16:25                                         ` pr-tracker-bot
2019-05-15  6:05                                   ` [GIT] Crypto Fixes for 5.2 Herbert Xu
2019-05-15 16:10                                     ` pr-tracker-bot
2019-05-21 12:58                                     ` Herbert Xu
2019-05-21 19:55                                       ` pr-tracker-bot
2019-06-06  6:03                                     ` Herbert Xu
2019-06-06 20:20                                       ` pr-tracker-bot
2019-07-05  4:24                                       ` Herbert Xu
2019-07-05  4:40                                         ` pr-tracker-bot
2019-07-19  3:12                                     ` [GIT] Crypto Fixes for 5.3 Herbert Xu
2019-07-19 19:45                                       ` pr-tracker-bot
2019-08-09  6:15                                       ` Herbert Xu
2019-08-09 16:35                                         ` pr-tracker-bot
2019-08-30  7:39                                         ` Herbert Xu
2019-08-31  2:01                                           ` Linus Torvalds
2019-08-31 12:12                                             ` Herbert Xu
2019-08-31  2:10                                           ` pr-tracker-bot
2017-09-04 10:12     ` Crypto Update for 4.14 Herbert Xu
2017-11-13  7:43       ` Crypto Update for 4.15 Herbert Xu
2018-01-29 14:50         ` Crypto Update for 4.16 Herbert Xu
2018-04-04 15:27           ` Crypto Update for 4.17 Herbert Xu
2018-06-04 17:15             ` Crypto Update for 4.18 Herbert Xu
2018-08-15 12:05               ` Crypto Update for 4.19 Herbert Xu
2018-10-23 10:09         ` Crypto Update for 4.20 Herbert Xu
2018-10-25 23:46           ` Linus Torvalds
2018-12-26 13:22           ` Crypto Update for 4.21 Herbert Xu
2018-12-26 16:49             ` Eric Biggers
2018-12-27  1:03               ` Herbert Xu
2016-12-15 16:07 ` Crypto Fixes for 4.10 Herbert Xu
2016-12-27  9:45   ` Herbert Xu
2016-12-30 10:19     ` Herbert Xu
2017-01-11 11:56   ` Herbert Xu
2017-02-01  9:04     ` Herbert Xu
2017-02-06  9:25       ` Herbert Xu
2017-03-04  7:41     ` Crypto Fixes for 4.11 Herbert Xu
2017-03-15  6:31       ` Herbert Xu
2017-03-24 13:46         ` Herbert Xu
2017-03-31 10:29           ` Herbert Xu
2017-04-10 11:04             ` Herbert Xu
2017-04-18 10:27               ` Herbert Xu
2017-05-23  3:42       ` Crypto Fixes for 4.12 Herbert Xu
2017-06-08  9:23         ` Herbert Xu
2017-06-08 14:05           ` David Miller
2017-06-09  2:52             ` Herbert Xu
2017-06-15  0:54           ` Herbert Xu
2017-06-15  9:04             ` Linus Torvalds
2017-06-15  9:05               ` Linus Torvalds
2017-06-15  9:42               ` Herbert Xu
2017-06-15 15:02                 ` David Miller
2017-06-15 15:01               ` David Miller
2017-06-16 12:50                 ` Theodore Ts'o
2017-06-16 16:49                   ` David Miller

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAAUqJDsHzgGZTpYCAxAy=b3P0bq7dGrw6St68SkwjqwHpusN+A@mail.gmail.com' \
    --to=omosnacek+linux-crypto@gmail.com \
    --cc=davem@davemloft.net \
    --cc=gmazyland@gmail.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Crypto Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-crypto/0 linux-crypto/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-crypto linux-crypto/ https://lore.kernel.org/linux-crypto \
		linux-crypto@vger.kernel.org
	public-inbox-index linux-crypto

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-crypto


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git