Bug 1122937 - aarch64 AES crypto: block misalignment causes crash or data corruption and cryptsetup fails to activate partitions
Summary: aarch64 AES crypto: block misalignment causes crash or data corruption and cr...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: kernel
Version: 21
Hardware: aarch64
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Mikuláš Patočka
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: TRACKER-bugs-affecting-libguestfs 1123522
TreeView+ depends on / blocked
 
Reported: 2014-07-24 12:49 UTC by Richard W.M. Jones
Modified: 2014-07-29 13:53 UTC (History)
12 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
: 1123522 (view as bug list)
Environment:
Last Closed: 2014-07-29 13:53:10 UTC


Attachments (Terms of Use)
cryptsetup-test-log.txt (44.88 KB, text/plain)
2014-07-25 09:33 UTC, Richard W.M. Jones
no flags Details
cryptsetup dmesg output during test (39.48 KB, text/plain)
2014-07-25 09:34 UTC, Richard W.M. Jones
no flags Details
A patch for the bug (3.11 KB, patch)
2014-07-25 23:17 UTC, Mikuláš Patočka
no flags Details | Diff
A patch for a similar bug in arm32 code (2.07 KB, patch)
2014-07-25 23:18 UTC, Mikuláš Patočka
no flags Details | Diff

Description Richard W.M. Jones 2014-07-24 12:49:33 UTC
Description of problem:

On aarch64, cryptsetup cannot activate partitions, or rather, it
can sometimes for reasons that don't make any rational sense.
Here is an example:

# lvcreate -L 1G -n tmptest vg_ssd
# echo 123456 > /tmp/key
# cryptsetup --key-slot 0 luksFormat /dev/vg_ssd/tmptest /tmp/key
# file -bsL /dev/vg_ssd/tmptest 
LUKS encrypted file, ver 1 [aes, xts-plain64, sha1] UUID: 0427b734-4ec2-483d-b814-53dcca51d800

So far, this works.  However activating the partition does not:

# cryptsetup luksOpen /dev/vg_ssd/tmptest lukstest
Enter passphrase for /dev/vg_ssd/tmptest: <type 123456> 
No key available with this passphrase.
Enter passphrase for /dev/vg_ssd/tmptest: ^C

However if you specify the key file, it works:

# cryptsetup -d /tmp/key luksOpen /dev/vg_ssd/tmptest lukstest

I've been trying to debug this and it looks like it might be
a code generation bug inside libgcrypt, but I don't have anything
definite at the moment.

Unfortunately valgrind fails on cryptsetup because of a separate
bug (https://bugs.kde.org/show_bug.cgi?id=337762).

Version-Release number of selected component (if applicable):

cryptsetup-1.6.5-2.fc21.aarch64

How reproducible:

100%

Steps to Reproduce:
1. See above.

Comment 1 Milan Broz 2014-07-24 13:13:10 UTC
First, are you sure that your key file does not contain EOL char (see man page, for historic reason it is so tricky here...)?

Because in keyfiles this is processed as part of key but in terminal input it EOL mean end of passphrase input.

(IOW try "echo -n 123456 >/tmp/key" and repeat the test ;-)


If it itill fails, you can try to eliminate gcrypt by simply recompile cryptsetup with another crypto backend, I would suggest kernel (because it will not use any userspace library - just it will be slow).

If you want to five it a try, add --with-crypto_backend=kernel to spec and recompile the whole cryptsetup/libcryptsetup.

Also please paste output of working and non-working command with added --debug, there is a lot more info, thanks.

Comment 2 Richard W.M. Jones 2014-07-24 13:32:37 UTC
Yup, actually I used echo -n, not echo so it's fine:

$ hexdump -C /tmp/key 
00000000  31 32 33 34 35 36                                 |123456|
00000006

Using --with-crypto_backend=kernel fails in the same way.

Enabling --debug is fun.  It *changes* the behaviour of the good
case so it fails as well.

BTW I really think this is an aarch64-specific code gen bug.
For some reason af.c:AF_merge produces different results from
exactly the same input in the two different cases.  The only
difference between the two cases would be memory/stack alignment.

Comment 3 Milan Broz 2014-07-24 13:50:38 UTC
Hm. The --debug should not change anything. So this looks like some nasty alignment bug... (Also passphrase and keyfile is for keyslot processing exactly the same, just buffer of data. So that it worked sometimes was just luck I guess...)

Unfortunately I do not have access to any aarch64 machine. Are you sure the bug is in af.c? This is something I would like to definitely fix upstream asap.
(But that code run on many strange architectures without problem.)

Comment 4 Richard W.M. Jones 2014-07-24 14:01:21 UTC
Sorry the hardware is covered by an NDA so we cannot give out access
to it (yet).

I'm not sure that AF_merge is the problem.  However does below look
right or wrong to you?  Should the outputs be different, given that
the inputs are the same?

The failure happens immediately afterwards in LUKS_verify_volume_key()
where the only difference between the two cases is the content of vk->key.
vk->key is generated by AF_merge.  Hence I'm looking at AF_merge.

Another thing: char is unsigned on ARM.

-- AF_merge entry (af.c:128) --

good case:

Breakpoint 1, AF_merge (
    src=src@entry=0x4242f8 "\371h\nյ{\372\315 +\243gw<\264\006h\333\372f\355\374\274\265\241\235o\256\001\177\371\223|\021\275\020\016[3\271]8\034\226FN\206\017\245}\r\340\323\361*\v\224v\262\224XNTl\302\024\220\320T\025\235\203", <incomplete sequence \303>, dst=dst@entry=0x423158 "", blocksize=32, 
    blocknumbers=4000, hash=hash@entry=0x4228d8 "sha1") at af.c:130

failing case:

Breakpoint 1, AF_merge (
    src=src@entry=0x4233e8 "\371h\nյ{\372\315 +\243gw<\264\006h\333\372f\355\374\274\265\241\235o\256\001\177\371\223|\021\275\020\016[3\271]8\034\226FN\206\017\245}\r\340\323\361*\v\224v\262\224XNTl\302\024\220\320T\025\235\203", <incomplete sequence \303>, dst=dst@entry=0x4230a8 "", blocksize=32, 
    blocknumbers=4000, hash=hash@entry=0x422848 "sha1") at af.c:130

[The inputs look identical to me]

-- AF_merge exit (af.c:145) --

good:
Breakpoint 2, AF_merge (
    src=src@entry=0x4242f8 "\371h\nյ{\372\315 +\243gw<\264\006h\333\372f\355\374\274\265\241\235o\256\001\177\371\223|\021\275\020\016[3\271]8\034\226FN\206\017\245}\r\340\323\361*\v\224v\262\224XNTl\302\024\220\320T\025\235\203", <incomplete sequence \303>, 
    dst=dst@entry=0x423158 "\274\246\"~\264[R϶\267>\231ũ}\033\254\317\322\037\232t\267cZ]\302\v\305\a[\260!", blocksize=32, blocknumbers=4000, 
    hash=hash@entry=0x4228d8 "sha1") at af.c:145

bad:
Breakpoint 1, AF_merge (
    src=src@entry=0x4233e8 "\371h\nյ{\372\315 +\243gw<\264\006h\333\372f\355\374\274\265\241\235o\256\001\177\371\223|\021\275\020\016[3\271]8\034\226FN\206\017\245}\r\340\323\361*\v\224v\262\224XNTl\302\024\220\320T\025\235\203", <incomplete sequence \303>, 
    dst=dst@entry=0x4230a8 "c\271t\335\026M<8\243\371\024\276\336\353V+J\033e\353\377R\360~\346[?", blocksize=32, blocknumbers=4000, 
    hash=hash@entry=0x422848 "sha1") at af.c:145

Comment 5 Milan Broz 2014-07-24 15:01:58 UTC
With the same header and keyslot  this code is deterministic, so it must always return the same key.

And id unsigned char is the problem it should fail every time.

If you replace calloc() with e.g. posix_memalign() is it still reproducible?

BTW does cryptsetup 1.6.4 work or it is old bug?

Anyway, if there is no clear solution soon, Ondra can try to debug it once is back from vacation... ;-)

Comment 6 Richard W.M. Jones 2014-07-24 15:53:05 UTC
(In reply to Milan Broz from comment #5)
> If you replace calloc() with e.g. posix_memalign() is it still reproducible?

I didn't specifically try this, but since we fixed valgrind[1]
I played around with the --alignment option in valgrind and that
made no difference, even forcing large alignment.

[1] https://bugs.kde.org/show_bug.cgi?id=337762

> BTW does cryptsetup 1.6.4 work or it is old bug?

Both commands fail in 1.6.4.

This surely must be an obscure code gen bug ...

Comment 7 Mikuláš Patočka 2014-07-24 18:42:03 UTC
It could be due to a compiler bug or due to undefined behavior (for example, too
big shift count).

I suggest - compile cryptsetup with -O0 -fsanitize=undefined (you need at least
gcc-4.9 for -fsanitize=undefined).

If it works with -O0, the failure is likely caused by optimization bug.
If you get some error from the sanitizer, it means that you are doing undefined
operation.
If it still doesn't work (and doesn't print any error), try compiling the crypto
library cryptsetup is using with -O0 -fsanitize=undefined too.

Comment 8 Richard W.M. Jones 2014-07-24 19:11:35 UTC
Thanks will try this.  The next private reply contains details of
how Red Hat employees can get access to a machine for dev/testing.

Comment 9 Milan Broz 2014-07-24 19:14:26 UTC
(In reply to Mikulas Patocka from comment #7)
> If it still doesn't work (and doesn't print any error), try compiling the
> crypto
> library cryptsetup is using with -O0 -fsanitize=undefined too.

Just note: the main point of testing with --with-crypto_backend=kernel is that there is no crypto library used, all crypto operations are done through kernel crypto API socket. So it is probably ideal for testing these bugs because it eliminates linked crypto libraries bugs (well, kernel has some bugs too for sure :).

I see that current koji builds are using gcc-4.9, so using that options Mikulas is suggesting is very good idea...

When testing, I would suggest to run in loop LUKS device open and header keyslots reencrypt - this will utilise the AF code to the maximum with both write and read operations, IOW run

echo "123456" | cryptsetup-reencrypt --keep-key --use-urandom --iter-time 1 <device>
echo "123456" | cryptsetup luksOpen --test-passphrase <device>

Comment 11 Milan Broz 2014-07-24 19:29:55 UTC
Anyway, I couldn't resist but what's the point in reporting Fedora bug on architecture which package maintainer cannot access because it is under NDA? :-)
</rant>

The real question is: is it reproducible with ARM AArch64 foundation model (which I hope is available for me)?

Comment 12 Richard W.M. Jones 2014-07-24 19:32:22 UTC
I'm afraid the sanity checker for aarch64 has only just arrived upstream in
gcc, and only works with 4K pages (we use 64K pages).

Using -O0 didn't fix it.

Currently going through warnings.

Comment 13 Richard W.M. Jones 2014-07-24 19:35:02 UTC
(In reply to Milan Broz from comment #11)
> Anyway, I couldn't resist but what's the point in reporting Fedora bug on
> architecture which package maintainer cannot access because it is under NDA?
> :-)
> </rant>

You should be able to buy one real soon.  AllWinner are going into full
production after October, and there are others coming out sooner.  But
yes it's not an ideal situation.

> The real question is: is it reproducible with ARM AArch64 foundation model
> (which I hope is available for me)?

Assuming this bug doesn't depend on kernel code (which is unlikely),
a much easier and faster way to do it is to follow the instructions here:

http://rwmj.wordpress.com/2013/12/22/how-to-run-aarch64-binaries-on-an-x86-64-host-using-qemu-userspace-emulation/#content

Comment 14 Richard W.M. Jones 2014-07-24 19:36:54 UTC
And BTW you can just use upstream qemu now, don't need to use
the special qemu branch.

Comment 15 Richard W.M. Jones 2014-07-24 20:16:21 UTC
After following Alasdair's suggestion, and tracing AF_merge on x86_64
and aarch64, I've found the point where the trace diverges, for me.

I am now using cryptsetup from git with the following command line:

sudo LD_LIBRARY_PATH=/home/rjones/d/cryptsetup/lib/.libs libtool --mode=execute gdb --args ./src/cryptsetup luksOpen /dev/fedora/tmptest lukstest

I placed a breakpoint at af.c:140.

I ran both loops until i = 97 at which point it diverges.

---- On x86_64 ----

Breakpoint 1, AF_merge (
    src=0x613b88 "\371h\nյ{\372\315 +\243gw<\264\006h\333\372f\355\374\274\265\241\235o\256\001\177\371\223|\021\275\020\016[3\271]8\034\226FN\206\017\245}\r\340\323\361*\v\224v\262\224XNTl\302\024\220\320T\025\235\203", <incomplete sequence \303>, dst=0x6138b8 "", blocksize=32, blocknumbers=4000, 
    hash=0x612848 "sha1") at af.c:140
140			XORblock(src+(blocksize*i),bufblock,bufblock,blocksize);
(gdb) print bufblock
$58 = 0x632f90 "\272\241w\337Zx\004\341;i\231*rA\345\f\001l\276\231&ʜ\347%\254\346\256@\nƃ"
(gdb) cont
Continuing.

Breakpoint 1, AF_merge (
    src=0x613b88 "\371h\nյ{\372\315 +\243gw<\264\006h\333\372f\355\374\274\265\241\235o\256\001\177\371\223|\021\275\020\016[3\271]8\034\226FN\206\017\245}\r\340\323\361*\v\224v\262\224XNTl\302\024\220\320T\025\235\203", <incomplete sequence \303>, dst=0x6138b8 "", blocksize=32, blocknumbers=4000, 
    hash=0x612848 "sha1") at af.c:140
140			XORblock(src+(blocksize*i),bufblock,bufblock,blocksize);
(gdb) print bufblock
$59 = 0x632f90 "DW\243 \032%\374\200\276؉\217\017\n\"\332\370\f\022\215\330N\026ٔ{\367\331\063q[\204"
(gdb) print i
$60 = 97

---- On aarch64 ----

Breakpoint 1, AF_merge (
    src=0x4243e8 "\371h\nյ{\372\315 +\243gw<\264\006h\333\372f\355\374\274\265\241\235o\256\001\177\371\223|\021\275\020\016[3\271]8\034\226FN\206\017\245}\r\340\323\361*\v\224v\262\224XNTl\302\024\220\320T\025\235\203", <incomplete sequence \303>, dst=0x4240a8 "", blocksize=32, blocknumbers=4000, 
    hash=0x423848 "sha1") at af.c:140
140			XORblock(src+(blocksize*i),bufblock,bufblock,blocksize);
(gdb) print bufblock
$59 = 0x424140 "\272\241w\337Zx\004\341;i\231*rA\345\f\001l\276\231&ʜ\347%\254\346\256@\nƃ"
(gdb) cont
Continuing.

Breakpoint 1, AF_merge (
    src=0x4243e8 "\371h\nյ{\372\315 +\243gw<\264\006h\333\372f\355\374\274\265\241\235o\256\001\177\371\223|\021\275\020\016[3\271]8\034\226FN\206\017\245}\r\340\323\361*\v\224v\262\224XNTl\302\024\220\320T\025\235\203", <incomplete sequence \303>, dst=0x4240a8 "", blocksize=32, blocknumbers=4000, 
    hash=0x423848 "sha1") at af.c:140
140			XORblock(src+(blocksize*i),bufblock,bufblock,blocksize);
(gdb) print bufblock
$60 = 0x424140 "N\374\310\033\273\031OYf\366\231p\265\016\371\006\344\235\tN\317K\221\002\nռB3\005\063\266"
(gdb) print i
$61 = 97
(gdb) print blocksize
$62 = 32
(gdb) print src+(blocksize*i)
$63 = 0x425008 "EzBA\"\240(<S\325\062\016\311.\343\"\376\377"
(gdb) print hash
$64 = 0x423848 "sha1"

Comment 16 Richard W.M. Jones 2014-07-24 20:23:48 UTC
Of course now after posting that I realize my methodology mistake,
which is that 'src' is not completely identical for both inputs.

The source string is different starting at AfKey[3087].  All previous
bytes in AfKey appear to be identical.

It's very strange because we know the preceeding call to
LUKS_decrypt_from_storage did not fail.

Comment 17 Mikuláš Patočka 2014-07-25 01:16:07 UTC
I found out that when we enable unconditionally this fallback piece of code in cryptsetup sources, it works on arm64. I will continue to analyze it tomorrow.

        /* Fallback to old temporary dmcrypt device */
        if (r == -ENOTSUP || r == -ENOENT || 1)
                return LUKS_endec_template(dst, dstLength, cipher, cipher_mode,
                                           vk, sector, read_blockwise, O_RDONLY, ctx);

        /* Fallback to old temporary dmcrypt device */
        if (r == -ENOTSUP || r == -ENOENT || 1)
                return LUKS_endec_template(src, srcLength, cipher, cipher_mode,
                                           vk, sector, write_blockwise, O_RDWR, ctx);

Comment 18 Milan Broz 2014-07-25 07:19:02 UTC
(In reply to Mikulas Patocka from comment #17)
> I found out that when we enable unconditionally this fallback piece of code
> in cryptsetup sources, it works on arm64. I will continue to analyze it
> tomorrow.

But this is why I asked if it works in 1.6.4 - this part is new code in 1.6.5.

So then I guess is bug in crypto_storage.c. Please use upstream git then, there are already some fixes (it should not be relevant here but...:)
(Also note kernel selinux bug#1115120)

The whole part of this code is replacing encryption of keyslot area by userspace code which utilizes kernel crypto API.

Comment 19 Richard W.M. Jones 2014-07-25 08:02:33 UTC
(In reply to Milan Broz from comment #18)
> (Also note kernel selinux bug#1115120)

SELinux is permissive on this machine.

Comment 20 Milan Broz 2014-07-25 08:43:55 UTC
Just if Mikulas or anyone has access to this hw: please could you run upstream regression tests? (just "make check" with upstream repo, not only 1.6.5 tar).

I added recently some LUKS images which should catch these kind of problems.
(But these images are specially modified with zeroed keyslots areas to compress well, so it is kind of synthetic test here.)

Anyway, it would be nice to know if this problem have reproducer in tests to not repeat this in future.

Thanks!

Comment 21 Richard W.M. Jones 2014-07-25 09:33:46 UTC
Created attachment 920928 [details]
cryptsetup-test-log.txt

Quite a lot of failures.  Also a lot of errors in the kernel
messages which I'll attach in a minute.

This is with cryptsetup.git @ 59fdf2a6bb461a39e6db6b7d515873419f8a8ada

Comment 22 Richard W.M. Jones 2014-07-25 09:34:33 UTC
Created attachment 920929 [details]
cryptsetup dmesg output during test

Comment 23 Richard W.M. Jones 2014-07-25 09:36:32 UTC
Another odd thing, maybe.  When I copied the test LUKS partition
(created in comment 0) from the aarch64 machine to an x86-64
machine, I could not open it at all.  Is that expected?  Or does
that indicate that the luksFormat command has created a broken
LUKS partition?  I did `md5sum' on both the source and destination
copies of this partition and they are bit for bit identical.

Comment 24 Milan Broz 2014-07-25 09:49:48 UTC
Thanks!

ok, something is apparently broken on that arch (that segfault is very strange).
I am trying to setup qemu here, but I have quite limited time (and my testing environment is Debian now so will see if it works).

Anyway, if read fails, I would expect new LUKS header to be corrupted as well.

The error messages in syslog (IO error, corrupted verity device) are expected but the crash I see there appears like a kernel bug.
Mikulas can analyse this better with access to this system.

Comment 25 Richard W.M. Jones 2014-07-25 10:17:34 UTC
(In reply to Milan Broz from comment #24)
> ok, something is apparently broken on that arch (that segfault is very
> strange).

I made a valiant attempt to collect core dumps (for stack traces),
but somehow even with ulimit -c unlimited and
/proc/sys/kernel/core_pattern set correctly, it's not producing
any core dumps.

Comment 26 Mikuláš Patočka 2014-07-25 15:43:00 UTC
The corruption is in crypt_storage_decrypt (and crypt_storage_encrypt). It receives the same data as on x86-64, yet it produces different result. These functions just call the kernel, so the bug is probably in the kernel.

The bug is page-alignment-related. The resulting data is corrupted in 80-byte chunks. The stride between the corrupted chunks is 4096.

If I modify cryptsetup so that the buffer passed to crypt_storage_decrypt is 4096-byte aligned, the bug goes away and the volume can be activated.

For now, it looks like a kernel bug when accessing the userspace in the AF_ALG
subsystem.

Comment 27 Mikuláš Patočka 2014-07-25 22:02:24 UTC
The reason for the bug is that the code in arch/arm64/crypto/aes-glue.c assumes that the pointer is aligned to AES block size (16 bytes). If it isn't, crash or data corruption happens.

Comment 28 Mikuláš Patočka 2014-07-25 23:17:04 UTC
Created attachment 921078 [details]
A patch for the bug

Comment 29 Mikuláš Patočka 2014-07-25 23:18:17 UTC
Created attachment 921079 [details]
A patch for a similar bug in arm32 code

Comment 30 Richard W.M. Jones 2014-07-26 20:19:19 UTC
(In reply to Mikulas Patocka from comment #28)
> Created attachment 921078 [details]
> A patch for the bug

This patch (the arm64 one) fixes the original cryptsetup problem
I was having in the libguestfs test suite.

Comment 31 Milan Broz 2014-07-27 06:37:37 UTC
Thanks Mikulas!
(Mainly the arm32 part which is in stable kernel is scary....)

Whatever, do we have some built image with new kernel which can I run in Qemu?
(similar to that F190 buildroot image)

(I have also schroot for aarch64 but it is unusable for these kind of kernel related tests and qemu complains even for some loop ioctl so cryptsetup tests are not directly usable in this chroot.)

Comment 32 Richard W.M. Jones 2014-07-28 08:15:18 UTC
(In reply to Milan Broz from comment #31)
> Thanks Mikulas!
> (Mainly the arm32 part which is in stable kernel is scary....)
> 
> Whatever, do we have some built image with new kernel which can I run in
> Qemu?
> (similar to that F190 buildroot image)
> 
> (I have also schroot for aarch64 but it is unusable for these kind of kernel
> related tests and qemu complains even for some loop ioctl so cryptsetup
> tests are not directly usable in this chroot.)

Last time I tried it, qemu couldn't successfully emulate a whole
aarch64 system, just userspace processes.  (This is supposed to
be fixed/implemented at some point).

Comment 33 Milan Broz 2014-07-28 09:41:44 UTC
(In reply to Richard W.M. Jones from comment #32)
> Last time I tried it, qemu couldn't successfully emulate a whole
> aarch64 system, just userspace processes.  (This is supposed to
> be fixed/implemented at some point).

Yes, seems so. Cryptsetup tests definitely depend on some kernel ioctl emulations.

Just FYI I have hw ARM testing environment but it is stuck with 3.4 kernel for now (one of the first ARM Chromebooks) and all cryptsetup tests works there.

So seems we need to run test on ARM64 as well regularly, at least in emulated environment.

Comment 34 Josh Boyer 2014-07-28 20:15:10 UTC
These patches hit Linus' tree today.  They'll be in the -rc7.git1 build tomorrow.


Note You need to log in before you can comment on or make changes to this bug.