Broken userspace crypto in linux-4.1.18

Broken userspace crypto in linux-4.1.18

[Thomas D.]

Hi,

something is broken with crypto in linux-4.1.18.

On my system I have two disks (sda and sdb), both encrypted with LUKS
(cipher=aes-xts-plain64).

My rootfs resides encrypted on sda2 (sda1 is an unencrypted boot
partition).
sdb has one full encrypted partition (sdb1) mounted in "/backup".

After I upgraded from linux-4.1.17 to linux-4.1.18 and rebooted I noticed
that my encrypted rootfs was opened successfully (must be my initramfs)
however opening sdb1 with key file failed:

>  * Setting up dm-crypt mappings ...
>  *   backupVault using:   open /dev/sdb1 backupVault ...
> Failed to setup dm-crypt key mapping for device /dev/sdb1.
> Check that kernel supports aes-xts-plain64 cipher (check syslog for more info).
>  * failure running cryptsetup
>  [ !! ]
>  * Failed to setup dm-crypt devices
>  [ !! ]
>  * ERROR: dmcrypt failed to start

Calling cryptsetup from terminal with debug option showed

> Failed to setup dm-crypt key mapping for device /dev/sdb1.
> Check that kernel supports aes-xts-plain64 cipher (check syslog for more info).
> Command failed with code 22: Invalid argument
> # cryptsetup 1.7.0 processing "cryptsetup --verbose --debug --key-file /etc/backupVault.key luksOpen /dev/sdb1 backupVault"
> # Running command open.
> # Locking memory.
> # Installing SIGINT/SIGTERM handler.
> # Unblocking interruption on signal.
> # Allocating crypt device /dev/sdb1 context.
> # Trying to open and read device /dev/sdb1 with direct-io.
> # Initialising device-mapper backend library.
> # Trying to load LUKS1 crypt type from device /dev/sdb1.
> # Crypto backend (gcrypt 1.6.5) initialized in cryptsetup library version 1.7.0.
> # Detected kernel Linux 4.1.18 x86_64.
> # Reading LUKS header of size 1024 from device /dev/sdb1
> # Key length 64, device size 10483679 sectors, header size 4036 sectors.
> # Timeout set to 0 miliseconds.
> # Password retry count set to 3.
> # Password verification disabled.
> # Iteration time set to 2000 miliseconds.
> # Password retry count set to 1.
> # Activating volume backupVault [keyslot -1] using keyfile /etc/backupVault.key.
> # dm version   OF   [16384] (*1)
> # dm versions   OF   [16384] (*1)
> # Detected dm-crypt version 1.14.1, dm-ioctl version 4.31.0.
> # Device-mapper backend running with UDEV support disabled.
> # dm status backupVault  OF   [16384] (*1)
> # File descriptor passphrase entry requested.
> # Trying to open key slot 0 [ACTIVE].
> # Reading key slot 0 area.
> # Userspace crypto wrapper cannot use aes-xts-plain64 (-22).
> # Releasing crypt device /dev/sdb1 context.
> # Releasing device-mapper backend.
> # Unlocking memory.

dmesg/syslog never showed an error.


Calling `cryptsetup benchmark` shows (notice the "N/A"):

> # Tests are approximate using memory only (no storage IO).
> PBKDF2-sha1       647269 iterations per second for 256-bit key
> PBKDF2-sha256     832203 iterations per second for 256-bit key
> PBKDF2-sha512     576140 iterations per second for 256-bit key
> PBKDF2-ripemd160  379918 iterations per second for 256-bit key
> PBKDF2-whirlpool  264791 iterations per second for 256-bit key
> #  Algorithm | Key |  Encryption |  Decryption
>      aes-cbc   128b           N/A           N/A
>  serpent-cbc   128b           N/A           N/A
>  twofish-cbc   128b           N/A           N/A
>      aes-cbc   256b           N/A           N/A
>  serpent-cbc   256b           N/A           N/A
>  twofish-cbc   256b           N/A           N/A
>      aes-xts   256b           N/A           N/A
>  serpent-xts   256b           N/A           N/A
>  twofish-xts   256b           N/A           N/A
>      aes-xts   512b           N/A           N/A
>  serpent-xts   512b           N/A           N/A
>  twofish-xts   512b           N/A           N/A


Comparing /proc/crypto between 4.1.17 and 4.1.18:

> --- proc_crypto_4.1.17	2016-02-17 01:40:02.028647072 +0100
> +++ proc_crypto_4.1.18	2016-02-17 01:20:26.049998773 +0100
> @@ -1,158 +1,8 @@
> -name         : __xts-twofish-avx
> -driver       : cryptd(__driver-xts-twofish-avx)
> -module       : kernel
> -priority     : 50
> -refcnt       : 1
> -selftest     : passed
> -internal     : yes
> -type         : ablkcipher
> -async        : yes
> -blocksize    : 16
> -min keysize  : 32
> -max keysize  : 64
> -ivsize       : 16
> -geniv        : <default>
> -
> -name         : xts(twofish)
> -driver       : xts-twofish-avx
> -module       : kernel
> -priority     : 400
> -refcnt       : 1
> -selftest     : passed
> -internal     : no
> -type         : givcipher
> -async        : yes
> -blocksize    : 16
> -min keysize  : 32
> -max keysize  : 64
> -ivsize       : 16
> -geniv        : eseqiv
> -
> -name         : __xts-serpent-avx
> -driver       : cryptd(__driver-xts-serpent-avx)
> -module       : kernel
> -priority     : 50
> -refcnt       : 1
> -selftest     : passed
> -internal     : yes
> -type         : ablkcipher
> -async        : yes
> -blocksize    : 16
> -min keysize  : 0
> -max keysize  : 64
> -ivsize       : 16
> -geniv        : <default>
> -
> -name         : xts(serpent)
> -driver       : xts-serpent-avx
> -module       : kernel
> -priority     : 500
> -refcnt       : 1
> -selftest     : passed
> -internal     : no
> -type         : givcipher
> -async        : yes
> -blocksize    : 16
> -min keysize  : 0
> -max keysize  : 64
> -ivsize       : 16
> -geniv        : eseqiv
> -
> -name         : __cbc-twofish-avx
> -driver       : cryptd(__driver-cbc-twofish-avx)
> -module       : kernel
> -priority     : 50
> -refcnt       : 1
> -selftest     : passed
> -internal     : yes
> -type         : ablkcipher
> -async        : yes
> -blocksize    : 16
> -min keysize  : 16
> -max keysize  : 32
> -ivsize       : 0
> -geniv        : <default>
> -
> -name         : cbc(twofish)
> -driver       : cbc-twofish-avx
> -module       : kernel
> -priority     : 400
> -refcnt       : 1
> -selftest     : passed
> -internal     : no
> -type         : givcipher
> -async        : yes
> -blocksize    : 16
> -min keysize  : 16
> -max keysize  : 32
> -ivsize       : 16
> -geniv        : eseqiv
> -
> -name         : __cbc-serpent-avx
> -driver       : cryptd(__driver-cbc-serpent-avx)
> -module       : kernel
> -priority     : 50
> -refcnt       : 1
> -selftest     : passed
> -internal     : yes
> -type         : ablkcipher
> -async        : yes
> -blocksize    : 16
> -min keysize  : 0
> -max keysize  : 32
> -ivsize       : 0
> -geniv        : <default>
> -
> -name         : cbc(serpent)
> -driver       : cbc-serpent-avx
> -module       : kernel
> -priority     : 500
> -refcnt       : 1
> -selftest     : passed
> -internal     : no
> -type         : givcipher
> -async        : yes
> -blocksize    : 16
> -min keysize  : 0
> -max keysize  : 32
> -ivsize       : 16
> -geniv        : eseqiv
> -
> -name         : __cbc-aes-aesni
> -driver       : cryptd(__driver-cbc-aes-aesni)
> -module       : kernel
> -priority     : 50
> -refcnt       : 1
> -selftest     : passed
> -internal     : yes
> -type         : ablkcipher
> -async        : yes
> -blocksize    : 16
> -min keysize  : 16
> -max keysize  : 32
> -ivsize       : 0
> -geniv        : <default>
> -
> -name         : cbc(aes)
> -driver       : cbc-aes-aesni
> -module       : kernel
> -priority     : 400
> -refcnt       : 1
> -selftest     : passed
> -internal     : no
> -type         : givcipher
> -async        : yes
> -blocksize    : 16
> -min keysize  : 16
> -max keysize  : 32
> -ivsize       : 16
> -geniv        : eseqiv
> -
>  name         : __xts-aes-aesni
>  driver       : cryptd(__driver-xts-aes-aesni)
>  module       : kernel
>  priority     : 50
> -refcnt       : 3
> +refcnt       : 2
>  selftest     : passed
>  internal     : yes
>  type         : ablkcipher
> @@ -167,7 +17,7 @@
>  driver       : xts-aes-aesni
>  module       : kernel
>  priority     : 400
> -refcnt       : 3
> +refcnt       : 2
>  selftest     : passed
>  internal     : no
>  type         : givcipher
> @@ -1524,7 +1374,7 @@
>  driver       : xts-aes-aesni
>  module       : kernel
>  priority     : 400
> -refcnt       : 3
> +refcnt       : 2
>  selftest     : passed
>  internal     : no
>  type         : ablkcipher
> @@ -1554,7 +1404,7 @@
>  driver       : __driver-xts-aes-aesni
>  module       : kernel
>  priority     : 0
> -refcnt       : 3
> +refcnt       : 2
>  selftest     : passed
>  internal     : yes
>  type         : blkcipher

However I am using the same kernel configuration :)


After I bisect the kernel I found the following bad commit:

> commit 0571ba52a19e18a1c20469454231eef681cb1310
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> Date:   Wed Dec 30 11:47:53 2015 +0800
> 
>     crypto: af_alg - Disallow bind/setkey/... after accept(2)
> 
>     [ Upstream commit c840ac6af3f8713a71b4d2363419145760bd6044 ]
> 
>     Each af_alg parent socket obtained by socket(2) corresponds to a
>     tfm object once bind(2) has succeeded.  An accept(2) call on that
>     parent socket creates a context which then uses the tfm object.
> 
>     Therefore as long as any child sockets created by accept(2) exist
>     the parent socket must not be modified or freed.
> 
>     This patch guarantees this by using locks and a reference count
>     on the parent socket.  Any attempt to modify the parent socket will
>     fail with EBUSY.


bisect log:

> Bisecting: 114 revisions left to test after this (roughly 7 steps)
> [3a1e81ad84e4d880b00ecf7ad8d03b9b772ddfa7] crypto: algif_hash - Fix race condition in hash_check_key
> Bisecting: 56 revisions left to test after this (roughly 6 steps)
> [d6341753c418d3699948290d8c0b9d9dc78bd209] udf: Prevent buffer overrun with multi-byte characters
> Bisecting: 28 revisions left to test after this (roughly 5 steps)
> [13aedd784b84cb7d8a3bb835941d80e99f5c796e] dmaengine: dw: fix cyclic transfer setup
> Bisecting: 14 revisions left to test after this (roughly 4 steps)
> [664ecf4f243bac17065cd9878790d40a592e2f3d] zram/zcomp: use GFP_NOIO to allocate streams
> Bisecting: 7 revisions left to test after this (roughly 3 steps)
> [0571ba52a19e18a1c20469454231eef681cb1310] crypto: af_alg - Disallow bind/setkey/... after accept(2)
> Bisecting: 3 revisions left to test after this (roughly 2 steps)
> [2c641f5b0c8e87d43235ce39890bcc4d0c7cd2fb] memcg: only free spare array when readers are done
> Bisecting: 1 revision left to test after this (roughly 1 step)
> [0e19e24c3fe0abde8e2c5f4543616a251ccea6bf] kernel/panic.c: turn off locks debug before releasing console lock
> Bisecting: 0 revisions left to test after this (roughly 0 steps)
> [bc24ac15b0746172a8f603171352aa54abcf7c78] printk: do cond_resched() between lines while outputting to consoles
> 0571ba52a19e18a1c20469454231eef681cb1310 is the first bad commit


-Thomas

Re: Broken userspace crypto in linux-4.1.18

[Sasha Levin]

On 02/17/2016 09:04 AM, Thomas D. wrote:
> Hi,
> 
> something is broken with crypto in linux-4.1.18.
> 
> On my system I have two disks (sda and sdb), both encrypted with LUKS
> (cipher=aes-xts-plain64).
> 
> My rootfs resides encrypted on sda2 (sda1 is an unencrypted boot
> partition).
> sdb has one full encrypted partition (sdb1) mounted in "/backup".
> 
> After I upgraded from linux-4.1.17 to linux-4.1.18 and rebooted I noticed
> that my encrypted rootfs was opened successfully (must be my initramfs)
> however opening sdb1 with key file failed:

Thanks for the report Thomas.

[...]

> After I bisect the kernel I found the following bad commit:
> 
>> commit 0571ba52a19e18a1c20469454231eef681cb1310
>> Author: Herbert Xu <herbert@gondor.apana.org.au>
>> Date:   Wed Dec 30 11:47:53 2015 +0800
>>
>>     crypto: af_alg - Disallow bind/setkey/... after accept(2)
>>
>>     [ Upstream commit c840ac6af3f8713a71b4d2363419145760bd6044 ]
>>
>>     Each af_alg parent socket obtained by socket(2) corresponds to a
>>     tfm object once bind(2) has succeeded.  An accept(2) call on that
>>     parent socket creates a context which then uses the tfm object.
>>
>>     Therefore as long as any child sockets created by accept(2) exist
>>     the parent socket must not be modified or freed.
>>
>>     This patch guarantees this by using locks and a reference count
>>     on the parent socket.  Any attempt to modify the parent socket will
>>     fail with EBUSY.

So either the upstream patch is broken, or the 4.1 backport is wrong/missing
dependency/missing fix.

Any chance you could try 4.5-rc3 and see if that works for you? That'll narrow
it down a lot.


Thanks,
Sasha

Re: Broken userspace crypto in linux-4.1.18

[Thomas D.]

Hi,

Sasha Levin wrote:
> So either the upstream patch is broken, or the 4.1 backport is wrong/missing
> dependency/missing fix.
> 
> Any chance you could try 4.5-rc3 and see if that works for you? That'll narrow
> it down a lot.

4.5-rc3 works for me.

Linux-4.4.0 works, too.


-Thomas

Re: Broken userspace crypto in linux-4.1.18

[Sasha Levin]

On 02/17/2016 10:24 AM, Thomas D. wrote:
> Hi,
> 
> Sasha Levin wrote:
>> > So either the upstream patch is broken, or the 4.1 backport is wrong/missing
>> > dependency/missing fix.
>> > 
>> > Any chance you could try 4.5-rc3 and see if that works for you? That'll narrow
>> > it down a lot.
> 4.5-rc3 works for me.
> 
> Linux-4.4.0 works, too.

Herbert,

Is there a dependency I missed in 4.1? I don't really see anything that
could have gone wrong there.


Thanks,
Sasha

Re: Broken userspace crypto in linux-4.1.18

[Willy Tarreau]

On Wed, Feb 17, 2016 at 05:12:56PM -0500, Sasha Levin wrote:
> On 02/17/2016 10:24 AM, Thomas D. wrote:
> > Hi,
> > 
> > Sasha Levin wrote:
> >> > So either the upstream patch is broken, or the 4.1 backport is wrong/missing
> >> > dependency/missing fix.
> >> > 
> >> > Any chance you could try 4.5-rc3 and see if that works for you? That'll narrow
> >> > it down a lot.
> > 4.5-rc3 works for me.
> > 
> > Linux-4.4.0 works, too.
> 
> Herbert,
> 
> Is there a dependency I missed in 4.1? I don't really see anything that
> could have gone wrong there.

Or maybe Thomas can run a bisect ? Thomas, do you feel comfortable with
doing this ? There are a few patches between 4.1.17 and 4.1.18, in 8
steps it should be done, and only 4 if you limit yourself to the crypto
directory. Stable versions are fast at bisecting because each patch
touches a very isolated area so only the first build takes a bit of
time but the other ones are often pretty fast. Finding the faulty
patch can help spotting if something is wrong in its backport.

Regards,
Willy

Re: Broken userspace crypto in linux-4.1.18

[Thomas D.]

Hi

Willy Tarreau wrote:
>> Is there a dependency I missed in 4.1? I don't really see anything that
>> could have gone wrong there.
> 
> Or maybe Thomas can run a bisect ?

I cannot follow. I did a bisect between 4.1.7 and 4.1.8 as I have written
in my first mail. The bad commit was:

> commit 0571ba52a19e18a1c20469454231eef681cb1310
> Author: Herbert Xu
> Date:   Wed Dec 30 11:47:53 2015 +0800
> 
>     crypto: af_alg - Disallow bind/setkey/... after accept(2)
> 
>     [ Upstream commit c840ac6af3f8713a71b4d2363419145760bd6044 ]
> 
>     Each af_alg parent socket obtained by socket(2) corresponds to a
>     tfm object once bind(2) has succeeded.  An accept(2) call on that
>     parent socket creates a context which then uses the tfm object.
> 
>     Therefore as long as any child sockets created by accept(2) exist
>     the parent socket must not be modified or freed.
> 
>     This patch guarantees this by using locks and a reference count
>     on the parent socket.  Any attempt to modify the parent socket will
>     fail with EBUSY.


bisect log:

> Bisecting: 114 revisions left to test after this (roughly 7 steps)
> [3a1e81ad84e4d880b00ecf7ad8d03b9b772ddfa7] crypto: algif_hash - Fix race condition in hash_check_key
> Bisecting: 56 revisions left to test after this (roughly 6 steps)
> [d6341753c418d3699948290d8c0b9d9dc78bd209] udf: Prevent buffer overrun with multi-byte characters
> Bisecting: 28 revisions left to test after this (roughly 5 steps)
> [13aedd784b84cb7d8a3bb835941d80e99f5c796e] dmaengine: dw: fix cyclic transfer setup
> Bisecting: 14 revisions left to test after this (roughly 4 steps)
> [664ecf4f243bac17065cd9878790d40a592e2f3d] zram/zcomp: use GFP_NOIO to allocate streams
> Bisecting: 7 revisions left to test after this (roughly 3 steps)
> [0571ba52a19e18a1c20469454231eef681cb1310] crypto: af_alg - Disallow bind/setkey/... after accept(2)
> Bisecting: 3 revisions left to test after this (roughly 2 steps)
> [2c641f5b0c8e87d43235ce39890bcc4d0c7cd2fb] memcg: only free spare array when readers are done
> Bisecting: 1 revision left to test after this (roughly 1 step)
> [0e19e24c3fe0abde8e2c5f4543616a251ccea6bf] kernel/panic.c: turn off locks debug before releasing console lock
> Bisecting: 0 revisions left to test after this (roughly 0 steps)
> [bc24ac15b0746172a8f603171352aa54abcf7c78] printk: do cond_resched() between lines while outputting to consoles
> 0571ba52a19e18a1c20469454231eef681cb1310 is the first bad commit


-Thomas

Re: Broken userspace crypto in linux-4.1.18

[Willy Tarreau]

On Thu, Feb 18, 2016 at 12:49:57AM +0100, Thomas D. wrote:
> Hi
> 
> Willy Tarreau wrote:
> >> Is there a dependency I missed in 4.1? I don't really see anything that
> >> could have gone wrong there.
> > 
> > Or maybe Thomas can run a bisect ?
> 
> I cannot follow. I did a bisect between 4.1.7 and 4.1.8 as I have written
> in my first mail. The bad commit was:
> 
> > commit 0571ba52a19e18a1c20469454231eef681cb1310

Ah sorry, I missed this one, I was confused by the test results on the
more recent kernels. So indeed, only this patch's backport has to be
studied.

Thanks and sorry for the noise.

Willy

Re: Broken userspace crypto in linux-4.1.18

[Stephan Mueller]

Am Donnerstag, 18. Februar 2016, 00:49:57 schrieb Thomas D.:

Hi Thomas,

> Hi
> 
> Willy Tarreau wrote:
> >> Is there a dependency I missed in 4.1? I don't really see anything that
> >> could have gone wrong there.
> > 
> > Or maybe Thomas can run a bisect ?
> 
> I cannot follow. I did a bisect between 4.1.7 and 4.1.8 as I have written
> 
> in my first mail. The bad commit was:

That breakage was expected and forcasted by Milan Broz a couple of days ago on 
this mailing list. The referenced patch covered a bug that must have been 
fixed but introduced a regression for backwards compatibility. Since then, 
this regression was fixed.



> > commit 0571ba52a19e18a1c20469454231eef681cb1310
> > Author: Herbert Xu
> > Date:   Wed Dec 30 11:47:53 2015 +0800
> > 
> >     crypto: af_alg - Disallow bind/setkey/... after accept(2)
> >     
> >     [ Upstream commit c840ac6af3f8713a71b4d2363419145760bd6044 ]
> >     
> >     Each af_alg parent socket obtained by socket(2) corresponds to a
> >     tfm object once bind(2) has succeeded.  An accept(2) call on that
> >     parent socket creates a context which then uses the tfm object.
> >     
> >     Therefore as long as any child sockets created by accept(2) exist
> >     the parent socket must not be modified or freed.
> >     
> >     This patch guarantees this by using locks and a reference count
> >     on the parent socket.  Any attempt to modify the parent socket will
> >     fail with EBUSY.
> 
> bisect log:
> > Bisecting: 114 revisions left to test after this (roughly 7 steps)
> > [3a1e81ad84e4d880b00ecf7ad8d03b9b772ddfa7] crypto: algif_hash - Fix race
> > condition in hash_check_key Bisecting: 56 revisions left to test after
> > this (roughly 6 steps)
> > [d6341753c418d3699948290d8c0b9d9dc78bd209] udf: Prevent buffer overrun
> > with multi-byte characters Bisecting: 28 revisions left to test after
> > this (roughly 5 steps)
> > [13aedd784b84cb7d8a3bb835941d80e99f5c796e] dmaengine: dw: fix cyclic
> > transfer setup Bisecting: 14 revisions left to test after this (roughly 4
> > steps)
> > [664ecf4f243bac17065cd9878790d40a592e2f3d] zram/zcomp: use GFP_NOIO to
> > allocate streams Bisecting: 7 revisions left to test after this (roughly
> > 3 steps)
> > [0571ba52a19e18a1c20469454231eef681cb1310] crypto: af_alg - Disallow
> > bind/setkey/... after accept(2) Bisecting: 3 revisions left to test after
> > this (roughly 2 steps)
> > [2c641f5b0c8e87d43235ce39890bcc4d0c7cd2fb] memcg: only free spare array
> > when readers are done Bisecting: 1 revision left to test after this
> > (roughly 1 step)
> > [0e19e24c3fe0abde8e2c5f4543616a251ccea6bf] kernel/panic.c: turn off locks
> > debug before releasing console lock Bisecting: 0 revisions left to test
> > after this (roughly 0 steps)
> > [bc24ac15b0746172a8f603171352aa54abcf7c78] printk: do cond_resched()
> > between lines while outputting to consoles
> > 0571ba52a19e18a1c20469454231eef681cb1310 is the first bad commit
> 
> -Thomas
> 
> --
> 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

Re: Broken userspace crypto in linux-4.1.18

[Jiri Slaby]

Hi,

On 02/18/2016, 09:17 AM, Stephan Mueller wrote:
> Am Donnerstag, 18. Februar 2016, 00:49:57 schrieb Thomas D.:
>> Willy Tarreau wrote:
>>>> Is there a dependency I missed in 4.1? I don't really see anything that
>>>> could have gone wrong there.
>>>
>>> Or maybe Thomas can run a bisect ?
>>
>> I cannot follow. I did a bisect between 4.1.7 and 4.1.8 as I have written
>>
>> in my first mail. The bad commit was:
> 
> That breakage was expected and forcasted by Milan Broz a couple of days ago on 
> this mailing list.

Could you be more specific? What is "this mailing list"? stable or
crypto? I cannot see anything from Milan on this mailing list aka stable.

> The referenced patch covered a bug that must have been 
> fixed but introduced a regression for backwards compatibility. Since then, 
> this regression was fixed.

By what? Please use SHAs, links, whatever.

thanks,
-- 
js
suse labs

Re: Broken userspace crypto in linux-4.1.18

[Thomas D.]

Hi,

Jiri Slaby wrote:
>> That breakage was expected and forcasted by Milan Broz a couple of days ago on 
>> this mailing list.
>
> Could you be more specific? What is "this mailing list"? stable or
> crypto? I cannot see anything from Milan on this mailing list aka stable.

Cannot speak for Stephan but I guess he means the "[PATCH v2] crypto:
algif_skcipher - Require setkey before accept(2)" thread on the crypto ml:

http://www.spinics.net/lists/linux-crypto/msg17931.html

http://www.spinics.net/lists/linux-crypto/msg17936.html


PS: In 4.4.2 it looks like the same commit breaking 4.1.8 was added
however 4.4.2 works for me.


-Thomas

Re: Broken userspace crypto in linux-4.1.18

[Thomas D.]

Hi,

FYI: v3.10.97, v3.14.61 and 3.18.27 are also affected.

v4.3.6 works. Looks like the patch set is only compatible with >=linux-4.3.

v3.12.54 works because it doesn't contain the patch in question.


-Thomas

[PATCH] Re: Broken userspace crypto in linux-4.1.18

[Milan Broz]

On 02/20/2016 03:33 PM, Thomas D. wrote:
> Hi,
> 
> FYI: v3.10.97, v3.14.61 and 3.18.27 are also affected.
> 
> v4.3.6 works. Looks like the patch set is only compatible with >=linux-4.3.
> 
> v3.12.54 works because it doesn't contain the patch in question.

Hi,

indeed, because whoever backported this patchset skipped two patches
from series (because of skcipher interface file was introduced later).

I tried to backport it (on 4.1.18 tree, see patch below) and it fixes the problem
for me.

Anyway, it is just quick test what is the problem, patch need proper review or
backport from someone who knows the code better.

I will release stable cryptsetup soon that will work with new kernel even without
this patch but I would strongly recommend that stable kernels get these compatibility
backports as well to avoid breaking existing userspace.

Thanks,
Milan
-

This patch backports missing parts of crypto API compatibility changes:

  dd504589577d8e8e70f51f997ad487a4cb6c026f
  crypto: algif_skcipher - Require setkey before accept(2)

  a0fa2d037129a9849918a92d91b79ed6c7bd2818
  crypto: algif_skcipher - Add nokey compatibility path

--- crypto/algif_skcipher.c.orig	2016-02-21 16:44:27.172312990 +0100
+++ crypto/algif_skcipher.c	2016-02-21 17:03:58.555785347 +0100
@@ -31,6 +31,11 @@
 	struct scatterlist sg[0];
 };
 
+struct skcipher_tfm {
+	struct crypto_ablkcipher *skcipher;
+	bool has_key;
+};
+
 struct skcipher_ctx {
 	struct list_head tsgl;
 	struct af_alg_sgl rsgl;
@@ -750,19 +755,136 @@
 	.poll		=	skcipher_poll,
 };
 
+static int skcipher_check_key(struct socket *sock)
+{
+	int err;
+	struct sock *psk;
+	struct alg_sock *pask;
+	struct skcipher_tfm *tfm;
+	struct sock *sk = sock->sk;
+	struct alg_sock *ask = alg_sk(sk);
+
+	if (ask->refcnt)
+		return 0;
+
+	psk = ask->parent;
+	pask = alg_sk(ask->parent);
+	tfm = pask->private;
+
+	err = -ENOKEY;
+	lock_sock(psk);
+	if (!tfm->has_key)
+		goto unlock;
+
+	if (!pask->refcnt++)
+		sock_hold(psk);
+
+	ask->refcnt = 1;
+	sock_put(psk);
+
+	err = 0;
+
+unlock:
+	release_sock(psk);
+
+	return err;
+}
+
+static int skcipher_sendmsg_nokey(struct socket *sock, struct msghdr *msg,
+				  size_t size)
+{
+	int err;
+
+	err = skcipher_check_key(sock);
+	if (err)
+		return err;
+
+	return skcipher_sendmsg(sock, msg, size);
+}
+
+static ssize_t skcipher_sendpage_nokey(struct socket *sock, struct page *page,
+				       int offset, size_t size, int flags)
+{
+	int err;
+
+	err = skcipher_check_key(sock);
+	if (err)
+		return err;
+
+	return skcipher_sendpage(sock, page, offset, size, flags);
+}
+
+static int skcipher_recvmsg_nokey(struct socket *sock, struct msghdr *msg,
+				  size_t ignored, int flags)
+{
+	int err;
+
+	err = skcipher_check_key(sock);
+	if (err)
+		return err;
+
+	return skcipher_recvmsg(sock, msg, ignored, flags);
+}
+
+static struct proto_ops algif_skcipher_ops_nokey = {
+	.family		=	PF_ALG,
+
+	.connect	=	sock_no_connect,
+	.socketpair	=	sock_no_socketpair,
+	.getname	=	sock_no_getname,
+	.ioctl		=	sock_no_ioctl,
+	.listen		=	sock_no_listen,
+	.shutdown	=	sock_no_shutdown,
+	.getsockopt	=	sock_no_getsockopt,
+	.mmap		=	sock_no_mmap,
+	.bind		=	sock_no_bind,
+	.accept		=	sock_no_accept,
+	.setsockopt	=	sock_no_setsockopt,
+
+	.release	=	af_alg_release,
+	.sendmsg	=	skcipher_sendmsg_nokey,
+	.sendpage	=	skcipher_sendpage_nokey,
+	.recvmsg	=	skcipher_recvmsg_nokey,
+	.poll		=	skcipher_poll,
+};
+
 static void *skcipher_bind(const char *name, u32 type, u32 mask)
 {
-	return crypto_alloc_ablkcipher(name, type, mask);
+	struct skcipher_tfm *tfm;
+	struct crypto_ablkcipher *skcipher;
+
+	tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
+	if (!tfm)
+		return ERR_PTR(-ENOMEM);
+
+	skcipher = crypto_alloc_ablkcipher(name, type, mask);
+	if (IS_ERR(skcipher)) {
+		kfree(tfm);
+		return ERR_CAST(skcipher);
+	}
+
+	tfm->skcipher = skcipher;
+
+	return tfm;
 }
 
 static void skcipher_release(void *private)
 {
-	crypto_free_ablkcipher(private);
+	struct skcipher_tfm *tfm = private;
+
+	crypto_free_ablkcipher(tfm->skcipher);
+	kfree(tfm);
 }
 
 static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
 {
-	return crypto_ablkcipher_setkey(private, key, keylen);
+	struct skcipher_tfm *tfm = private;
+	int err;
+
+	err = crypto_ablkcipher_setkey(tfm->skcipher, key, keylen);
+	tfm->has_key = !err;
+
+	return err;
 }
 
 static void skcipher_wait(struct sock *sk)
@@ -775,7 +897,7 @@
 		msleep(100);
 }
 
-static void skcipher_sock_destruct(struct sock *sk)
+static void skcipher_sock_destruct_common(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
 	struct skcipher_ctx *ctx = ask->private;
@@ -790,24 +912,50 @@
 	af_alg_release_parent(sk);
 }
 
-static int skcipher_accept_parent(void *private, struct sock *sk)
+static void skcipher_sock_destruct(struct sock *sk)
+{
+	skcipher_sock_destruct_common(sk);
+	af_alg_release_parent(sk);
+}
+
+static void skcipher_release_parent_nokey(struct sock *sk)
+{
+	struct alg_sock *ask = alg_sk(sk);
+
+	if (!ask->refcnt) {
+		sock_put(ask->parent);
+		return;
+	}
+
+	af_alg_release_parent(sk);
+}
+
+static void skcipher_sock_destruct_nokey(struct sock *sk)
+{
+	skcipher_sock_destruct_common(sk);
+	skcipher_release_parent_nokey(sk);
+}
+
+static int skcipher_accept_parent_common(void *private, struct sock *sk)
 {
 	struct skcipher_ctx *ctx;
 	struct alg_sock *ask = alg_sk(sk);
-	unsigned int len = sizeof(*ctx) + crypto_ablkcipher_reqsize(private);
+	struct skcipher_tfm *tfm = private;
+	struct crypto_ablkcipher *skcipher = tfm->skcipher;
+	unsigned int len = sizeof(*ctx) + crypto_ablkcipher_reqsize(skcipher);
 
 	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 
-	ctx->iv = sock_kmalloc(sk, crypto_ablkcipher_ivsize(private),
+	ctx->iv = sock_kmalloc(sk, crypto_ablkcipher_ivsize(skcipher),
 			       GFP_KERNEL);
 	if (!ctx->iv) {
 		sock_kfree_s(sk, ctx, len);
 		return -ENOMEM;
 	}
 
-	memset(ctx->iv, 0, crypto_ablkcipher_ivsize(private));
+	memset(ctx->iv, 0, crypto_ablkcipher_ivsize(skcipher));
 
 	INIT_LIST_HEAD(&ctx->tsgl);
 	ctx->len = len;
@@ -820,7 +968,7 @@
 
 	ask->private = ctx;
 
-	ablkcipher_request_set_tfm(&ctx->req, private);
+	ablkcipher_request_set_tfm(&ctx->req, skcipher);
 	ablkcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
 					af_alg_complete, &ctx->completion);
 
@@ -829,12 +977,38 @@
 	return 0;
 }
 
+static int skcipher_accept_parent(void *private, struct sock *sk)
+{
+	struct skcipher_tfm *tfm = private;
+
+	if (!tfm->has_key)
+		return -ENOKEY;
+
+	return skcipher_accept_parent_common(private, sk);
+}
+
+static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
+{
+	int err;
+
+	err = skcipher_accept_parent_common(private, sk);
+	if (err)
+		goto out;
+
+	sk->sk_destruct = skcipher_sock_destruct_nokey;
+
+out:
+	return err;
+}
+
 static const struct af_alg_type algif_type_skcipher = {
 	.bind		=	skcipher_bind,
 	.release	=	skcipher_release,
 	.setkey		=	skcipher_setkey,
 	.accept		=	skcipher_accept_parent,
+	.accept_nokey	=	skcipher_accept_parent_nokey,
 	.ops		=	&algif_skcipher_ops,
+	.ops_nokey	=	&algif_skcipher_ops_nokey,
 	.name		=	"skcipher",
 	.owner		=	THIS_MODULE
 };

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Milan Broz]

On 02/21/2016 05:40 PM, Milan Broz wrote:
> On 02/20/2016 03:33 PM, Thomas D. wrote:
>> Hi,
>>
>> FYI: v3.10.97, v3.14.61 and 3.18.27 are also affected.
>>
>> v4.3.6 works. Looks like the patch set is only compatible with >=linux-4.3.
>>
>> v3.12.54 works because it doesn't contain the patch in question.
> 
> Hi,
> 
> indeed, because whoever backported this patchset skipped two patches
> from series (because of skcipher interface file was introduced later).

Ping?

I always thought that breaking userspace is not the way mainline kernel
operates and here we break even stable tree...

Anyone planning to release new kernel version with properly backported patches?
There is already a lot of downstream distro bugs reported.

Thanks,
Milan

> 
> I tried to backport it (on 4.1.18 tree, see patch below) and it fixes the problem
> for me.
> 
> Anyway, it is just quick test what is the problem, patch need proper review or
> backport from someone who knows the code better.
> 
> I will release stable cryptsetup soon that will work with new kernel even without
> this patch but I would strongly recommend that stable kernels get these compatibility
> backports as well to avoid breaking existing userspace.
> 
> Thanks,
> Milan
> -
> 
> This patch backports missing parts of crypto API compatibility changes:
> 
>   dd504589577d8e8e70f51f997ad487a4cb6c026f
>   crypto: algif_skcipher - Require setkey before accept(2)
> 
>   a0fa2d037129a9849918a92d91b79ed6c7bd2818
>   crypto: algif_skcipher - Add nokey compatibility path
> 
> --- crypto/algif_skcipher.c.orig	2016-02-21 16:44:27.172312990 +0100
> +++ crypto/algif_skcipher.c	2016-02-21 17:03:58.555785347 +0100
> @@ -31,6 +31,11 @@
>  	struct scatterlist sg[0];
>  };
>  
> +struct skcipher_tfm {
> +	struct crypto_ablkcipher *skcipher;
> +	bool has_key;
> +};
> +
>  struct skcipher_ctx {
>  	struct list_head tsgl;
>  	struct af_alg_sgl rsgl;
> @@ -750,19 +755,136 @@
>  	.poll		=	skcipher_poll,
>  };
>  
> +static int skcipher_check_key(struct socket *sock)
> +{
> +	int err;
> +	struct sock *psk;
> +	struct alg_sock *pask;
> +	struct skcipher_tfm *tfm;
> +	struct sock *sk = sock->sk;
> +	struct alg_sock *ask = alg_sk(sk);
> +
> +	if (ask->refcnt)
> +		return 0;
> +
> +	psk = ask->parent;
> +	pask = alg_sk(ask->parent);
> +	tfm = pask->private;
> +
> +	err = -ENOKEY;
> +	lock_sock(psk);
> +	if (!tfm->has_key)
> +		goto unlock;
> +
> +	if (!pask->refcnt++)
> +		sock_hold(psk);
> +
> +	ask->refcnt = 1;
> +	sock_put(psk);
> +
> +	err = 0;
> +
> +unlock:
> +	release_sock(psk);
> +
> +	return err;
> +}
> +
> +static int skcipher_sendmsg_nokey(struct socket *sock, struct msghdr *msg,
> +				  size_t size)
> +{
> +	int err;
> +
> +	err = skcipher_check_key(sock);
> +	if (err)
> +		return err;
> +
> +	return skcipher_sendmsg(sock, msg, size);
> +}
> +
> +static ssize_t skcipher_sendpage_nokey(struct socket *sock, struct page *page,
> +				       int offset, size_t size, int flags)
> +{
> +	int err;
> +
> +	err = skcipher_check_key(sock);
> +	if (err)
> +		return err;
> +
> +	return skcipher_sendpage(sock, page, offset, size, flags);
> +}
> +
> +static int skcipher_recvmsg_nokey(struct socket *sock, struct msghdr *msg,
> +				  size_t ignored, int flags)
> +{
> +	int err;
> +
> +	err = skcipher_check_key(sock);
> +	if (err)
> +		return err;
> +
> +	return skcipher_recvmsg(sock, msg, ignored, flags);
> +}
> +
> +static struct proto_ops algif_skcipher_ops_nokey = {
> +	.family		=	PF_ALG,
> +
> +	.connect	=	sock_no_connect,
> +	.socketpair	=	sock_no_socketpair,
> +	.getname	=	sock_no_getname,
> +	.ioctl		=	sock_no_ioctl,
> +	.listen		=	sock_no_listen,
> +	.shutdown	=	sock_no_shutdown,
> +	.getsockopt	=	sock_no_getsockopt,
> +	.mmap		=	sock_no_mmap,
> +	.bind		=	sock_no_bind,
> +	.accept		=	sock_no_accept,
> +	.setsockopt	=	sock_no_setsockopt,
> +
> +	.release	=	af_alg_release,
> +	.sendmsg	=	skcipher_sendmsg_nokey,
> +	.sendpage	=	skcipher_sendpage_nokey,
> +	.recvmsg	=	skcipher_recvmsg_nokey,
> +	.poll		=	skcipher_poll,
> +};
> +
>  static void *skcipher_bind(const char *name, u32 type, u32 mask)
>  {
> -	return crypto_alloc_ablkcipher(name, type, mask);
> +	struct skcipher_tfm *tfm;
> +	struct crypto_ablkcipher *skcipher;
> +
> +	tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
> +	if (!tfm)
> +		return ERR_PTR(-ENOMEM);
> +
> +	skcipher = crypto_alloc_ablkcipher(name, type, mask);
> +	if (IS_ERR(skcipher)) {
> +		kfree(tfm);
> +		return ERR_CAST(skcipher);
> +	}
> +
> +	tfm->skcipher = skcipher;
> +
> +	return tfm;
>  }
>  
>  static void skcipher_release(void *private)
>  {
> -	crypto_free_ablkcipher(private);
> +	struct skcipher_tfm *tfm = private;
> +
> +	crypto_free_ablkcipher(tfm->skcipher);
> +	kfree(tfm);
>  }
>  
>  static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
>  {
> -	return crypto_ablkcipher_setkey(private, key, keylen);
> +	struct skcipher_tfm *tfm = private;
> +	int err;
> +
> +	err = crypto_ablkcipher_setkey(tfm->skcipher, key, keylen);
> +	tfm->has_key = !err;
> +
> +	return err;
>  }
>  
>  static void skcipher_wait(struct sock *sk)
> @@ -775,7 +897,7 @@
>  		msleep(100);
>  }
>  
> -static void skcipher_sock_destruct(struct sock *sk)
> +static void skcipher_sock_destruct_common(struct sock *sk)
>  {
>  	struct alg_sock *ask = alg_sk(sk);
>  	struct skcipher_ctx *ctx = ask->private;
> @@ -790,24 +912,50 @@
>  	af_alg_release_parent(sk);
>  }
>  
> -static int skcipher_accept_parent(void *private, struct sock *sk)
> +static void skcipher_sock_destruct(struct sock *sk)
> +{
> +	skcipher_sock_destruct_common(sk);
> +	af_alg_release_parent(sk);
> +}
> +
> +static void skcipher_release_parent_nokey(struct sock *sk)
> +{
> +	struct alg_sock *ask = alg_sk(sk);
> +
> +	if (!ask->refcnt) {
> +		sock_put(ask->parent);
> +		return;
> +	}
> +
> +	af_alg_release_parent(sk);
> +}
> +
> +static void skcipher_sock_destruct_nokey(struct sock *sk)
> +{
> +	skcipher_sock_destruct_common(sk);
> +	skcipher_release_parent_nokey(sk);
> +}
> +
> +static int skcipher_accept_parent_common(void *private, struct sock *sk)
>  {
>  	struct skcipher_ctx *ctx;
>  	struct alg_sock *ask = alg_sk(sk);
> -	unsigned int len = sizeof(*ctx) + crypto_ablkcipher_reqsize(private);
> +	struct skcipher_tfm *tfm = private;
> +	struct crypto_ablkcipher *skcipher = tfm->skcipher;
> +	unsigned int len = sizeof(*ctx) + crypto_ablkcipher_reqsize(skcipher);
>  
>  	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
>  	if (!ctx)
>  		return -ENOMEM;
>  
> -	ctx->iv = sock_kmalloc(sk, crypto_ablkcipher_ivsize(private),
> +	ctx->iv = sock_kmalloc(sk, crypto_ablkcipher_ivsize(skcipher),
>  			       GFP_KERNEL);
>  	if (!ctx->iv) {
>  		sock_kfree_s(sk, ctx, len);
>  		return -ENOMEM;
>  	}
>  
> -	memset(ctx->iv, 0, crypto_ablkcipher_ivsize(private));
> +	memset(ctx->iv, 0, crypto_ablkcipher_ivsize(skcipher));
>  
>  	INIT_LIST_HEAD(&ctx->tsgl);
>  	ctx->len = len;
> @@ -820,7 +968,7 @@
>  
>  	ask->private = ctx;
>  
> -	ablkcipher_request_set_tfm(&ctx->req, private);
> +	ablkcipher_request_set_tfm(&ctx->req, skcipher);
>  	ablkcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
>  					af_alg_complete, &ctx->completion);
>  
> @@ -829,12 +977,38 @@
>  	return 0;
>  }
>  
> +static int skcipher_accept_parent(void *private, struct sock *sk)
> +{
> +	struct skcipher_tfm *tfm = private;
> +
> +	if (!tfm->has_key)
> +		return -ENOKEY;
> +
> +	return skcipher_accept_parent_common(private, sk);
> +}
> +
> +static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
> +{
> +	int err;
> +
> +	err = skcipher_accept_parent_common(private, sk);
> +	if (err)
> +		goto out;
> +
> +	sk->sk_destruct = skcipher_sock_destruct_nokey;
> +
> +out:
> +	return err;
> +}
> +
>  static const struct af_alg_type algif_type_skcipher = {
>  	.bind		=	skcipher_bind,
>  	.release	=	skcipher_release,
>  	.setkey		=	skcipher_setkey,
>  	.accept		=	skcipher_accept_parent,
> +	.accept_nokey	=	skcipher_accept_parent_nokey,
>  	.ops		=	&algif_skcipher_ops,
> +	.ops_nokey	=	&algif_skcipher_ops_nokey,
>  	.name		=	"skcipher",
>  	.owner		=	THIS_MODULE
>  };
> 
> 

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Sasha Levin]

On 02/23/2016 04:02 PM, Milan Broz wrote:
> On 02/21/2016 05:40 PM, Milan Broz wrote:
>> > On 02/20/2016 03:33 PM, Thomas D. wrote:
>>> >> Hi,
>>> >>
>>> >> FYI: v3.10.97, v3.14.61 and 3.18.27 are also affected.
>>> >>
>>> >> v4.3.6 works. Looks like the patch set is only compatible with >=linux-4.3.
>>> >>
>>> >> v3.12.54 works because it doesn't contain the patch in question.
>> > 
>> > Hi,
>> > 
>> > indeed, because whoever backported this patchset skipped two patches
>> > from series (because of skcipher interface file was introduced later).
> Ping?
> 
> I always thought that breaking userspace is not the way mainline kernel
> operates and here we break even stable tree...
> 
> Anyone planning to release new kernel version with properly backported patches?
> There is already a lot of downstream distro bugs reported.

Hi Milan,

I'd really like to see an ack on your patch by one of the crypto/ maintainers
before putting it into a -stable release.


Thanks,
Sasha

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Thomas D.]

Hi,

I have applied Milan's patch on top of 4.1.18. I can reboot and open all
of my LUKS-encrypted disks. "cryptsetup benchmark" also works.

However, don't we need all the recent changes from
"crypto/algif_skcipher.c", too?


-Thomas

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Greg KH]

On Wed, Feb 24, 2016 at 01:10:55AM +0100, Thomas D. wrote:
> Hi,
> 
> I have applied Milan's patch on top of 4.1.18. I can reboot and open all
> of my LUKS-encrypted disks. "cryptsetup benchmark" also works.
> 
> However, don't we need all the recent changes from
> "crypto/algif_skcipher.c", too?

Can someone just backport the full patches in a proper format that I can
apply them in for the 3.14 and 3.10 kernels?  I told people that they
failed to apply, or at least I thought I did...

thanks,

greg k-h

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Jiri Slaby]

On 02/21/2016, 05:40 PM, Milan Broz wrote:
> On 02/20/2016 03:33 PM, Thomas D. wrote:
>> Hi,
>>
>> FYI: v3.10.97, v3.14.61 and 3.18.27 are also affected.
>>
>> v4.3.6 works. Looks like the patch set is only compatible with >=linux-4.3.
>>
>> v3.12.54 works because it doesn't contain the patch in question.
> 
> Hi,
> 
> indeed, because whoever backported this patchset skipped two patches
> from series (because of skcipher interface file was introduced later).
> 
> I tried to backport it (on 4.1.18 tree, see patch below) and it fixes the problem
> for me.
> 
> Anyway, it is just quick test what is the problem, patch need proper review or
> backport from someone who knows the code better.
> 
> I will release stable cryptsetup soon that will work with new kernel even without
> this patch but I would strongly recommend that stable kernels get these compatibility
> backports as well to avoid breaking existing userspace.
> 
> Thanks,
> Milan
> -
> 
> This patch backports missing parts of crypto API compatibility changes:
> 
>   dd504589577d8e8e70f51f997ad487a4cb6c026f
>   crypto: algif_skcipher - Require setkey before accept(2)
> 
>   a0fa2d037129a9849918a92d91b79ed6c7bd2818
>   crypto: algif_skcipher - Add nokey compatibility path
> 
> --- crypto/algif_skcipher.c.orig	2016-02-21 16:44:27.172312990 +0100
> +++ crypto/algif_skcipher.c	2016-02-21 17:03:58.555785347 +0100
...
> @@ -790,24 +912,50 @@
>  	af_alg_release_parent(sk);

This,

>  }
>  
> -static int skcipher_accept_parent(void *private, struct sock *sk)
> +static void skcipher_sock_destruct(struct sock *sk)
> +{
> +	skcipher_sock_destruct_common(sk);
> +	af_alg_release_parent(sk);

this,

> +}
> +
> +static void skcipher_release_parent_nokey(struct sock *sk)
> +{
> +	struct alg_sock *ask = alg_sk(sk);
> +
> +	if (!ask->refcnt) {
> +		sock_put(ask->parent);
> +		return;
> +	}
> +
> +	af_alg_release_parent(sk);

and this occurs to me like a double release?

thanks,
-- 
js
suse labs

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Milan Broz]

On 02/24/2016 09:32 AM, Jiri Slaby wrote:
>> +	af_alg_release_parent(sk);
> 
> and this occurs to me like a double release?

yes, my copy&paste mistake.

Anyway, there should be also two more patches from series.

If it helps, I copied proper backport here (upstream commit is referenced in header)
https://mbroz.fedorapeople.org/tmp/4.1/

Older kernel probably need some more changes but I hope these should be trivial.

Thanks,
Milan

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Greg KH]

On Wed, Feb 24, 2016 at 09:54:48AM +0100, Milan Broz wrote:
> On 02/24/2016 09:32 AM, Jiri Slaby wrote:
> >> +	af_alg_release_parent(sk);
> > 
> > and this occurs to me like a double release?
> 
> yes, my copy&paste mistake.

Which is why I want the real patches backported please.  Whenever we do
a "just this smaller patch" for a stable kernel, it is ALWAYS wrong.

Please backport the patches in a correct way so that we can apply
them...

thanks,

greg k-h

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Milan Broz]

On 02/24/2016 06:12 PM, Greg KH wrote:
> On Wed, Feb 24, 2016 at 09:54:48AM +0100, Milan Broz wrote:
>> On 02/24/2016 09:32 AM, Jiri Slaby wrote:
>>>> +	af_alg_release_parent(sk);
>>>
>>> and this occurs to me like a double release?
>>
>> yes, my copy&paste mistake.
> 
> Which is why I want the real patches backported please.  Whenever we do
> a "just this smaller patch" for a stable kernel, it is ALWAYS wrong.

I think that it was clear that I do not want you to directly include
this patch, just it points to the direction where is the problem.

Anyway, seems the problem is only in 4.1.18.

> Please backport the patches in a correct way so that we can apply
> them...

Not sure if it is still needed, but I'll reply to this thread with my git version
of backported patches for 4.1.18.
(Resp. only the first need changes, other then applied cleanly from upstream).

Thanks,
Milan

[PATCH 1/4] crypto: algif_skcipher - Require setkey before accept(2)

[Milan Broz]

From: Herbert Xu <herbert@gondor.apana.org.au>

    commit dd504589577d8e8e70f51f997ad487a4cb6c026f upstream.

    Some cipher implementations will crash if you try to use them
    without calling setkey first.  This patch adds a check so that
    the accept(2) call will fail with -ENOKEY if setkey hasn't been
    done on the socket yet.

    Cc: stable@vger.kernel.org
    Reported-by: Dmitry Vyukov <dvyukov@google.com>
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
    Tested-by: Dmitry Vyukov <dvyukov@google.com>
    [backported to 4.1 by Milan Broz <gmazyland@gmail.com>]
---
 crypto/algif_skcipher.c | 48 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 7 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 5bc42f9..1c9879d 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -31,6 +31,11 @@ struct skcipher_sg_list {
 	struct scatterlist sg[0];
 };
 
+struct skcipher_tfm {
+	struct crypto_ablkcipher *skcipher;
+	bool has_key;
+};
+
 struct skcipher_ctx {
 	struct list_head tsgl;
 	struct af_alg_sgl rsgl;
@@ -752,17 +757,41 @@ static struct proto_ops algif_skcipher_ops = {
 
 static void *skcipher_bind(const char *name, u32 type, u32 mask)
 {
-	return crypto_alloc_ablkcipher(name, type, mask);
+	struct skcipher_tfm *tfm;
+	struct crypto_ablkcipher *skcipher;
+
+	tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
+	if (!tfm)
+		return ERR_PTR(-ENOMEM);
+
+	skcipher = crypto_alloc_ablkcipher(name, type, mask);
+	if (IS_ERR(skcipher)) {
+		kfree(tfm);
+		return ERR_CAST(skcipher);
+	}
+
+	tfm->skcipher = skcipher;
+
+	return tfm;
 }
 
 static void skcipher_release(void *private)
 {
-	crypto_free_ablkcipher(private);
+	struct skcipher_tfm *tfm = private;
+
+	crypto_free_ablkcipher(tfm->skcipher);
+	kfree(tfm);
 }
 
 static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
 {
-	return crypto_ablkcipher_setkey(private, key, keylen);
+	struct skcipher_tfm *tfm = private;
+	int err;
+
+	err = crypto_ablkcipher_setkey(tfm->skcipher, key, keylen);
+	tfm->has_key = !err;
+
+	return err;
 }
 
 static void skcipher_wait(struct sock *sk)
@@ -794,20 +823,25 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 {
 	struct skcipher_ctx *ctx;
 	struct alg_sock *ask = alg_sk(sk);
-	unsigned int len = sizeof(*ctx) + crypto_ablkcipher_reqsize(private);
+	struct skcipher_tfm *tfm = private;
+	struct crypto_ablkcipher *skcipher = tfm->skcipher;
+	unsigned int len = sizeof(*ctx) + crypto_ablkcipher_reqsize(skcipher);
+
+	if (!tfm->has_key)
+		return -ENOKEY;
 
 	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 
-	ctx->iv = sock_kmalloc(sk, crypto_ablkcipher_ivsize(private),
+	ctx->iv = sock_kmalloc(sk, crypto_ablkcipher_ivsize(skcipher),
 			       GFP_KERNEL);
 	if (!ctx->iv) {
 		sock_kfree_s(sk, ctx, len);
 		return -ENOMEM;
 	}
 
-	memset(ctx->iv, 0, crypto_ablkcipher_ivsize(private));
+	memset(ctx->iv, 0, crypto_ablkcipher_ivsize(skcipher));
 
 	INIT_LIST_HEAD(&ctx->tsgl);
 	ctx->len = len;
@@ -820,7 +854,7 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 
 	ask->private = ctx;
 
-	ablkcipher_request_set_tfm(&ctx->req, private);
+	ablkcipher_request_set_tfm(&ctx->req, skcipher);
 	ablkcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
 					af_alg_complete, &ctx->completion);
 
-- 
2.7.0

[PATCH 2/4] crypto: algif_skcipher - Add nokey compatibility path

[Milan Broz]

From: Herbert Xu <herbert@gondor.apana.org.au>

    commit a0fa2d037129a9849918a92d91b79ed6c7bd2818 upstream.

    This patch adds a compatibility path to support old applications
    that do acept(2) before setkey.

    Cc: stable@vger.kernel.org
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/algif_skcipher.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 144 insertions(+), 5 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 1c9879d..566df2c 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -755,6 +755,99 @@ static struct proto_ops algif_skcipher_ops = {
 	.poll		=	skcipher_poll,
 };
 
+static int skcipher_check_key(struct socket *sock)
+{
+	int err;
+	struct sock *psk;
+	struct alg_sock *pask;
+	struct skcipher_tfm *tfm;
+	struct sock *sk = sock->sk;
+	struct alg_sock *ask = alg_sk(sk);
+
+	if (ask->refcnt)
+		return 0;
+
+	psk = ask->parent;
+	pask = alg_sk(ask->parent);
+	tfm = pask->private;
+
+	err = -ENOKEY;
+	lock_sock(psk);
+	if (!tfm->has_key)
+		goto unlock;
+
+	if (!pask->refcnt++)
+		sock_hold(psk);
+
+	ask->refcnt = 1;
+	sock_put(psk);
+
+	err = 0;
+
+unlock:
+	release_sock(psk);
+
+	return err;
+}
+
+static int skcipher_sendmsg_nokey(struct socket *sock, struct msghdr *msg,
+				  size_t size)
+{
+	int err;
+
+	err = skcipher_check_key(sock);
+	if (err)
+		return err;
+
+	return skcipher_sendmsg(sock, msg, size);
+}
+
+static ssize_t skcipher_sendpage_nokey(struct socket *sock, struct page *page,
+				       int offset, size_t size, int flags)
+{
+	int err;
+
+	err = skcipher_check_key(sock);
+	if (err)
+		return err;
+
+	return skcipher_sendpage(sock, page, offset, size, flags);
+}
+
+static int skcipher_recvmsg_nokey(struct socket *sock, struct msghdr *msg,
+				  size_t ignored, int flags)
+{
+	int err;
+
+	err = skcipher_check_key(sock);
+	if (err)
+		return err;
+
+	return skcipher_recvmsg(sock, msg, ignored, flags);
+}
+
+static struct proto_ops algif_skcipher_ops_nokey = {
+	.family		=	PF_ALG,
+
+	.connect	=	sock_no_connect,
+	.socketpair	=	sock_no_socketpair,
+	.getname	=	sock_no_getname,
+	.ioctl		=	sock_no_ioctl,
+	.listen		=	sock_no_listen,
+	.shutdown	=	sock_no_shutdown,
+	.getsockopt	=	sock_no_getsockopt,
+	.mmap		=	sock_no_mmap,
+	.bind		=	sock_no_bind,
+	.accept		=	sock_no_accept,
+	.setsockopt	=	sock_no_setsockopt,
+
+	.release	=	af_alg_release,
+	.sendmsg	=	skcipher_sendmsg_nokey,
+	.sendpage	=	skcipher_sendpage_nokey,
+	.recvmsg	=	skcipher_recvmsg_nokey,
+	.poll		=	skcipher_poll,
+};
+
 static void *skcipher_bind(const char *name, u32 type, u32 mask)
 {
 	struct skcipher_tfm *tfm;
@@ -804,7 +897,7 @@ static void skcipher_wait(struct sock *sk)
 		msleep(100);
 }
 
-static void skcipher_sock_destruct(struct sock *sk)
+static void skcipher_sock_destruct_common(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
 	struct skcipher_ctx *ctx = ask->private;
@@ -816,10 +909,33 @@ static void skcipher_sock_destruct(struct sock *sk)
 	skcipher_free_sgl(sk);
 	sock_kzfree_s(sk, ctx->iv, crypto_ablkcipher_ivsize(tfm));
 	sock_kfree_s(sk, ctx, ctx->len);
+}
+
+static void skcipher_sock_destruct(struct sock *sk)
+{
+	skcipher_sock_destruct_common(sk);
 	af_alg_release_parent(sk);
 }
 
-static int skcipher_accept_parent(void *private, struct sock *sk)
+static void skcipher_release_parent_nokey(struct sock *sk)
+{
+	struct alg_sock *ask = alg_sk(sk);
+
+	if (!ask->refcnt) {
+		sock_put(ask->parent);
+		return;
+	}
+
+	af_alg_release_parent(sk);
+}
+
+static void skcipher_sock_destruct_nokey(struct sock *sk)
+{
+	skcipher_sock_destruct_common(sk);
+	skcipher_release_parent_nokey(sk);
+}
+
+static int skcipher_accept_parent_common(void *private, struct sock *sk)
 {
 	struct skcipher_ctx *ctx;
 	struct alg_sock *ask = alg_sk(sk);
@@ -827,9 +943,6 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 	struct crypto_ablkcipher *skcipher = tfm->skcipher;
 	unsigned int len = sizeof(*ctx) + crypto_ablkcipher_reqsize(skcipher);
 
-	if (!tfm->has_key)
-		return -ENOKEY;
-
 	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
@@ -863,12 +976,38 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 	return 0;
 }
 
+static int skcipher_accept_parent(void *private, struct sock *sk)
+{
+	struct skcipher_tfm *tfm = private;
+
+	if (!tfm->has_key)
+		return -ENOKEY;
+
+	return skcipher_accept_parent_common(private, sk);
+}
+
+static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
+{
+	int err;
+
+	err = skcipher_accept_parent_common(private, sk);
+	if (err)
+		goto out;
+
+	sk->sk_destruct = skcipher_sock_destruct_nokey;
+
+out:
+	return err;
+}
+
 static const struct af_alg_type algif_type_skcipher = {
 	.bind		=	skcipher_bind,
 	.release	=	skcipher_release,
 	.setkey		=	skcipher_setkey,
 	.accept		=	skcipher_accept_parent,
+	.accept_nokey	=	skcipher_accept_parent_nokey,
 	.ops		=	&algif_skcipher_ops,
+	.ops_nokey	=	&algif_skcipher_ops_nokey,
 	.name		=	"skcipher",
 	.owner		=	THIS_MODULE
 };
-- 
2.7.0

[PATCH 3/4] crypto: algif_skcipher - Remove custom release parent function

[Milan Broz]

From: Herbert Xu <herbert@gondor.apana.org.au>

    commit d7b65aee1e7b4c87922b0232eaba56a8a143a4a0 upstream.

    This patch removes the custom release parent function as the
    generic af_alg_release_parent now works for nokey sockets too.

    Cc: stable@vger.kernel.org
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/algif_skcipher.c | 43 +++----------------------------------------
 1 file changed, 3 insertions(+), 40 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 566df2c..83bcf75 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -897,7 +897,7 @@ static void skcipher_wait(struct sock *sk)
 		msleep(100);
 }
 
-static void skcipher_sock_destruct_common(struct sock *sk)
+static void skcipher_sock_destruct(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
 	struct skcipher_ctx *ctx = ask->private;
@@ -909,33 +909,10 @@ static void skcipher_sock_destruct_common(struct sock *sk)
 	skcipher_free_sgl(sk);
 	sock_kzfree_s(sk, ctx->iv, crypto_ablkcipher_ivsize(tfm));
 	sock_kfree_s(sk, ctx, ctx->len);
-}
-
-static void skcipher_sock_destruct(struct sock *sk)
-{
-	skcipher_sock_destruct_common(sk);
-	af_alg_release_parent(sk);
-}
-
-static void skcipher_release_parent_nokey(struct sock *sk)
-{
-	struct alg_sock *ask = alg_sk(sk);
-
-	if (!ask->refcnt) {
-		sock_put(ask->parent);
-		return;
-	}
-
 	af_alg_release_parent(sk);
 }
 
-static void skcipher_sock_destruct_nokey(struct sock *sk)
-{
-	skcipher_sock_destruct_common(sk);
-	skcipher_release_parent_nokey(sk);
-}
-
-static int skcipher_accept_parent_common(void *private, struct sock *sk)
+static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
 {
 	struct skcipher_ctx *ctx;
 	struct alg_sock *ask = alg_sk(sk);
@@ -983,21 +960,7 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 	if (!tfm->has_key)
 		return -ENOKEY;
 
-	return skcipher_accept_parent_common(private, sk);
-}
-
-static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
-{
-	int err;
-
-	err = skcipher_accept_parent_common(private, sk);
-	if (err)
-		goto out;
-
-	sk->sk_destruct = skcipher_sock_destruct_nokey;
-
-out:
-	return err;
+	return skcipher_accept_parent_nokey(private, sk);
 }
 
 static const struct af_alg_type algif_type_skcipher = {
-- 
2.7.0

[PATCH 4/4] crypto: algif_skcipher - Fix race condition in skcipher_check_key

[Milan Broz]

From: Herbert Xu <herbert@gondor.apana.org.au>

    commit 1822793a523e5d5730b19cc21160ff1717421bc8 upstream.

    We need to lock the child socket in skcipher_check_key as otherwise
    two simultaneous calls can cause the parent socket to be freed.

    Cc: stable@vger.kernel.org
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/algif_skcipher.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 83bcf75..c0f0356 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -757,22 +757,23 @@ static struct proto_ops algif_skcipher_ops = {
 
 static int skcipher_check_key(struct socket *sock)
 {
-	int err;
+	int err = 0;
 	struct sock *psk;
 	struct alg_sock *pask;
 	struct skcipher_tfm *tfm;
 	struct sock *sk = sock->sk;
 	struct alg_sock *ask = alg_sk(sk);
 
+	lock_sock(sk);
 	if (ask->refcnt)
-		return 0;
+		goto unlock_child;
 
 	psk = ask->parent;
 	pask = alg_sk(ask->parent);
 	tfm = pask->private;
 
 	err = -ENOKEY;
-	lock_sock(psk);
+	lock_sock_nested(psk, SINGLE_DEPTH_NESTING);
 	if (!tfm->has_key)
 		goto unlock;
 
@@ -786,6 +787,8 @@ static int skcipher_check_key(struct socket *sock)
 
 unlock:
 	release_sock(psk);
+unlock_child:
+	release_sock(sk);
 
 	return err;
 }
-- 
2.7.0

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Sasha Levin]

On 02/26/2016 06:25 AM, Milan Broz wrote:
> On 02/24/2016 06:12 PM, Greg KH wrote:
>> On Wed, Feb 24, 2016 at 09:54:48AM +0100, Milan Broz wrote:
>>> On 02/24/2016 09:32 AM, Jiri Slaby wrote:
>>>>> +	af_alg_release_parent(sk);
>>>>
>>>> and this occurs to me like a double release?
>>>
>>> yes, my copy&paste mistake.
>>
>> Which is why I want the real patches backported please.  Whenever we do
>> a "just this smaller patch" for a stable kernel, it is ALWAYS wrong.
> 
> I think that it was clear that I do not want you to directly include
> this patch, just it points to the direction where is the problem.
> 
> Anyway, seems the problem is only in 4.1.18.
> 
>> Please backport the patches in a correct way so that we can apply
>> them...
> 
> Not sure if it is still needed, but I'll reply to this thread with my git version
> of backported patches for 4.1.18.
> (Resp. only the first need changes, other then applied cleanly from upstream).

Please do.


Thanks,
Sasha

Re: [PATCH 1/4] crypto: algif_skcipher - Require setkey before accept(2)

[Herbert Xu]

On Fri, Feb 26, 2016 at 12:44:08PM +0100, Milan Broz wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> 
>     commit dd504589577d8e8e70f51f997ad487a4cb6c026f upstream.
> 
>     Some cipher implementations will crash if you try to use them
>     without calling setkey first.  This patch adds a check so that
>     the accept(2) call will fail with -ENOKEY if setkey hasn't been
>     done on the socket yet.
> 
>     Cc: stable@vger.kernel.org
>     Reported-by: Dmitry Vyukov <dvyukov@google.com>
>     Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>     Tested-by: Dmitry Vyukov <dvyukov@google.com>
>     [backported to 4.1 by Milan Broz <gmazyland@gmail.com>]

Ack to all four patches.

Thanks Milan!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/4] crypto: algif_skcipher - Require setkey before accept(2)

[Sasha Levin]

On 02/26/2016 06:44 AM, Milan Broz wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> 
>     commit dd504589577d8e8e70f51f997ad487a4cb6c026f upstream.
> 
>     Some cipher implementations will crash if you try to use them
>     without calling setkey first.  This patch adds a check so that
>     the accept(2) call will fail with -ENOKEY if setkey hasn't been
>     done on the socket yet.
> 
>     Cc: stable@vger.kernel.org
>     Reported-by: Dmitry Vyukov <dvyukov@google.com>
>     Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>     Tested-by: Dmitry Vyukov <dvyukov@google.com>
>     [backported to 4.1 by Milan Broz <gmazyland@gmail.com>]

Thanks Milan. Can I add your Signed-off-by to this series?


Thanks,
Sasha

Re: [PATCH 1/4] crypto: algif_skcipher - Require setkey before accept(2)

[Milan Broz]

On 02/27/2016 10:40 PM, Sasha Levin wrote:
> On 02/26/2016 06:44 AM, Milan Broz wrote:
>> From: Herbert Xu <herbert@gondor.apana.org.au>
>>
>>     commit dd504589577d8e8e70f51f997ad487a4cb6c026f upstream.
>>
>>     Some cipher implementations will crash if you try to use them
>>     without calling setkey first.  This patch adds a check so that
>>     the accept(2) call will fail with -ENOKEY if setkey hasn't been
>>     done on the socket yet.
>>
>>     Cc: stable@vger.kernel.org
>>     Reported-by: Dmitry Vyukov <dvyukov@google.com>
>>     Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>>     Tested-by: Dmitry Vyukov <dvyukov@google.com>
>>     [backported to 4.1 by Milan Broz <gmazyland@gmail.com>]
> 
> Thanks Milan. Can I add your Signed-off-by to this series?

yes,
Signed-off-by: Milan Broz <gmazyland@gmail.com>

Thanks,
Milan

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Thomas D.]

Hi,

Sasha, can you please revert commit
f857638dd72680e2a8faafef7eebb4534cb39fd1 like Greg did with linux-3.10.101

> commit 1f2493fcd87bd810c608aa7976388157852eadb2
> Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Date:   Sat Mar 12 21:30:16 2016 -0800
> 
>     Revert: "crypto: af_alg - Disallow bind/setkey/... after accept(2)"
>     
>     This reverts commit 5a707f0972e1c9d8a4a921ddae79d0f9dc36a341 which is
>     commit c840ac6af3f8713a71b4d2363419145760bd6044 upstream.
>     
>     It's been widely reported that this patch breaks existing userspace
>     applications when backported to the stable kernel releases.  As no fix
>     seems to be forthcoming, just revert it to let systems work again.

and linux-3.14.65

> commit c4eb62da6f34bfa9bbcbd005210a90fdfca7e367
> Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Date:   Sat Mar 12 21:30:16 2016 -0800
> 
>     Revert: "crypto: af_alg - Disallow bind/setkey/... after accept(2)"
>     
>     This reverts commit 06b4194533ff92ed5888840e3a6beaf29a8fe5d4 which is
>     commit c840ac6af3f8713a71b4d2363419145760bd6044 upstream.
>     
>     It's been widely reported that this patch breaks existing userspace
>     applications when backported to the stable kernel releases.  As no fix
>     seems to be forthcoming, just revert it to let systems work again.


Linux-3.18.x is the only LTS kernel left with this problem. If nobody
cares we should at least revert back to a working kernel...


-Thomas

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Sasha Levin]

On 04/17/2016 06:17 PM, Thomas D. wrote:
> Hi,
> 
> Sasha, can you please revert commit
> f857638dd72680e2a8faafef7eebb4534cb39fd1 like Greg did with linux-3.10.101
> 
>> commit 1f2493fcd87bd810c608aa7976388157852eadb2
>> Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Date:   Sat Mar 12 21:30:16 2016 -0800
>>
>>     Revert: "crypto: af_alg - Disallow bind/setkey/... after accept(2)"
>>     
>>     This reverts commit 5a707f0972e1c9d8a4a921ddae79d0f9dc36a341 which is
>>     commit c840ac6af3f8713a71b4d2363419145760bd6044 upstream.
>>     
>>     It's been widely reported that this patch breaks existing userspace
>>     applications when backported to the stable kernel releases.  As no fix
>>     seems to be forthcoming, just revert it to let systems work again.
> 
> and linux-3.14.65
> 
>> commit c4eb62da6f34bfa9bbcbd005210a90fdfca7e367
>> Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Date:   Sat Mar 12 21:30:16 2016 -0800
>>
>>     Revert: "crypto: af_alg - Disallow bind/setkey/... after accept(2)"
>>     
>>     This reverts commit 06b4194533ff92ed5888840e3a6beaf29a8fe5d4 which is
>>     commit c840ac6af3f8713a71b4d2363419145760bd6044 upstream.
>>     
>>     It's been widely reported that this patch breaks existing userspace
>>     applications when backported to the stable kernel releases.  As no fix
>>     seems to be forthcoming, just revert it to let systems work again.
> 
> 
> Linux-3.18.x is the only LTS kernel left with this problem. If nobody
> cares we should at least revert back to a working kernel...

So I mixed stuff up here, I've received a backport that would fix this problem
on 4.1, and applied it. However, I forgot about 3.18.

Would Milan's backport work on 3.18 as well (https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg17949.html)?


Thanks,
Sasha

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Herbert Xu]

On Sun, Apr 17, 2016 at 06:39:18PM -0400, Sasha Levin wrote:
>
> So I mixed stuff up here, I've received a backport that would fix this problem
> on 4.1, and applied it. However, I forgot about 3.18.
> 
> Would Milan's backport work on 3.18 as well (https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg17949.html)?

It should work.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Thomas D.]

Hi,

Milan's patches apply against 3.18.30, however I am getting build errors:

> *  CC [M]  fs/fat/namei_vfat.o
> *  CC [M]  crypto/algif_hash.o
> *  LD [M]  fs/isofs/isofs.o
> *  CC [M]  crypto/algif_skcipher.o
> *  LD [M]  fs/fat/fat.o
> *crypto/algif_hash.c:350:13: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> *  .sendmsg = hash_sendmsg_nokey,
> *             ^
> *crypto/algif_hash.c:350:13: note: (near initialization for ‘algif_hash_ops_nokey.sendmsg’)
> *crypto/algif_hash.c:352:13: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> *--
> *crypto/algif_hash.c:352:13: note: (near initialization for ‘algif_hash_ops_nokey.recvmsg’)
> *  CC [M]  drivers/acpi/fan.o
> *  CC [M]  crypto/xor.o
> *  LD [M]  fs/fat/msdos.o
> *crypto/algif_skcipher.c: In function ‘skcipher_sendmsg_nokey’:
> *crypto/algif_skcipher.c:599:26: warning: passing argument 1 of ‘skcipher_sendmsg’ from incompatible pointer type [-Wincompatible-pointer-types]
> *  return skcipher_sendmsg(sock, msg, size);
> *                          ^
> *crypto/algif_skcipher.c:247:12: note: expected ‘struct kiocb *’ but argument is of type ‘struct socket *’
> * static int skcipher_sendmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c:599:32: warning: passing argument 2 of ‘skcipher_sendmsg’ from incompatible pointer type [-Wincompatible-pointer-types]
> *  return skcipher_sendmsg(sock, msg, size);
> *                                ^
> *crypto/algif_skcipher.c:247:12: note: expected ‘struct socket *’ but argument is of type ‘struct msghdr *’
> * static int skcipher_sendmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c:599:37: warning: passing argument 3 of ‘skcipher_sendmsg’ makes pointer from integer without a cast [-Wint-conversion]
> *  return skcipher_sendmsg(sock, msg, size);
> *                                     ^
> *crypto/algif_skcipher.c:247:12: note: expected ‘struct msghdr *’ but argument is of type ‘size_t {aka long unsigned int}’
> * static int skcipher_sendmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c:599:9: error: too few arguments to function ‘skcipher_sendmsg’
> *--
> *         ^
> *crypto/algif_skcipher.c:247:12: note: declared here
> * static int skcipher_sendmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c: In function ‘skcipher_recvmsg_nokey’:
> *crypto/algif_skcipher.c:623:26: warning: passing argument 1 of ‘skcipher_recvmsg’ from incompatible pointer type [-Wincompatible-pointer-types]
> *  return skcipher_recvmsg(sock, msg, ignored, flags);
> *                          ^
> *crypto/algif_skcipher.c:426:12: note: expected ‘struct kiocb *’ but argument is of type ‘struct socket *’
> * static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c:623:32: warning: passing argument 2 of ‘skcipher_recvmsg’ from incompatible pointer type [-Wincompatible-pointer-types]
> *  return skcipher_recvmsg(sock, msg, ignored, flags);
> *                                ^
> *crypto/algif_skcipher.c:426:12: note: expected ‘struct socket *’ but argument is of type ‘struct msghdr *’
> * static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c:623:37: warning: passing argument 3 of ‘skcipher_recvmsg’ makes pointer from integer without a cast [-Wint-conversion]
> *  return skcipher_recvmsg(sock, msg, ignored, flags);
> *                                     ^
> *crypto/algif_skcipher.c:426:12: note: expected ‘struct msghdr *’ but argument is of type ‘size_t {aka long unsigned int}’
> * static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c:623:9: error: too few arguments to function ‘skcipher_recvmsg’
> *--
> *         ^
> *crypto/algif_skcipher.c:426:12: note: declared here
> * static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c: At top level:
> *crypto/algif_skcipher.c:642:13: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> *  .sendmsg = skcipher_sendmsg_nokey,
> *             ^
> *crypto/algif_skcipher.c:642:13: note: (near initialization for ‘algif_skcipher_ops_nokey.sendmsg’)
> *crypto/algif_skcipher.c:644:13: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> *  .recvmsg = skcipher_recvmsg_nokey,
> *             ^
> *crypto/algif_skcipher.c:644:13: note: (near initialization for ‘algif_skcipher_ops_nokey.recvmsg’)
> *crypto/algif_skcipher.c: In function ‘skcipher_recvmsg_nokey’:
> *crypto/algif_skcipher.c:624:1: warning: control reaches end of non-void function [-Wreturn-type]
> * }
> * ^
> *crypto/algif_skcipher.c: In function ‘skcipher_sendmsg_nokey’:
> *  CC [M]  crypto/async_tx/async_tx.o
> *crypto/algif_skcipher.c:600:1: warning: control reaches end of non-void function [-Wreturn-type]
> * }
> * ^
> *  LD [M]  fs/fat/vfat.o
> *scripts/Makefile.build:263: recipe for target 'crypto/algif_skcipher.o' failed
> *make[1]: *** [crypto/algif_skcipher.o] Error 1
> *--
> *  CC [M]  arch/x86/oprofile/../../../drivers/oprofile/buffer_sync.o
> *  CC [M]  drivers/i2c/busses/i2c-piix4.o
> *  CC [M]  arch/x86/oprofile/../../../drivers/oprofile/event_buffer.o
> *  CC [M]  arch/x86/oprofile/../../../drivers/oprofile/oprofile_files.o
> *  CC [M]  arch/x86/oprofile/../../../drivers/oprofile/oprofile_stats.o
> *Makefile:937: recipe for target 'crypto' failed
> *make: *** [crypto] Error 2


-Thomas

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Thomas D.]

Hi,

Milan's patches apply against 3.18.30, however I am getting build errors:

> *  CC [M]  fs/fat/namei_vfat.o
> *  CC [M]  crypto/algif_hash.o
> *  LD [M]  fs/isofs/isofs.o
> *  CC [M]  crypto/algif_skcipher.o
> *  LD [M]  fs/fat/fat.o
> *crypto/algif_hash.c:350:13: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> *  .sendmsg = hash_sendmsg_nokey,
> *             ^
> *crypto/algif_hash.c:350:13: note: (near initialization for �algif_hash_ops_nokey.sendmsg�)
> *crypto/algif_hash.c:352:13: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> *--
> *crypto/algif_hash.c:352:13: note: (near initialization for �algif_hash_ops_nokey.recvmsg�)
> *  CC [M]  drivers/acpi/fan.o
> *  CC [M]  crypto/xor.o
> *  LD [M]  fs/fat/msdos.o
> *crypto/algif_skcipher.c: In function �skcipher_sendmsg_nokey�:
> *crypto/algif_skcipher.c:599:26: warning: passing argument 1 of �skcipher_sendmsg� from incompatible pointer type [-Wincompatible-pointer-types]
> *  return skcipher_sendmsg(sock, msg, size);
> *                          ^
> *crypto/algif_skcipher.c:247:12: note: expected �struct kiocb *� but argument is of type �struct socket *�
> * static int skcipher_sendmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c:599:32: warning: passing argument 2 of �skcipher_sendmsg� from incompatible pointer type [-Wincompatible-pointer-types]
> *  return skcipher_sendmsg(sock, msg, size);
> *                                ^
> *crypto/algif_skcipher.c:247:12: note: expected �struct socket *� but argument is of type �struct msghdr *�
> * static int skcipher_sendmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c:599:37: warning: passing argument 3 of �skcipher_sendmsg� makes pointer from integer without a cast [-Wint-conversion]
> *  return skcipher_sendmsg(sock, msg, size);
> *                                     ^
> *crypto/algif_skcipher.c:247:12: note: expected �struct msghdr *� but argument is of type �size_t {aka long unsigned int}�
> * static int skcipher_sendmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c:599:9: error: too few arguments to function �skcipher_sendmsg�
> *--
> *         ^
> *crypto/algif_skcipher.c:247:12: note: declared here
> * static int skcipher_sendmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c: In function �skcipher_recvmsg_nokey�:
> *crypto/algif_skcipher.c:623:26: warning: passing argument 1 of �skcipher_recvmsg� from incompatible pointer type [-Wincompatible-pointer-types]
> *  return skcipher_recvmsg(sock, msg, ignored, flags);
> *                          ^
> *crypto/algif_skcipher.c:426:12: note: expected �struct kiocb *� but argument is of type �struct socket *�
> * static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c:623:32: warning: passing argument 2 of �skcipher_recvmsg� from incompatible pointer type [-Wincompatible-pointer-types]
> *  return skcipher_recvmsg(sock, msg, ignored, flags);
> *                                ^
> *crypto/algif_skcipher.c:426:12: note: expected �struct socket *� but argument is of type �struct msghdr *�
> * static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c:623:37: warning: passing argument 3 of �skcipher_recvmsg� makes pointer from integer without a cast [-Wint-conversion]
> *  return skcipher_recvmsg(sock, msg, ignored, flags);
> *                                     ^
> *crypto/algif_skcipher.c:426:12: note: expected �struct msghdr *� but argument is of type �size_t {aka long unsigned int}�
> * static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c:623:9: error: too few arguments to function �skcipher_recvmsg�
> *--
> *         ^
> *crypto/algif_skcipher.c:426:12: note: declared here
> * static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
> *            ^
> *crypto/algif_skcipher.c: At top level:
> *crypto/algif_skcipher.c:642:13: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> *  .sendmsg = skcipher_sendmsg_nokey,
> *             ^
> *crypto/algif_skcipher.c:642:13: note: (near initialization for �algif_skcipher_ops_nokey.sendmsg�)
> *crypto/algif_skcipher.c:644:13: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> *  .recvmsg = skcipher_recvmsg_nokey,
> *             ^
> *crypto/algif_skcipher.c:644:13: note: (near initialization for �algif_skcipher_ops_nokey.recvmsg�)
> *crypto/algif_skcipher.c: In function �skcipher_recvmsg_nokey�:
> *crypto/algif_skcipher.c:624:1: warning: control reaches end of non-void function [-Wreturn-type]
> * }
> * ^
> *crypto/algif_skcipher.c: In function �skcipher_sendmsg_nokey�:
> *  CC [M]  crypto/async_tx/async_tx.o
> *crypto/algif_skcipher.c:600:1: warning: control reaches end of non-void function [-Wreturn-type]
> * }
> * ^
> *  LD [M]  fs/fat/vfat.o
> *scripts/Makefile.build:263: recipe for target 'crypto/algif_skcipher.o' failed
> *make[1]: *** [crypto/algif_skcipher.o] Error 1
> *--
> *  CC [M]  arch/x86/oprofile/../../../drivers/oprofile/buffer_sync.o
> *  CC [M]  drivers/i2c/busses/i2c-piix4.o
> *  CC [M]  arch/x86/oprofile/../../../drivers/oprofile/event_buffer.o
> *  CC [M]  arch/x86/oprofile/../../../drivers/oprofile/oprofile_files.o
> *  CC [M]  arch/x86/oprofile/../../../drivers/oprofile/oprofile_stats.o
> *Makefile:937: recipe for target 'crypto' failed
> *make: *** [crypto] Error 2


-Thomas

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Sasha Levin]

On 04/18/2016 05:48 AM, Thomas D. wrote:
> Hi,
> 
> Milan's patches apply against 3.18.30, however I am getting build errors:

Milan, Herbert, should I just be reverting the offending patches:

bcfa841 crypto: af_alg - Forbid bind(2) when nokey child sockets are present
eb1ab00 crypto: af_alg - Allow af_af_alg_release_parent to be called on nokey path
3db68eb crypto: af_alg - Add nokey compatibility path
ac9c75f crypto: af_alg - Fix socket double-free when accept fails
f857638 crypto: af_alg - Disallow bind/setkey/... after accept(2)

Or will you send me a backport?

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Milan Broz]

On 04/18/2016 02:54 PM, Sasha Levin wrote:
> On 04/18/2016 05:48 AM, Thomas D. wrote:
>> Hi,
>>
>> Milan's patches apply against 3.18.30, however I am getting build errors:
> 
> Milan, Herbert, should I just be reverting the offending patches:
> 
> bcfa841 crypto: af_alg - Forbid bind(2) when nokey child sockets are present
> eb1ab00 crypto: af_alg - Allow af_af_alg_release_parent to be called on nokey path
> 3db68eb crypto: af_alg - Add nokey compatibility path
> ac9c75f crypto: af_alg - Fix socket double-free when accept fails
> f857638 crypto: af_alg - Disallow bind/setkey/... after accept(2)
> 
> Or will you send me a backport?

Hi,

could you please try backported patches here
https://mbroz.fedorapeople.org/tmp/3.18/ ?

It should be the same backport as I sent for 4.1 here (just with some 3.18 socket specifics).
("cryptsetup benchmark" works again and that command uses this API.)

I think similar patches should be in 3.12 stable tree already.

Thanks,
Milan

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Thomas D.]

Hi,

Milan Broz wrote:
> could you please try backported patches here
> https://mbroz.fedorapeople.org/tmp/3.18/ ?

This patch set works for me and fixes my reported problem (tested
against 3.18.30).

Thank you!


-Thomas

Re: [PATCH] Re: Broken userspace crypto in linux-4.1.18

[Sasha Levin]

On 04/18/2016 04:56 PM, Thomas D. wrote:
> Hi,
> 
> Milan Broz wrote:
>> could you please try backported patches here
>> https://mbroz.fedorapeople.org/tmp/3.18/ ?
> 
> This patch set works for me and fixes my reported problem (tested
> against 3.18.30).
> 
> Thank you!

Excellent, I'll add this in and release. Thank you both.

Sorry for missing it on my end.


Thanks,
Sasha