Re: Very slow unlockall()

[Milan Broz]

On 01/02/2021 19:55, Vlastimil Babka wrote:
> On 2/1/21 7:00 PM, Milan Broz wrote:
>> On 01/02/2021 14:08, Vlastimil Babka wrote:
>>> On 1/8/21 3:39 PM, Milan Broz wrote:
>>>> On 08/01/2021 14:41, Michal Hocko wrote:
>>>>> On Wed 06-01-21 16:20:15, Milan Broz wrote:
>>>>>> Hi,
>>>>>>
>>>>>> we use mlockall(MCL_CURRENT | MCL_FUTURE) / munlockall() in cryptsetup 
>>>>>> code
>>>>>> and someone tried to use it with hardened memory allocator library.
>>>>>>
>>>>>> Execution time was increased to extreme (minutes) and as we found, the 
>>>>>> problem
>>>>>> is in munlockall().
>>>>>>
>>>>>> Here is a plain reproducer for the core without any external code - it 
>>>>>> takes
>>>>>> unlocking on Fedora rawhide kernel more than 30 seconds!
>>>>>> I can reproduce it on 5.10 kernels and Linus' git.
>>>>>>
>>>>>> The reproducer below tries to mmap large amount memory with PROT_NONE 
>>>>>> (later never used).
>>>>>> The real code of course does something more useful but the problem is 
>>>>>> the same.
>>>>>>
>>>>>> #include 
>>>>>> #include 
>>>>>> #include 
>>>>>> #include 
>>>>>>
>>>>>> int main (int argc, char *argv[])
>>>>>> {
>>>>>> void *p  = mmap(NULL, 1UL << 41, PROT_NONE, MAP_PRIVATE | 
>>>>>> MAP_ANONYMOUS, -1, 0);
> 
> So, this is 2TB memory area, but PROT_NONE means it's never actually 
> populated,
> although mlockall(MCL_CURRENT) should do that. Once you put PROT_READ |
> PROT_WRITE there, the mlockall() starts taking ages.
> 
> So does that reflect your use case? munlockall() with large PROT_NONE areas? 
> If
> so, munlock_vma_pages_range() is indeed not optimized for that, but I would
> expect such scenario to be uncommon, so better clarify first.

It is just a simple reproducer of the underlying problem, as suggested here 
https://gitlab.com/cryptsetup/cryptsetup/-/issues/617#note_478342301

We use mlockall() in cryptsetup and with hardened malloc it slows down unlock 
significantly.
(For the real case problem please read the whole issue report above.)

m.

Re: Very slow unlockall()

[Milan Broz]

On 01/02/2021 14:08, Vlastimil Babka wrote:
> On 1/8/21 3:39 PM, Milan Broz wrote:
>> On 08/01/2021 14:41, Michal Hocko wrote:
>>> On Wed 06-01-21 16:20:15, Milan Broz wrote:
>>>> Hi,
>>>>
>>>> we use mlockall(MCL_CURRENT | MCL_FUTURE) / munlockall() in cryptsetup code
>>>> and someone tried to use it with hardened memory allocator library.
>>>>
>>>> Execution time was increased to extreme (minutes) and as we found, the 
>>>> problem
>>>> is in munlockall().
>>>>
>>>> Here is a plain reproducer for the core without any external code - it 
>>>> takes
>>>> unlocking on Fedora rawhide kernel more than 30 seconds!
>>>> I can reproduce it on 5.10 kernels and Linus' git.
>>>>
>>>> The reproducer below tries to mmap large amount memory with PROT_NONE 
>>>> (later never used).
>>>> The real code of course does something more useful but the problem is the 
>>>> same.
>>>>
>>>> #include 
>>>> #include 
>>>> #include 
>>>> #include 
>>>>
>>>> int main (int argc, char *argv[])
>>>> {
>>>> void *p  = mmap(NULL, 1UL << 41, PROT_NONE, MAP_PRIVATE | 
>>>> MAP_ANONYMOUS, -1, 0);
>>>>
>>>> if (p == MAP_FAILED) return 1;
>>>>
>>>> if (mlockall(MCL_CURRENT | MCL_FUTURE)) return 1;
>>>> printf("locked\n");
>>>>
>>>> if (munlockall()) return 1;
>>>> printf("unlocked\n");
>>>>
>>>> return 0;
>>>> }

...

>> Today's Linus git - 5.11.0-rc2+ in my testing x86_64 VM (no extensive kernel 
>> debug options):
>>
>> # time ./lock
>> locked
>> unlocked
>>
>> real0m4.172s
>> user0m0.000s
>> sys 0m4.172s
> 
> The perf report would be more interesting from this configuration.

ok, I cannot run perf on that particular VM but tried the latest Fedora stable
kernel without debug options  - 5.10.12-200.fc33.x86_64

This is the report running reproducer above:

time:
real0m6.123s
user0m0.099s
sys 0m5.310s

perf:

# Total Lost Samples: 0
#
# Samples: 20K of event 'cycles'
# Event count (approx.): 20397603279
#
# Overhead  Command  Shared Object  Symbol  
#   ...  .  
#
47.26%  lock [kernel.kallsyms]  [k] follow_page_mask
20.43%  lock [kernel.kallsyms]  [k] munlock_vma_pages_range
15.92%  lock [kernel.kallsyms]  [k] follow_page
 7.40%  lock [kernel.kallsyms]  [k] rcu_all_qs
 5.87%  lock [kernel.kallsyms]  [k] _cond_resched
 3.08%  lock [kernel.kallsyms]  [k] follow_huge_addr
 0.01%  lock [kernel.kallsyms]  [k] __update_load_avg_cfs_rq
 0.01%  lock [kernel.kallsyms]  [k] fput
 0.01%  lock [kernel.kallsyms]  [k] rmap_walk_file
 0.00%  lock [kernel.kallsyms]  [k] page_mapped
 0.00%  lock [kernel.kallsyms]  [k] native_irq_return_iret
 0.00%  lock [kernel.kallsyms]  [k] _raw_spin_lock_irq
 0.00%  lock [kernel.kallsyms]  [k] perf_iterate_ctx
 0.00%  lock [kernel.kallsyms]  [k] finish_task_switch
 0.00%  perf [kernel.kallsyms]  [k] native_sched_clock
 0.00%  lock [kernel.kallsyms]  [k] native_write_msr
 0.00%  perf [kernel.kallsyms]  [k] native_write_msr


m.

Re: Very slow unlockall()

[Milan Broz]

On 08/01/2021 15:39, Milan Broz wrote:
> On 08/01/2021 14:41, Michal Hocko wrote:
>> On Wed 06-01-21 16:20:15, Milan Broz wrote:
>>> Hi,
>>>
>>> we use mlockall(MCL_CURRENT | MCL_FUTURE) / munlockall() in cryptsetup code
>>> and someone tried to use it with hardened memory allocator library.
>>>
>>> Execution time was increased to extreme (minutes) and as we found, the 
>>> problem
>>> is in munlockall().
>>>
>>> Here is a plain reproducer for the core without any external code - it takes
>>> unlocking on Fedora rawhide kernel more than 30 seconds!
>>> I can reproduce it on 5.10 kernels and Linus' git.
>>>
>>> The reproducer below tries to mmap large amount memory with PROT_NONE 
>>> (later never used).
>>> The real code of course does something more useful but the problem is the 
>>> same.
>>>
>>> #include 
>>> #include 
>>> #include 
>>> #include 
>>>
>>> int main (int argc, char *argv[])
>>> {
>>> void *p  = mmap(NULL, 1UL << 41, PROT_NONE, MAP_PRIVATE | 
>>> MAP_ANONYMOUS, -1, 0);
>>>
>>> if (p == MAP_FAILED) return 1;
>>>
>>> if (mlockall(MCL_CURRENT | MCL_FUTURE)) return 1;
>>> printf("locked\n");
>>>
>>> if (munlockall()) return 1;
>>> printf("unlocked\n");
>>>
>>> return 0;
>>> }
>>>
>>> In traceback I see that time is spent in munlock_vma_pages_range.
>>>
>>> [ 2962.006813] Call Trace:
>>> [ 2962.006814]  ? munlock_vma_pages_range+0xe7/0x4b0
>>> [ 2962.006814]  ? vma_merge+0xf3/0x3c0
>>> [ 2962.006815]  ? mlock_fixup+0x111/0x190
>>> [ 2962.006815]  ? apply_mlockall_flags+0xa7/0x110
>>> [ 2962.006816]  ? __do_sys_munlockall+0x2e/0x60
>>> [ 2962.006816]  ? do_syscall_64+0x33/0x40
>>> ...
>>>
>>> Or with perf, I see
>>>
>>> # Overhead  Command  Shared Object  Symbol  
>>>  
>>> #   ...  .  
>>> .
>>> #
>>> 48.18%  lock [kernel.kallsyms]  [k] lock_is_held_type
>>> 11.67%  lock [kernel.kallsyms]  [k] ___might_sleep
>>> 10.65%  lock [kernel.kallsyms]  [k] follow_page_mask
>>>  9.17%  lock [kernel.kallsyms]  [k] debug_lockdep_rcu_enabled
>>>  6.73%  lock [kernel.kallsyms]  [k] munlock_vma_pages_range
>>> ...
>>>
>>>
>>> Could please anyone check what's wrong here with the memory locking code?
>>> Running it on my notebook I can effectively DoS the system :)
>>>
>>> Original report is https://gitlab.com/cryptsetup/cryptsetup/-/issues/617
>>> but this is apparently a kernel issue, just amplified by usage of 
>>> munlockall().
>>
>> Which kernel version do you see this with? Have older releases worked
>> better?
> 
> Hi,
> 
> I tried 5.10 stable and randomly few kernels I have built on testing VM (5.3 
> was the oldest),
> it seems to be very similar run time, so the problem is apparently old...(I 
> can test some specific kernel version if it make any sense).
> 
> For mainline (reproducer above):
> 
> With 5.11.0-0.rc2.20210106git36bbbd0e234d.117.fc34.x86_64 (latest Fedora 
> rawhide kernel build - many debug options are on)
> 
> # time ./lock 
> locked
> unlocked
> 
> real0m32.287s
> user0m0.001s
> sys 0m32.126s
> 
> 
> Today's Linus git - 5.11.0-rc2+ in my testing x86_64 VM (no extensive kernel 
> debug options):
> 
> # time ./lock
> locked
> unlocked
> 
> real0m4.172s
> user0m0.000s
> sys 0m4.172s
> 
> m.

Hi,

so because there is no response, is this expected behavior of memory management 
subsystem then?

Thanks,
Milan



 

Re: Very slow unlockall()

[Milan Broz]

On 08/01/2021 14:41, Michal Hocko wrote:
> On Wed 06-01-21 16:20:15, Milan Broz wrote:
>> Hi,
>>
>> we use mlockall(MCL_CURRENT | MCL_FUTURE) / munlockall() in cryptsetup code
>> and someone tried to use it with hardened memory allocator library.
>>
>> Execution time was increased to extreme (minutes) and as we found, the 
>> problem
>> is in munlockall().
>>
>> Here is a plain reproducer for the core without any external code - it takes
>> unlocking on Fedora rawhide kernel more than 30 seconds!
>> I can reproduce it on 5.10 kernels and Linus' git.
>>
>> The reproducer below tries to mmap large amount memory with PROT_NONE (later 
>> never used).
>> The real code of course does something more useful but the problem is the 
>> same.
>>
>> #include 
>> #include 
>> #include 
>> #include 
>>
>> int main (int argc, char *argv[])
>> {
>> void *p  = mmap(NULL, 1UL << 41, PROT_NONE, MAP_PRIVATE | 
>> MAP_ANONYMOUS, -1, 0);
>>
>> if (p == MAP_FAILED) return 1;
>>
>> if (mlockall(MCL_CURRENT | MCL_FUTURE)) return 1;
>> printf("locked\n");
>>
>> if (munlockall()) return 1;
>> printf("unlocked\n");
>>
>> return 0;
>> }
>>
>> In traceback I see that time is spent in munlock_vma_pages_range.
>>
>> [ 2962.006813] Call Trace:
>> [ 2962.006814]  ? munlock_vma_pages_range+0xe7/0x4b0
>> [ 2962.006814]  ? vma_merge+0xf3/0x3c0
>> [ 2962.006815]  ? mlock_fixup+0x111/0x190
>> [ 2962.006815]  ? apply_mlockall_flags+0xa7/0x110
>> [ 2962.006816]  ? __do_sys_munlockall+0x2e/0x60
>> [ 2962.006816]  ? do_syscall_64+0x33/0x40
>> ...
>>
>> Or with perf, I see
>>
>> # Overhead  Command  Shared Object  Symbol   
>> #   ...  .  .
>> #
>> 48.18%  lock [kernel.kallsyms]  [k] lock_is_held_type
>> 11.67%  lock [kernel.kallsyms]  [k] ___might_sleep
>> 10.65%  lock [kernel.kallsyms]  [k] follow_page_mask
>>  9.17%  lock [kernel.kallsyms]  [k] debug_lockdep_rcu_enabled
>>  6.73%  lock [kernel.kallsyms]  [k] munlock_vma_pages_range
>> ...
>>
>>
>> Could please anyone check what's wrong here with the memory locking code?
>> Running it on my notebook I can effectively DoS the system :)
>>
>> Original report is https://gitlab.com/cryptsetup/cryptsetup/-/issues/617
>> but this is apparently a kernel issue, just amplified by usage of 
>> munlockall().
> 
> Which kernel version do you see this with? Have older releases worked
> better?

Hi,

I tried 5.10 stable and randomly few kernels I have built on testing VM (5.3 
was the oldest),
it seems to be very similar run time, so the problem is apparently old...(I can 
test some specific kernel version if it make any sense).

For mainline (reproducer above):

With 5.11.0-0.rc2.20210106git36bbbd0e234d.117.fc34.x86_64 (latest Fedora 
rawhide kernel build - many debug options are on)

# time ./lock 
locked
unlocked

real0m32.287s
user0m0.001s
sys 0m32.126s


Today's Linus git - 5.11.0-rc2+ in my testing x86_64 VM (no extensive kernel 
debug options):

# time ./lock
locked
unlocked

real0m4.172s
user0m0.000s
sys 0m4.172s

m.

Very slow unlockall()

[Milan Broz]

Hi,

we use mlockall(MCL_CURRENT | MCL_FUTURE) / munlockall() in cryptsetup code
and someone tried to use it with hardened memory allocator library.

Execution time was increased to extreme (minutes) and as we found, the problem
is in munlockall().

Here is a plain reproducer for the core without any external code - it takes
unlocking on Fedora rawhide kernel more than 30 seconds!
I can reproduce it on 5.10 kernels and Linus' git.

The reproducer below tries to mmap large amount memory with PROT_NONE (later 
never used).
The real code of course does something more useful but the problem is the same.

#include 
#include 
#include 
#include 

int main (int argc, char *argv[])
{
void *p  = mmap(NULL, 1UL << 41, PROT_NONE, MAP_PRIVATE | 
MAP_ANONYMOUS, -1, 0);

if (p == MAP_FAILED) return 1;

if (mlockall(MCL_CURRENT | MCL_FUTURE)) return 1;
printf("locked\n");

if (munlockall()) return 1;
printf("unlocked\n");

return 0;
}

In traceback I see that time is spent in munlock_vma_pages_range.

[ 2962.006813] Call Trace:
[ 2962.006814]  ? munlock_vma_pages_range+0xe7/0x4b0
[ 2962.006814]  ? vma_merge+0xf3/0x3c0
[ 2962.006815]  ? mlock_fixup+0x111/0x190
[ 2962.006815]  ? apply_mlockall_flags+0xa7/0x110
[ 2962.006816]  ? __do_sys_munlockall+0x2e/0x60
[ 2962.006816]  ? do_syscall_64+0x33/0x40
...

Or with perf, I see

# Overhead  Command  Shared Object  Symbol   
#   ...  .  .
#
48.18%  lock [kernel.kallsyms]  [k] lock_is_held_type
11.67%  lock [kernel.kallsyms]  [k] ___might_sleep
10.65%  lock [kernel.kallsyms]  [k] follow_page_mask
 9.17%  lock [kernel.kallsyms]  [k] debug_lockdep_rcu_enabled
 6.73%  lock [kernel.kallsyms]  [k] munlock_vma_pages_range
...


Could please anyone check what's wrong here with the memory locking code?
Running it on my notebook I can effectively DoS the system :)

Original report is https://gitlab.com/cryptsetup/cryptsetup/-/issues/617
but this is apparently a kernel issue, just amplified by usage of munlockall().

Thanks,
Milan

Re: [PATCH v3 1/4] crypto: add eboiv as a crypto API template

[Milan Broz]

On 29/10/2020 11:05, Gilad Ben-Yossef wrote:
>  
> +config CRYPTO_EBOIV
> + tristate "EBOIV support for block encryption"
> + default DM_CRYPT
> + select CRYPTO_CBC
> + help
> +   Encrypted byte-offset initialization vector (EBOIV) is an IV
> +   generation method that is used in some cases by dm-crypt for
> +   supporting the BitLocker volume encryption used by Windows 8
> +   and onwards as a backwards compatible version in lieu of XTS
> +   support.
> +
> +   It uses the block encryption key as the symmetric key for a
> +   block encryption pass applied to the sector offset of the block.
> +   Additional details can be found at
> +   https://www.jedec.org/sites/default/files/docs/JESD223C.pdf

This page is not available. Are you sure this is the proper documentation?

I think the only description we used (for dm-crypt) was original Ferguson's 
Bitlocker doc:
https://download.microsoft.com/download/0/2/3/0238acaf-d3bf-4a6d-b3d6-0a0be4bbb36e/bitlockercipher200608.pdf

IIRC EBOIV was a shortcut I added to dm-crypt because we found no official 
terminology for this IV.
And after lunchtime, nobody invented anything better, so it stayed as it is now 
:-)

Milan

Re: [PATCH 3/4] dm crypt: switch to EBOIV crypto API template

[Milan Broz]

On 27/10/2020 07:59, Gilad Ben-Yossef wrote:
> On Mon, Oct 26, 2020 at 9:04 PM Milan Broz  wrote:
...
>> We had all of disk-IV inside dmcrypt before - but once it is partially moved 
>> into crypto API
>> (ESSIV, EBOIV for now), it becomes much more complicated for user to select 
>> what he needs.
>>
>> I think we have no way to check that IV is available from userspace - it
>> will report the same error as if block cipher is not available, not helping 
>> user much
>> with the error.
>>
>> But then I also think we should add abstract dm-crypt options here (Legacy 
>> TrueCrypt modes,
>> Bitlocker modes) that will select these crypto API configuration switches.
>> Otherwise it will be only a complicated matrix of crypto API options...
> 
> hm... just thinking out loud, but maybe the right say to go is to not
> have a build dependency,
> but add some user assistance code in cryptosetup that parses
> /proc/crypto after failures to
> try and suggest the user with a way forward?
> 
> e.g. if eboiv mapping initiation fails, scan /proc/crypto and either
> warn of a lack of AES
> or, assuming some instance of AES is found, warn of lack of EBOIV.
> It's a little messy
> and heuristic code for sure, but it lives in a user space utility.
> 
> Does that sound sane?

Such an idea (try to parse /proc/crypto) is on my TODO list since 2009 :)
I expected userspace kernel crypto API could help here, but it seems it is not 
the case.

So yes, I think we need to add something like this in userspace. In combination 
with
the kernel and dmcrypt target version, we could have a pretty good hint matrix 
for the user,
instead of (literally) cryptic errors.

(There is also a problem that device-mapper targets are losing detailed error 
state.
We often end just with -EINVAL during table create ... and no descriptive log 
entry.
And leaking info about encrypted devices activation failures to syslog is not a 
good idea either.)

Anyway, this will not fix existing userspace that is not prepared for this kind
of EBOIV missing fail, so Herbert's solution seems like the solution for this 
particular
problem for now. (But I agree we should perhaps remove these build dependences 
in future completely...)

Milan

Re: [PATCH 3/4] dm crypt: switch to EBOIV crypto API template

[Milan Broz]

On 26/10/2020 19:39, Eric Biggers wrote:
> On Mon, Oct 26, 2020 at 07:29:57PM +0100, Milan Broz wrote:
>> On 26/10/2020 18:52, Eric Biggers wrote:
>>> On Mon, Oct 26, 2020 at 03:04:46PM +0200, Gilad Ben-Yossef wrote:
>>>> Replace the explicit EBOIV handling in the dm-crypt driver with calls
>>>> into the crypto API, which now possesses the capability to perform
>>>> this processing within the crypto subsystem.
>>>>
>>>> Signed-off-by: Gilad Ben-Yossef 
>>>>
>>>> ---
>>>>  drivers/md/Kconfig|  1 +
>>>>  drivers/md/dm-crypt.c | 61 ++-
>>>>  2 files changed, 20 insertions(+), 42 deletions(-)
>>>>
>>>> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
>>>> index 30ba3573626c..ca6e56a72281 100644
>>>> --- a/drivers/md/Kconfig
>>>> +++ b/drivers/md/Kconfig
>>>> @@ -273,6 +273,7 @@ config DM_CRYPT
>>>>select CRYPTO
>>>>select CRYPTO_CBC
>>>>select CRYPTO_ESSIV
>>>> +  select CRYPTO_EBOIV
>>>>help
>>>>  This device-mapper target allows you to create a device that
>>>>  transparently encrypts the data on it. You'll need to activate
>>>
>>> Can CRYPTO_EBOIV please not be selected by default?  If someone really wants
>>> Bitlocker compatibility support, they can select this option themselves.
>>
>> Please no! Until this move of IV to crypto API, we can rely on
>> support in dm-crypt (if it is not supported, it is just a very old kernel).
>> (Actually, this was the first thing I checked in this patchset - if it is
>> unconditionally enabled for compatibility once dmcrypt is selected.)
>>
>> People already use removable devices with BitLocker.
>> It was the whole point that it works out-of-the-box without enabling 
>> anything.
>>
>> If you insist on this to be optional, please better keep this IV inside 
>> dmcrypt.
>> (EBOIV has no other use than for disk encryption anyway.)
>>
>> Or maybe another option would be to introduce option under dm-crypt Kconfig 
>> that
>> defaults to enabled (like support for foreign/legacy disk encryption 
>> schemes) and that
>> selects these IVs/modes.
>> But requiring some random switch in crypto API will only confuse users.
> 
> CONFIG_DM_CRYPT can either select every weird combination of algorithms anyone
> can ever be using, or it can select some defaults and require any other needed
> algorithms to be explicitly selected.
> 
> In reality, dm-crypt has never even selected any particular block ciphers, 
> even
> AES.  Nor has it ever selected XTS.  So it's actually always made users (or
> kernel distributors) explicitly select algorithms.  Why the Bitlocker support
> suddenly different?
> 
> I'd think a lot of dm-crypt users don't want to bloat their kernels with 
> random
> legacy algorithms.

Yes, but IV is in reality not a cryptographic algorithm, it is kind-of a 
configuration
"option" of sector encryption mode here.

We had all of disk-IV inside dmcrypt before - but once it is partially moved 
into crypto API
(ESSIV, EBOIV for now), it becomes much more complicated for user to select 
what he needs.

I think we have no way to check that IV is available from userspace - it
will report the same error as if block cipher is not available, not helping 
user much
with the error.

But then I also think we should add abstract dm-crypt options here (Legacy 
TrueCrypt modes,
Bitlocker modes) that will select these crypto API configuration switches.
Otherwise it will be only a complicated matrix of crypto API options...

Milan

Re: [PATCH 3/4] dm crypt: switch to EBOIV crypto API template

[Milan Broz]

On 26/10/2020 18:52, Eric Biggers wrote:
> On Mon, Oct 26, 2020 at 03:04:46PM +0200, Gilad Ben-Yossef wrote:
>> Replace the explicit EBOIV handling in the dm-crypt driver with calls
>> into the crypto API, which now possesses the capability to perform
>> this processing within the crypto subsystem.
>>
>> Signed-off-by: Gilad Ben-Yossef 
>>
>> ---
>>  drivers/md/Kconfig|  1 +
>>  drivers/md/dm-crypt.c | 61 ++-
>>  2 files changed, 20 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
>> index 30ba3573626c..ca6e56a72281 100644
>> --- a/drivers/md/Kconfig
>> +++ b/drivers/md/Kconfig
>> @@ -273,6 +273,7 @@ config DM_CRYPT
>>  select CRYPTO
>>  select CRYPTO_CBC
>>  select CRYPTO_ESSIV
>> +select CRYPTO_EBOIV
>>  help
>>This device-mapper target allows you to create a device that
>>transparently encrypts the data on it. You'll need to activate
> 
> Can CRYPTO_EBOIV please not be selected by default?  If someone really wants
> Bitlocker compatibility support, they can select this option themselves.

Please no! Until this move of IV to crypto API, we can rely on
support in dm-crypt (if it is not supported, it is just a very old kernel).
(Actually, this was the first thing I checked in this patchset - if it is
unconditionally enabled for compatibility once dmcrypt is selected.)

People already use removable devices with BitLocker.
It was the whole point that it works out-of-the-box without enabling anything.

If you insist on this to be optional, please better keep this IV inside dmcrypt.
(EBOIV has no other use than for disk encryption anyway.)

Or maybe another option would be to introduce option under dm-crypt Kconfig that
defaults to enabled (like support for foreign/legacy disk encryption schemes) 
and that
selects these IVs/modes.
But requiring some random switch in crypto API will only confuse users.

Milan

Re: [PATCH v2] dm verity: Add support for signature verification with 2nd keyring

[Milan Broz]

On 16/10/2020 10:49, Mickaël Salaün wrote:

On 16/10/2020 10:29, Mickaël Salaün wrote:


On 15/10/2020 18:52, Mike Snitzer wrote:

Can you please explain why you've decided to make this a Kconfig CONFIG
knob?  Why not either add: a dm-verity table argument? A dm-verity
kernel module parameter? or both (to allow a particular default but then
per-device override)?


The purpose of signed dm-verity images is to authenticate files, or said
in another way, to enable the kernel to trust disk images in a flexible
way (i.e. thanks to certificate's chain of trust). Being able to update
such chain at run time requires to use the second trusted keyring. This
keyring automatically includes the certificate authorities from the
builtin trusted keyring, which are required to dynamically populate the
secondary trusted keyring with certificates signed by an already trusted
authority. The roots of trust must then be included at build time in the
builtin trusted keyring.

To be meaningful, using dm-verity signatures implies to have a
restricted user space, i.e. even the root user has limited power over
the kernel and the rest of the system. Blindly trusting data provided by
user space (e.g. dm-verity table argument, kernel module parameter)
defeat the purpose of (mandatory) authenticated images.



Otherwise, _all_ DM verity devices will be configured to use secondary
keyring fallback.  Is that really desirable?


That is already the current state (on purpose).


I meant that when DM_VERITY_VERIFY_ROOTHASH_SIG is set, dm-verity
signature becomes mandatory. This new configuration
DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING extend this trust to the
secondary trusted keyring, which contains certificates signed (directly
or indirectly) by CA from the builtin trusted keyring.

So yes, this new (optional) configuration *extends* the source of trust
for all dm-verity devices, and yes, it is desirable. I think it should
have been this way from the beginning (as for other authentication
mechanisms) but it wasn't necessary at that time.


Well, I understand why you need a config option here.
And using the secondary keyring actually makes much more sense to me than
the original approach.

But please do not forget that dm-verity is sometimes used in different
contexts where such strict in-kernel certificate trust is unnecessary.
With your configure options set, you deliberately remove the possibility
to configure such devices.
I understand that it is needed for "trusted" systems, but we should be clear
in the documentation.
Maybe also add note to /Documentation/admin-guide/device-mapper/verity.rst ?
We already mention DM_VERITY_VERIFY_ROOTHASH_SIG there.

The current userspace configuration through veritysetup does not need
any patches for your patch, correct?

Thanks,
Milan

Re: [PATCH v3 2/2] dm-crypt: collect data and submit to DM to measure

[Milan Broz]

On 28/08/2020 22:27, Tushar Sugandhi wrote:
> Currently, dm-crypt does not take advantage of IMA measuring
> capabilities, and ultimately the benefits of remote attestation.
> 
> Measure various dm-crypt constructs by calling various device-mapper
> functions - dm_ima_*() that use IMA measuring capabilities. Implement
> ima_measure_dm_crypt_data() to measure various dm-crypt constructs.
> 
> Ensure that ima_measure_dm_crypt_data() is non intrusive, i.e. failures
> in this function and the call-stack below should not affect the core
> functionality of dm-crypt.

Just my opinion, but I really do not like to add every relevant DM table option
as a harcoded value here. (But maybe it is necessary, dunno).

Why you cannot measure the whole device-mapper table as a string?
(output of STATUSTYPE_TABLE).
But it has own problems, like table can contain key etc.

Anyway with the above, the whole measurement can reside in DM core (I hope).

But there are some problems - we can activate device with optional flags
(for example allow_discards) - should this be IMA measurement?

And what about device size (you already measure offset)?

IMO it depends on situation (policy).

It is quite often that we add performance flags later (like these no_workqueue 
in 5.9).
Some of them should be measured because there is possible security/data 
integrity impact.

And one note - input table accepts also a device path, but output of table is 
always
major:minor, see:

# dmsetup create test --table "0 2097152 crypt aes-cbc-essiv:sha256 
9c1185a5c5e9fc54612808977ee8f548b2258d31ddadef707ba62c166051b9e3 0 /dev/sdg 0"
# dmsetup table test --showkeys
0 2097152 crypt aes-cbc-essiv:sha256 
9c1185a5c5e9fc54612808977ee8f548b2258d31ddadef707ba62c166051b9e3 0 8:96 0

I think dm_table_device_name() should work here (as you use it), but major 
number can change
on different activations (in general) later, isn't it problem here?
(Like dynamic major for DM devices that depends on module sequence load.)

Milan

> 
> Register dm-crypt as supported data source for IMA measurement in ima.h.
> 
> A demonstrative usage of above functionality on a system:
> 
> If the IMA policy contains the following rule:
> 
> measure func=CRITICAL_DATA critical_kernel_data_sources=dm-crypt 
> template=ima-buf
> 
> and, the following commands are used to setup a crypt target:
> 
>  #key="faf453b4ee938cff2f0d2c869a0b743f59125c0a37f5bcd8f1dbbd911a78abaa"
>  #arg="'0 1953125 crypt aes-xts-plain64 "
>  #arg="$arg $key 0 "
>  #arg="$arg /dev/loop0 0 1 allow_discards'"
>  #tgt_name="test-crypt"
>  #cmd="dmsetup create $tgt_name --table $arg"
>  #eval $cmd
> 
> then, the IMA log at
> /sys/kernel/security/integrity/ima/ascii_runtime_measurements should
> contain the dm-crypt measurements. And, the following IMA log entry
> should be added in the IMA log,
> 
>  ima-buf sha1:039d8ff71918608d585adca3e5aab2e3f41f84d6 
>  1598637500:520585536:dm-crypt:add_target 
>  74695f6e756d5f646973636172645f62696f733d313b7065725f62696f5f646
>  174615f73697a653d3834383b646d7265715f73746172743d3136383b74666d
>  735f636f756e743d313b6f6e5f6469736b5f7461675f73697a653d303b696e7
>  46567726974795f69765f73697a653d303b696e746567726974795f7461675f
>  73697a653d303b69765f73697a653d31363b69765f6f7365743d303b736
>  563746f725f73686966743d303b736563746f725f73697a653d3531323b666c
>  6167733d323b6369706865725f666c6167733d303b73746172743d303b6b657
>  95f6d61635f73697a653d303b6b65795f65787472615f73697a653d303b6b65
>  795f70617274733d313b6b65795f73697a653d33323b6369706865725f73747
>  2696e673d6165732d7874732d706c61696e36343b6465766963655f6e616d65
>  3d3235333a303b
> 
> where, the ascii representation of the above data is:
> 
>  ti_num_discard_bios=1;per_bio_data_size=848;dmreq_start=168;
>  tfms_count=1;on_disk_tag_size=0;integrity_iv_size=0;
>  integrity_tag_size=0;iv_size=16;iv_offset=0;sector_shift=0;
>  sector_size=512;flags=2;cipher_flags=0;start=0;key_mac_size=0;
>  key_extra_size=0;key_parts=1;key_size=32;
>  cipher_string=aes-xts-plain64;device_name=253:0;
> 
> Some of the above values can be verified using:
> 
>  #dmsetup table --showkeys
> 
> where, the output of the command should be similar to:
> 
>  test-crypt: 0 1953125 crypt aes-xts-plain64
>  faf453b4ee938cff2f0d2c869a0b743f59125c0a37f5bcd8f1dbbd911a78abaa
>  0 7:0 0 1 allow_discards
> 
> Signed-off-by: Tushar Sugandhi 
> ---
>  drivers/md/dm-crypt.c  | 171 +
>  security/integrity/ima/Kconfig |   3 +-
>  security/integrity/ima/ima.h   |   1 +
>  3 files changed, 173 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 148960721254..47fb2ce15211 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -2529,6 +2529,8 @@ static void crypt_dtr(struct dm_target *ti)
>  
>   ti->private = NULL;
>  
> + dm_ima_exit_measurements(ti->type);
> +
>   if (!cc)
>   return;
>  
> @@ -2991,6 +2993,167 @@ static int 

Re: New mode DM-Verity error handling

[Milan Broz]

On 23/06/2020 01:53, JeongHyeon Lee wrote:
> 
> For what reason isn't panic better?

I did not say panic is better, I said that while we have restart already in 
mainline dm-verity code,
panic() is almost the same, so I see no problem in merging this patch.

Stopping system this way could create more damage if it is not configured 
properly,
but I think it is quite common to stop the system as fast as possible if data 
system integrity
is violated...

> If when i suggested new patch, i will send you a patch that increased 
> minor version.

I think Mike can fold-in version increase, if the patch is accepted.

But please include these version changes with every new feature.

Actually I am tracking it here for dm-verity as part of veritysetup userspace 
documentation:
  https://gitlab.com/cryptsetup/cryptsetup/-/wikis/DMVerity

Thanks,
Milan

> On 22/06/2020 16:58, Milan Broz wrote:
>> On 18/06/2020 19:09, Mike Snitzer wrote:
>>> On Thu, Jun 18 2020 at 12:50pm -0400,
>>> Sami Tolvanen  wrote:
>>>
>>>> On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote:
>>>>> I do not accept that panicing the system because of verity failure is
>>>>> reasonable.
>>>>>
>>>>> In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong.
>>>>>
>>>>> The device should be put in a failed state and left for admin recovery.
>>>> That's exactly how the restart mode works on some Android devices. The
>>>> bootloader sees the verification error and puts the device in recovery
>>>> mode. Using the restart mode on systems without firmware support won't
>>>> make sense, obviously.
>>> OK, so I need further justification from Samsung why they are asking for
>>> this panic mode.
>> I think when we have reboot already, panic is not much better :-)
>>
>> Just please note that dm-verity is used not only in Android world (with own 
>> tooling)
>> but in normal Linux distributions, and I need to modify userspace 
>> (veritysetup) to support
>> and recognize this flag.
>>
>> Please *always* increase minor dm-verity target version when adding a new 
>> feature
>> - we can then provide some better hint if it is not supported.
>>
>> Thanks,
>> Milan
>>
>>

Re: New mode DM-Verity error handling

[Milan Broz]

On 18/06/2020 19:09, Mike Snitzer wrote:
> On Thu, Jun 18 2020 at 12:50pm -0400,
> Sami Tolvanen  wrote:
> 
>> On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote:
>>> I do not accept that panicing the system because of verity failure is
>>> reasonable.
>>>
>>> In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong.
>>>
>>> The device should be put in a failed state and left for admin recovery.
>>
>> That's exactly how the restart mode works on some Android devices. The
>> bootloader sees the verification error and puts the device in recovery
>> mode. Using the restart mode on systems without firmware support won't
>> make sense, obviously.
> 
> OK, so I need further justification from Samsung why they are asking for
> this panic mode.

I think when we have reboot already, panic is not much better :-)

Just please note that dm-verity is used not only in Android world (with own 
tooling)
but in normal Linux distributions, and I need to modify userspace (veritysetup) 
to support
and recognize this flag.

Please *always* increase minor dm-verity target version when adding a new 
feature
- we can then provide some better hint if it is not supported.

Thanks,
Milan

[PATCH v2] tpm: Fix null pointer dereference on chip register error path

[Milan Broz]

If clk_enable is not defined and chip initialization
is canceled code hits null dereference.

Easily reproducible with vTPM init fail:
  swtpm chardev --tpmstate dir=nonexistent_dir --tpm2 --vtpm-proxy

BUG: kernel NULL pointer dereference, address: 
...
Call Trace:
 tpm_chip_start+0x9d/0xa0 [tpm]
 tpm_chip_register+0x10/0x1a0 [tpm]
 vtpm_proxy_work+0x11/0x30 [tpm_vtpm_proxy]
 process_one_work+0x214/0x5a0
 worker_thread+0x134/0x3e0
 ? process_one_work+0x5a0/0x5a0
 kthread+0xd4/0x100
 ? process_one_work+0x5a0/0x5a0
 ? kthread_park+0x90/0x90
 ret_from_fork+0x19/0x24

Fixes: 719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")
Cc: sta...@vger.kernel.org # v5.1+
Signed-off-by: Milan Broz 
---
 drivers/char/tpm/tpm-chip.c | 23 ---
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 90325e1749fb..db6ac6f83948 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -77,6 +77,18 @@ static int tpm_go_idle(struct tpm_chip *chip)
return chip->ops->go_idle(chip);
 }
 
+static void tpm_clk_enable(struct tpm_chip *chip)
+{
+   if (chip->ops->clk_enable)
+   chip->ops->clk_enable(chip, true);
+}
+
+static void tpm_clk_disable(struct tpm_chip *chip)
+{
+   if (chip->ops->clk_enable)
+   chip->ops->clk_enable(chip, false);
+}
+
 /**
  * tpm_chip_start() - power on the TPM
  * @chip:  a TPM chip to use
@@ -89,13 +101,12 @@ int tpm_chip_start(struct tpm_chip *chip)
 {
int ret;
 
-   if (chip->ops->clk_enable)
-   chip->ops->clk_enable(chip, true);
+   tpm_clk_enable(chip);
 
if (chip->locality == -1) {
ret = tpm_request_locality(chip);
if (ret) {
-   chip->ops->clk_enable(chip, false);
+   tpm_clk_disable(chip);
return ret;
}
}
@@ -103,8 +114,7 @@ int tpm_chip_start(struct tpm_chip *chip)
ret = tpm_cmd_ready(chip);
if (ret) {
tpm_relinquish_locality(chip);
-   if (chip->ops->clk_enable)
-   chip->ops->clk_enable(chip, false);
+   tpm_clk_disable(chip);
return ret;
}
 
@@ -124,8 +134,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
 {
tpm_go_idle(chip);
tpm_relinquish_locality(chip);
-   if (chip->ops->clk_enable)
-   chip->ops->clk_enable(chip, false);
+   tpm_clk_disable(chip);
 }
 EXPORT_SYMBOL_GPL(tpm_chip_stop);
 
-- 
2.20.1

Re: [PATCH v2 0/2] tty: erase buffers when the kernel is done with it.

[Milan Broz]

On 04/10/2018 20:06, Greg Kroah-Hartman wrote:
> azlig and Milan Broz reported that when the tty layer is done with a
> buffer, the data can hang around in it for a very long time.  That
> sometimes can "leak" to userspace under some conditions.
> 
> Because of this, just zero out the data after the tty layer is finished
> with it, for buffers that we "think" should be zeroed out.
> 
> v2 - addressed some review comments on the 2/2 patch.

Hello Greg,

together with our intern we re-tested both patches and it fixes the reported 
problem,
so, if it helps anything, you can add

Tested-by: Milan Broz 
Tested-by: Daniel Zatovic 

Do you plan to add this to linux-next?

Thanks!
Milan

> 
> Greg Kroah-Hartman (1):
>   tty: wipe buffer if not echoing data
> 
> Linus Torvalds (1):
>   tty: wipe buffer.
> 
>  drivers/tty/n_tty.c  | 20 +---
>  drivers/tty/tty_buffer.c |  6 +-
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 

Re: [PATCH v2 0/2] tty: erase buffers when the kernel is done with it.

[Milan Broz]

On 04/10/2018 20:06, Greg Kroah-Hartman wrote:
> azlig and Milan Broz reported that when the tty layer is done with a
> buffer, the data can hang around in it for a very long time.  That
> sometimes can "leak" to userspace under some conditions.
> 
> Because of this, just zero out the data after the tty layer is finished
> with it, for buffers that we "think" should be zeroed out.
> 
> v2 - addressed some review comments on the 2/2 patch.

Hello Greg,

together with our intern we re-tested both patches and it fixes the reported 
problem,
so, if it helps anything, you can add

Tested-by: Milan Broz 
Tested-by: Daniel Zatovic 

Do you plan to add this to linux-next?

Thanks!
Milan

> 
> Greg Kroah-Hartman (1):
>   tty: wipe buffer if not echoing data
> 
> Linus Torvalds (1):
>   tty: wipe buffer.
> 
>  drivers/tty/n_tty.c  | 20 +---
>  drivers/tty/tty_buffer.c |  6 +-
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 

Re: [PATCH 2/2] tty: wipe buffer if not echoing data

[Milan Broz]

On 02/10/2018 19:17, Greg Kroah-Hartman wrote:
> From: Greg KH 
> 
> If we are not echoing the data to userspace, then perhaps it is a
> "secret" so we should wipe it once we are done with it.

Just to explain our test case for cryptsetup, where aszlig initially reported 
it:

cryptsetup reads a passphrase from terminal, derives a candidate key for keyslot
decryption and immediately wipes the passphrase from memory, because it is
not needed anymore.

And here is the problem - the kernel keeps this passphrase in a tty buffer 
memory
that is no longer available to userspace. It can be retrieved from memory dump
later, even after the crypt mapping is destroyed.
Almost all userspace tools working with passphrases through tty access have
the same problem here.

> This mirrors the logic that the audit code has.
> 
> Reported-by: aszlig 
> Tested-by: Milan Broz 
> Tested-by: aszlig 
> Cc: Willy Tarreau 
> Signed-off-by: Greg Kroah-Hartman 
> ---
>  drivers/tty/n_tty.c | 20 +---
...

>  static int tty_copy_to_user(struct tty_struct *tty, void __user *to,
>   size_t tail, size_t n)
>  {
>   struct n_tty_data *ldata = tty->disc_data;
>   size_t size = N_TTY_BUF_SIZE - tail;
> - const void *from = read_buf_addr(ldata, tail);
> + void *from = read_buf_addr(ldata, tail);
>   int uncopied;
>  
>   if (n > size) {
>   tty_audit_add_data(tty, from, size);
>   uncopied = copy_to_user(to, from, size);
> + zero_buffer(tty, from, size);

I think Linus mentioned in some previous mail that there should be
  zero_buffer(tty, from, size - uncopied);
to avoid wiping of yet uncopied data.

I tested the unmodified version though...

>   if (uncopied)
>   return uncopied;
>   to += size;
> @@ -171,7 +182,9 @@ static int tty_copy_to_user(struct tty_struct *tty, void 
> __user *to,
>   }
>  
>   tty_audit_add_data(tty, from, n);
> - return copy_to_user(to, from, n);
> + uncopied = copy_to_user(to, from, n);
> + zero_buffer(tty, from, n);
> + return uncopied;
>  }


Milan

Re: [PATCH 2/2] tty: wipe buffer if not echoing data

[Milan Broz]

On 02/10/2018 19:17, Greg Kroah-Hartman wrote:
> From: Greg KH 
> 
> If we are not echoing the data to userspace, then perhaps it is a
> "secret" so we should wipe it once we are done with it.

Just to explain our test case for cryptsetup, where aszlig initially reported 
it:

cryptsetup reads a passphrase from terminal, derives a candidate key for keyslot
decryption and immediately wipes the passphrase from memory, because it is
not needed anymore.

And here is the problem - the kernel keeps this passphrase in a tty buffer 
memory
that is no longer available to userspace. It can be retrieved from memory dump
later, even after the crypt mapping is destroyed.
Almost all userspace tools working with passphrases through tty access have
the same problem here.

> This mirrors the logic that the audit code has.
> 
> Reported-by: aszlig 
> Tested-by: Milan Broz 
> Tested-by: aszlig 
> Cc: Willy Tarreau 
> Signed-off-by: Greg Kroah-Hartman 
> ---
>  drivers/tty/n_tty.c | 20 +---
...

>  static int tty_copy_to_user(struct tty_struct *tty, void __user *to,
>   size_t tail, size_t n)
>  {
>   struct n_tty_data *ldata = tty->disc_data;
>   size_t size = N_TTY_BUF_SIZE - tail;
> - const void *from = read_buf_addr(ldata, tail);
> + void *from = read_buf_addr(ldata, tail);
>   int uncopied;
>  
>   if (n > size) {
>   tty_audit_add_data(tty, from, size);
>   uncopied = copy_to_user(to, from, size);
> + zero_buffer(tty, from, size);

I think Linus mentioned in some previous mail that there should be
  zero_buffer(tty, from, size - uncopied);
to avoid wiping of yet uncopied data.

I tested the unmodified version though...

>   if (uncopied)
>   return uncopied;
>   to += size;
> @@ -171,7 +182,9 @@ static int tty_copy_to_user(struct tty_struct *tty, void 
> __user *to,
>   }
>  
>   tty_audit_add_data(tty, from, n);
> - return copy_to_user(to, from, n);
> + uncopied = copy_to_user(to, from, n);
> + zero_buffer(tty, from, n);
> + return uncopied;
>  }


Milan

Re: [PATCH] md: dm-crypt: Add Inline Encryption support for dmcrypt

[Milan Broz]

On 05/28/2018 03:01 PM, Ladvine D Almeida wrote:
> 
> This patch adds new option
> -- perform_inline_encrypt
> that set the option inside dmcrypt to use inline encryption for
> the configured mapping. I want to introduce inline encryption support
> in the UFS Host Controller driver. The usage of the same for accomplishing
> the Full Disk Encryption is done by the following changes in the
> dmcrypt subsystem:
> - New function crypt_inline_encrypt_submit() is added to the
> dmcrypt which associate the crypto context to the bios which
> are submitted by the subsystem.
> - Successful configuration for the inline encryption will result in
> the crypto transformation job being bypassed in the dmcrypt layer and the
> actual encryption happens inline to the block device.

I am not sure this is a good idea. Dm-crypt should never ever forward
plaintext sector to underlying device.

And you do not even check that your hw is available in the underlying device!

If you want to use dm-crypt, write a crypto API driver for your crypto hw that 
defines
specific cipher and let dm-crypt use this cipher (no patches needed in this 
case!)

If I read the patch correctly, you do not check any parameters for
compatibility with your hw support (cipher, mode, IV algorithm, key length, 
sector size ...)

It seems like if the "perform_inline_encrypt" option is present, you just submit
the whole bio to your code with new INLINE_ENCRYPTION bit set.

What happens, if the cipher in table is different from what your hw is doing?

Milan

> Another patch set is sent to the block layer community for
> CONFIG_BLK_DEV_INLINE_ENCRYPTION config, which enables changes in the
> block layer for adding the bi_ie_private variable to the bio structure.
> 
> Signed-off-by: Ladvine D Almeida 
> ---
>  drivers/md/dm-crypt.c | 55 
> +--
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 44ff473..a9ed567 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -39,6 +39,7 @@
>  #include 
>  
>  #define DM_MSG_PREFIX "crypt"
> +#define REQ_INLINE_ENCRYPTION REQ_DRV
>  
>  /*
>   * context holding the current state of a multi-part conversion
> @@ -125,7 +126,8 @@ struct iv_tcw_private {
>   * and encrypts / decrypts at the same time.
>   */
>  enum flags { DM_CRYPT_SUSPENDED, DM_CRYPT_KEY_VALID,
> -  DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD };
> +  DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD,
> +  DM_CRYPT_INLINE_ENCRYPT };
>  
>  enum cipher_flags {
>   CRYPT_MODE_INTEGRITY_AEAD,  /* Use authenticated mode for cihper */
> @@ -215,6 +217,10 @@ struct crypt_config {
>  
>   u8 *authenc_key; /* space for keys in authenc() format (if used) */
>   u8 key[0];
> +#ifdef CONFIG_BLK_DEV_INLINE_ENCRYPTION
> + void *ie_private; /* crypto context for inline enc drivers */
> + int key_cfg_idx;  /* key configuration index for inline enc */
> +#endif
>  };
>  
>  #define MIN_IOS  64
> @@ -1470,6 +1476,20 @@ static void crypt_io_init(struct dm_crypt_io *io, 
> struct crypt_config *cc,
>   atomic_set(>io_pending, 0);
>  }
>  
> +#ifdef CONFIG_BLK_DEV_INLINE_ENCRYPTION
> +static void crypt_inline_encrypt_submit(struct crypt_config *cc,
> + struct dm_target *ti, struct bio *bio)
> +{
> + bio_set_dev(bio, cc->dev->bdev);
> + if (bio_sectors(bio))
> + bio->bi_iter.bi_sector = cc->start +
> + dm_target_offset(ti, bio->bi_iter.bi_sector);
> + bio->bi_opf |= REQ_INLINE_ENCRYPTION;
> + bio->bi_ie_private = cc->ie_private;
> + generic_make_request(bio);
> +}
> +#endif
> +
>  static void crypt_inc_pending(struct dm_crypt_io *io)
>  {
>   atomic_inc(>io_pending);
> @@ -1960,6 +1980,9 @@ static int crypt_setkey(struct crypt_config *cc)
>  
>   /* Ignore extra keys (which are used for IV etc) */
>   subkey_size = crypt_subkey_size(cc);
> +#ifdef CONFIG_BLK_DEV_INLINE_ENCRYPTION
> + cc->key_cfg_idx = -1;
> +#endif
>  
>   if (crypt_integrity_hmac(cc)) {
>   if (subkey_size < cc->key_mac_size)
> @@ -1978,10 +2001,19 @@ static int crypt_setkey(struct crypt_config *cc)
>   r = crypto_aead_setkey(cc->cipher_tfm.tfms_aead[i],
>  cc->key + (i * subkey_size),
>  subkey_size);
> - else
> + else {
>   r = crypto_skcipher_setkey(cc->cipher_tfm.tfms[i],
>  cc->key + (i * subkey_size),
>  subkey_size);
> +#ifdef CONFIG_BLK_DEV_INLINE_ENCRYPTION
> + if (r > 0) {
> + cc->key_cfg_idx = r;
> + cc->ie_private = cc->cipher_tfm.tfms[i];
> + r = 0;

Re: [PATCH] md: dm-crypt: Add Inline Encryption support for dmcrypt

[Milan Broz]

On 05/28/2018 03:01 PM, Ladvine D Almeida wrote:
> 
> This patch adds new option
> -- perform_inline_encrypt
> that set the option inside dmcrypt to use inline encryption for
> the configured mapping. I want to introduce inline encryption support
> in the UFS Host Controller driver. The usage of the same for accomplishing
> the Full Disk Encryption is done by the following changes in the
> dmcrypt subsystem:
> - New function crypt_inline_encrypt_submit() is added to the
> dmcrypt which associate the crypto context to the bios which
> are submitted by the subsystem.
> - Successful configuration for the inline encryption will result in
> the crypto transformation job being bypassed in the dmcrypt layer and the
> actual encryption happens inline to the block device.

I am not sure this is a good idea. Dm-crypt should never ever forward
plaintext sector to underlying device.

And you do not even check that your hw is available in the underlying device!

If you want to use dm-crypt, write a crypto API driver for your crypto hw that 
defines
specific cipher and let dm-crypt use this cipher (no patches needed in this 
case!)

If I read the patch correctly, you do not check any parameters for
compatibility with your hw support (cipher, mode, IV algorithm, key length, 
sector size ...)

It seems like if the "perform_inline_encrypt" option is present, you just submit
the whole bio to your code with new INLINE_ENCRYPTION bit set.

What happens, if the cipher in table is different from what your hw is doing?

Milan

> Another patch set is sent to the block layer community for
> CONFIG_BLK_DEV_INLINE_ENCRYPTION config, which enables changes in the
> block layer for adding the bi_ie_private variable to the bio structure.
> 
> Signed-off-by: Ladvine D Almeida 
> ---
>  drivers/md/dm-crypt.c | 55 
> +--
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 44ff473..a9ed567 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -39,6 +39,7 @@
>  #include 
>  
>  #define DM_MSG_PREFIX "crypt"
> +#define REQ_INLINE_ENCRYPTION REQ_DRV
>  
>  /*
>   * context holding the current state of a multi-part conversion
> @@ -125,7 +126,8 @@ struct iv_tcw_private {
>   * and encrypts / decrypts at the same time.
>   */
>  enum flags { DM_CRYPT_SUSPENDED, DM_CRYPT_KEY_VALID,
> -  DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD };
> +  DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD,
> +  DM_CRYPT_INLINE_ENCRYPT };
>  
>  enum cipher_flags {
>   CRYPT_MODE_INTEGRITY_AEAD,  /* Use authenticated mode for cihper */
> @@ -215,6 +217,10 @@ struct crypt_config {
>  
>   u8 *authenc_key; /* space for keys in authenc() format (if used) */
>   u8 key[0];
> +#ifdef CONFIG_BLK_DEV_INLINE_ENCRYPTION
> + void *ie_private; /* crypto context for inline enc drivers */
> + int key_cfg_idx;  /* key configuration index for inline enc */
> +#endif
>  };
>  
>  #define MIN_IOS  64
> @@ -1470,6 +1476,20 @@ static void crypt_io_init(struct dm_crypt_io *io, 
> struct crypt_config *cc,
>   atomic_set(>io_pending, 0);
>  }
>  
> +#ifdef CONFIG_BLK_DEV_INLINE_ENCRYPTION
> +static void crypt_inline_encrypt_submit(struct crypt_config *cc,
> + struct dm_target *ti, struct bio *bio)
> +{
> + bio_set_dev(bio, cc->dev->bdev);
> + if (bio_sectors(bio))
> + bio->bi_iter.bi_sector = cc->start +
> + dm_target_offset(ti, bio->bi_iter.bi_sector);
> + bio->bi_opf |= REQ_INLINE_ENCRYPTION;
> + bio->bi_ie_private = cc->ie_private;
> + generic_make_request(bio);
> +}
> +#endif
> +
>  static void crypt_inc_pending(struct dm_crypt_io *io)
>  {
>   atomic_inc(>io_pending);
> @@ -1960,6 +1980,9 @@ static int crypt_setkey(struct crypt_config *cc)
>  
>   /* Ignore extra keys (which are used for IV etc) */
>   subkey_size = crypt_subkey_size(cc);
> +#ifdef CONFIG_BLK_DEV_INLINE_ENCRYPTION
> + cc->key_cfg_idx = -1;
> +#endif
>  
>   if (crypt_integrity_hmac(cc)) {
>   if (subkey_size < cc->key_mac_size)
> @@ -1978,10 +2001,19 @@ static int crypt_setkey(struct crypt_config *cc)
>   r = crypto_aead_setkey(cc->cipher_tfm.tfms_aead[i],
>  cc->key + (i * subkey_size),
>  subkey_size);
> - else
> + else {
>   r = crypto_skcipher_setkey(cc->cipher_tfm.tfms[i],
>  cc->key + (i * subkey_size),
>  subkey_size);
> +#ifdef CONFIG_BLK_DEV_INLINE_ENCRYPTION
> + if (r > 0) {
> + cc->key_cfg_idx = r;
> + cc->ie_private = cc->cipher_tfm.tfms[i];
> + r = 0;
> +   

Re: [PATCH 2/2] crypto: ccree: enable support for hardware keys

[Milan Broz]

On 03/31/2018 07:30 PM, Gilad Ben-Yossef wrote:
...
>> Are there other crypto drivers doing this?
> 
> I thought the exact same thing until I ran into a presentation about the s390
> secure keys implementation. I basically imitated their use (or abuse?)
> of the Crypto API
> assuming it is the way to go.
> 
> Take a look at arch/s390/crypto/paes_s390.c
> 
> The slide for the presentation describing this is here:
> http://schd.ws/hosted_files/ossna2017/89/LC2017SecKeyDmCryptV5.pdf
> 
> And they seem to even have support for it in the DM-Crypt tools, which at
> the time they claimed to be in the process of getting it up-streamed.

It is "in the process", but definitely not accepted.

We are just discussing how to integrate paes wrapped keys in cryptsetup and
it will definitely not be the way presented in the slides above.

If you plan more such ciphers, I would welcome some unified way in crypto API
how to handle these HSM keys flavors.

For kernel dm-crypt, there is no change needed (dmcrypt just treats it as a 
normal cipher key).
(I would say that it is not the best idea either, IMHO it would be better to use
kernel keyring reference instead and somehow handle hw keys through keyring.)

Milan

Re: [PATCH 2/2] crypto: ccree: enable support for hardware keys

[Milan Broz]

On 03/31/2018 07:30 PM, Gilad Ben-Yossef wrote:
...
>> Are there other crypto drivers doing this?
> 
> I thought the exact same thing until I ran into a presentation about the s390
> secure keys implementation. I basically imitated their use (or abuse?)
> of the Crypto API
> assuming it is the way to go.
> 
> Take a look at arch/s390/crypto/paes_s390.c
> 
> The slide for the presentation describing this is here:
> http://schd.ws/hosted_files/ossna2017/89/LC2017SecKeyDmCryptV5.pdf
> 
> And they seem to even have support for it in the DM-Crypt tools, which at
> the time they claimed to be in the process of getting it up-streamed.

It is "in the process", but definitely not accepted.

We are just discussing how to integrate paes wrapped keys in cryptsetup and
it will definitely not be the way presented in the slides above.

If you plan more such ciphers, I would welcome some unified way in crypto API
how to handle these HSM keys flavors.

For kernel dm-crypt, there is no change needed (dmcrypt just treats it as a 
normal cipher key).
(I would say that it is not the best idea either, IMHO it would be better to use
kernel keyring reference instead and somehow handle hw keys through keyring.)

Milan

Re: [dm-devel] [PATCH 2/2] md: dm-verity: allow parallel processing of bio blocks

[Milan Broz]

Mike and others,

did anyone even try to run veritysetup tests?

We have verity-compat-test in our testsuite, is has even basic FEC tests 
included.

We just added userspace verification of FEC RS codes to compare if kernel 
behaves the same.

I tried to apply three last dm-verity patches from your tree to Linus mainline.

It does even pass the *first* line of the test script and blocks the kernel 
forever...
(Running on 32bit Intel VM.)

*NACK* to the last two dm-verity patches.

(The "validate hashes once" is ok, despite I really do not like this 
approach...)

And comments from Eric are very valid as well, I think all this need to be fixed
before it can go to mainline.

Thanks,
Milan

On 03/27/2018 08:55 AM, Eric Biggers wrote:
> [+Cc linux-crypto]
> 
> Hi Yael,
> 
> On Sun, Mar 25, 2018 at 07:41:30PM +0100, Yael Chemla wrote:
>>  Allow parallel processing of bio blocks by moving to async. completion
>>  handling. This allows for better resource utilization of both HW and
>>  software based hash tfm and therefore better performance in many cases,
>>  depending on the specific tfm in use.
>>  
>>  Tested on ARM32 (zynq board) and ARM64 (Juno board).
>>  Time of cat command was measured on a filesystem with various file sizes.
>>  12% performance improvement when HW based hash was used (ccree driver).
>>  SW based hash showed less than 1% improvement.
>>  CPU utilization when HW based hash was used presented 10% less context
>>  switch, 4% less cycles and 7% less instructions. No difference in
>>  CPU utilization noticed with SW based hash.
>>  
>> Signed-off-by: Yael Chemla 
> 
> Okay, I definitely would like to see dm-verity better support hardware crypto
> accelerators, but these patches were painful to read.
> 
> There are lots of smaller bugs, but the high-level problem which you need to
> address first is that on every bio you are always allocating all the extra
> memory to hold a hash request and scatterlist for every data block.  This will
> not only hurt performance when the hashing is done in software (I'm skeptical
> that your performance numbers are representative of that case), but it will 
> also
> fall apart under memory pressure.  We are trying to get low-end Android 
> devices
> to start using dm-verity, and such devices often have only 1 GB or even only 
> 512
> MB of RAM, so memory allocations are at increased risk of failing.  In fact 
> I'm
> pretty sure you didn't do any proper stress testing of these patches, since 
> the
> first thing they do for every bio is try to allocate a physically contiguous
> array that is nearly as long as the full bio data itself (n_blocks *
> sizeof(struct dm_verity_req_data) = n_blocks * 3264, at least on a 64-bit
> platform, mostly due to the 'struct dm_verity_fec_io'), so potentially up to
> about 1 MB; that's going to fail a lot even on systems with gigabytes of 
> RAM...
> 
> (You also need to verify that your new code is compatible with the forward 
> error
> correction feature, with the "ignore_zero_blocks" option, and with the new
> "check_at_most_once" option.  From my reading of the code, all of those seemed
> broken; the dm_verity_fec_io structures, for example, weren't even being
> initialized...)
> 
> I think you need to take a close look at how dm-crypt handles async crypto
> implementations, since it seems to do it properly without hurting the common
> case where the crypto happens synchronously.  What it does, is it reserves 
> space
> in the per-bio data for a single cipher request.  Then, *only* if the cipher
> implementation actually processes the request asynchronously (as indicated by
> -EINPROGRESS being returned) is a new cipher request allocated dynamically,
> using a mempool (not kmalloc, which is prone to fail).  Note that unlike your
> patches it also properly handles the case where the hardware crypto queue is
> full, as indicated by the cipher implementation returning -EBUSY; in that 
> case,
> dm-crypt waits to start another request until there is space in the queue.
> 
> I think it would be possible to adapt dm-crypt's solution to dm-verity.
> 
> Thanks,
> 
> Eric

Re: [dm-devel] [PATCH 2/2] md: dm-verity: allow parallel processing of bio blocks

[Milan Broz]

Mike and others,

did anyone even try to run veritysetup tests?

We have verity-compat-test in our testsuite, is has even basic FEC tests 
included.

We just added userspace verification of FEC RS codes to compare if kernel 
behaves the same.

I tried to apply three last dm-verity patches from your tree to Linus mainline.

It does even pass the *first* line of the test script and blocks the kernel 
forever...
(Running on 32bit Intel VM.)

*NACK* to the last two dm-verity patches.

(The "validate hashes once" is ok, despite I really do not like this 
approach...)

And comments from Eric are very valid as well, I think all this need to be fixed
before it can go to mainline.

Thanks,
Milan

On 03/27/2018 08:55 AM, Eric Biggers wrote:
> [+Cc linux-crypto]
> 
> Hi Yael,
> 
> On Sun, Mar 25, 2018 at 07:41:30PM +0100, Yael Chemla wrote:
>>  Allow parallel processing of bio blocks by moving to async. completion
>>  handling. This allows for better resource utilization of both HW and
>>  software based hash tfm and therefore better performance in many cases,
>>  depending on the specific tfm in use.
>>  
>>  Tested on ARM32 (zynq board) and ARM64 (Juno board).
>>  Time of cat command was measured on a filesystem with various file sizes.
>>  12% performance improvement when HW based hash was used (ccree driver).
>>  SW based hash showed less than 1% improvement.
>>  CPU utilization when HW based hash was used presented 10% less context
>>  switch, 4% less cycles and 7% less instructions. No difference in
>>  CPU utilization noticed with SW based hash.
>>  
>> Signed-off-by: Yael Chemla 
> 
> Okay, I definitely would like to see dm-verity better support hardware crypto
> accelerators, but these patches were painful to read.
> 
> There are lots of smaller bugs, but the high-level problem which you need to
> address first is that on every bio you are always allocating all the extra
> memory to hold a hash request and scatterlist for every data block.  This will
> not only hurt performance when the hashing is done in software (I'm skeptical
> that your performance numbers are representative of that case), but it will 
> also
> fall apart under memory pressure.  We are trying to get low-end Android 
> devices
> to start using dm-verity, and such devices often have only 1 GB or even only 
> 512
> MB of RAM, so memory allocations are at increased risk of failing.  In fact 
> I'm
> pretty sure you didn't do any proper stress testing of these patches, since 
> the
> first thing they do for every bio is try to allocate a physically contiguous
> array that is nearly as long as the full bio data itself (n_blocks *
> sizeof(struct dm_verity_req_data) = n_blocks * 3264, at least on a 64-bit
> platform, mostly due to the 'struct dm_verity_fec_io'), so potentially up to
> about 1 MB; that's going to fail a lot even on systems with gigabytes of 
> RAM...
> 
> (You also need to verify that your new code is compatible with the forward 
> error
> correction feature, with the "ignore_zero_blocks" option, and with the new
> "check_at_most_once" option.  From my reading of the code, all of those seemed
> broken; the dm_verity_fec_io structures, for example, weren't even being
> initialized...)
> 
> I think you need to take a close look at how dm-crypt handles async crypto
> implementations, since it seems to do it properly without hurting the common
> case where the crypto happens synchronously.  What it does, is it reserves 
> space
> in the per-bio data for a single cipher request.  Then, *only* if the cipher
> implementation actually processes the request asynchronously (as indicated by
> -EINPROGRESS being returned) is a new cipher request allocated dynamically,
> using a mempool (not kmalloc, which is prone to fail).  Note that unlike your
> patches it also properly handles the case where the hardware crypto queue is
> full, as indicated by the cipher implementation returning -EBUSY; in that 
> case,
> dm-crypt waits to start another request until there is space in the queue.
> 
> I think it would be possible to adapt dm-crypt's solution to dm-verity.
> 
> Thanks,
> 
> Eric

Re: [PATCH] Add an option to dm-verity to validate hashes at most once

[Milan Broz]

On 03/07/2018 12:14 AM, Patrik Torstensson wrote:
> Add an option to dm-verity to validate hashes at most once
> to allow platforms that is CPU/memory contraint to be
> protected by dm-verity against offline attacks.
> 
> The option introduces a bitset that is used to check if
> a block has been validated before or not. A block can
> be validated more than once as there is no thread protection
> for the bitset.

Hi,

what happens, if a block is read, validated, marked in bitset and 

1) something changes in the underlying device (data corruption, FTL hiccup,
intentional remapping to different device-mapper device through table change)

2) user flushes all page caches

3) the same block is read again.

Does it read and use unverified block from the corrupted device in this case?

(In general, just reading the whole device means de-facto verification 
deactivation.)

If so, your thread model assumes that you cannot attack underlying storage while
it is in operation, is it the correct assumption?

Milan

> 
> This patch has been developed and tested on entry-level
> Android Go devices.
> 
> Signed-off-by: Patrik Torstensson 
> ---
>  drivers/md/dm-verity-target.c | 58 +--
>  drivers/md/dm-verity.h|  1 +
>  2 files changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index aedb8222836b..479d0af212bf 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -32,6 +32,7 @@
>  #define DM_VERITY_OPT_LOGGING"ignore_corruption"
>  #define DM_VERITY_OPT_RESTART"restart_on_corruption"
>  #define DM_VERITY_OPT_IGN_ZEROES "ignore_zero_blocks"
> +#define DM_VERITY_OPT_AT_MOST_ONCE   "check_at_most_once"
>  
>  #define DM_VERITY_OPTS_MAX   (2 + DM_VERITY_OPTS_FEC)
>  
> @@ -432,6 +433,19 @@ static int verity_bv_zero(struct dm_verity *v, struct 
> dm_verity_io *io,
>   return 0;
>  }
>  
> +/*
> + * Moves the bio iter one data block forward.
> + */
> +static inline void verity_bv_skip_block(struct dm_verity *v,
> + struct dm_verity_io *io,
> + struct bvec_iter *iter)
> +{
> + struct bio *bio = dm_bio_from_per_bio_data(io,
> +v->ti->per_io_data_size);
> +
> + bio_advance_iter(bio, iter, 1 << v->data_dev_block_bits);
> +}
> +
>  /*
>   * Verify one "dm_verity_io" structure.
>   */
> @@ -445,8 +459,15 @@ static int verity_verify_io(struct dm_verity_io *io)
>  
>   for (b = 0; b < io->n_blocks; b++) {
>   int r;
> + sector_t cur_block = io->block + b;
>   struct ahash_request *req = verity_io_hash_req(v, io);
>  
> + if (v->validated_blocks &&
> + likely(test_bit(cur_block, v->validated_blocks))) {
> + verity_bv_skip_block(v, io, >iter);
> + continue;
> + }
> +
>   r = verity_hash_for_block(v, io, io->block + b,
> verity_io_want_digest(v, io),
> _zero);
> @@ -481,13 +502,17 @@ static int verity_verify_io(struct dm_verity_io *io)
>   return r;
>  
>   if (likely(memcmp(verity_io_real_digest(v, io),
> -   verity_io_want_digest(v, io), v->digest_size) 
> == 0))
> +   verity_io_want_digest(v, io),
> +   v->digest_size) == 0)) {
> + if (v->validated_blocks)
> + set_bit(cur_block, v->validated_blocks);
>   continue;
> + }
>   else if (verity_fec_decode(v, io, DM_VERITY_BLOCK_TYPE_DATA,
> -io->block + b, NULL, ) == 0)
> +cur_block, NULL, ) == 0)
>   continue;
>   else if (verity_handle_err(v, DM_VERITY_BLOCK_TYPE_DATA,
> -io->block + b))
> +cur_block))
>   return -EIO;
>   }
>  
> @@ -740,6 +765,7 @@ static void verity_dtr(struct dm_target *ti)
>   if (v->bufio)
>   dm_bufio_client_destroy(v->bufio);
>  
> + kvfree(v->validated_blocks);
>   kfree(v->salt);
>   kfree(v->root_digest);
>   kfree(v->zero_digest);
> @@ -760,6 +786,26 @@ static void verity_dtr(struct dm_target *ti)
>   kfree(v);
>  }
>  
> +static int verity_alloc_most_once(struct dm_verity *v)
> +{
> + struct dm_target *ti = v->ti;
> +
> + /* the bitset can only handle INT_MAX blocks */
> + if (v->data_blocks > INT_MAX) {
> + ti->error = "device too large to use check_at_most_once";
> + return -E2BIG;
> + }
> +
> + v->validated_blocks = 

Re: [PATCH] Add an option to dm-verity to validate hashes at most once

[Milan Broz]

On 03/07/2018 12:14 AM, Patrik Torstensson wrote:
> Add an option to dm-verity to validate hashes at most once
> to allow platforms that is CPU/memory contraint to be
> protected by dm-verity against offline attacks.
> 
> The option introduces a bitset that is used to check if
> a block has been validated before or not. A block can
> be validated more than once as there is no thread protection
> for the bitset.

Hi,

what happens, if a block is read, validated, marked in bitset and 

1) something changes in the underlying device (data corruption, FTL hiccup,
intentional remapping to different device-mapper device through table change)

2) user flushes all page caches

3) the same block is read again.

Does it read and use unverified block from the corrupted device in this case?

(In general, just reading the whole device means de-facto verification 
deactivation.)

If so, your thread model assumes that you cannot attack underlying storage while
it is in operation, is it the correct assumption?

Milan

> 
> This patch has been developed and tested on entry-level
> Android Go devices.
> 
> Signed-off-by: Patrik Torstensson 
> ---
>  drivers/md/dm-verity-target.c | 58 +--
>  drivers/md/dm-verity.h|  1 +
>  2 files changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index aedb8222836b..479d0af212bf 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -32,6 +32,7 @@
>  #define DM_VERITY_OPT_LOGGING"ignore_corruption"
>  #define DM_VERITY_OPT_RESTART"restart_on_corruption"
>  #define DM_VERITY_OPT_IGN_ZEROES "ignore_zero_blocks"
> +#define DM_VERITY_OPT_AT_MOST_ONCE   "check_at_most_once"
>  
>  #define DM_VERITY_OPTS_MAX   (2 + DM_VERITY_OPTS_FEC)
>  
> @@ -432,6 +433,19 @@ static int verity_bv_zero(struct dm_verity *v, struct 
> dm_verity_io *io,
>   return 0;
>  }
>  
> +/*
> + * Moves the bio iter one data block forward.
> + */
> +static inline void verity_bv_skip_block(struct dm_verity *v,
> + struct dm_verity_io *io,
> + struct bvec_iter *iter)
> +{
> + struct bio *bio = dm_bio_from_per_bio_data(io,
> +v->ti->per_io_data_size);
> +
> + bio_advance_iter(bio, iter, 1 << v->data_dev_block_bits);
> +}
> +
>  /*
>   * Verify one "dm_verity_io" structure.
>   */
> @@ -445,8 +459,15 @@ static int verity_verify_io(struct dm_verity_io *io)
>  
>   for (b = 0; b < io->n_blocks; b++) {
>   int r;
> + sector_t cur_block = io->block + b;
>   struct ahash_request *req = verity_io_hash_req(v, io);
>  
> + if (v->validated_blocks &&
> + likely(test_bit(cur_block, v->validated_blocks))) {
> + verity_bv_skip_block(v, io, >iter);
> + continue;
> + }
> +
>   r = verity_hash_for_block(v, io, io->block + b,
> verity_io_want_digest(v, io),
> _zero);
> @@ -481,13 +502,17 @@ static int verity_verify_io(struct dm_verity_io *io)
>   return r;
>  
>   if (likely(memcmp(verity_io_real_digest(v, io),
> -   verity_io_want_digest(v, io), v->digest_size) 
> == 0))
> +   verity_io_want_digest(v, io),
> +   v->digest_size) == 0)) {
> + if (v->validated_blocks)
> + set_bit(cur_block, v->validated_blocks);
>   continue;
> + }
>   else if (verity_fec_decode(v, io, DM_VERITY_BLOCK_TYPE_DATA,
> -io->block + b, NULL, ) == 0)
> +cur_block, NULL, ) == 0)
>   continue;
>   else if (verity_handle_err(v, DM_VERITY_BLOCK_TYPE_DATA,
> -io->block + b))
> +cur_block))
>   return -EIO;
>   }
>  
> @@ -740,6 +765,7 @@ static void verity_dtr(struct dm_target *ti)
>   if (v->bufio)
>   dm_bufio_client_destroy(v->bufio);
>  
> + kvfree(v->validated_blocks);
>   kfree(v->salt);
>   kfree(v->root_digest);
>   kfree(v->zero_digest);
> @@ -760,6 +786,26 @@ static void verity_dtr(struct dm_target *ti)
>   kfree(v);
>  }
>  
> +static int verity_alloc_most_once(struct dm_verity *v)
> +{
> + struct dm_target *ti = v->ti;
> +
> + /* the bitset can only handle INT_MAX blocks */
> + if (v->data_blocks > INT_MAX) {
> + ti->error = "device too large to use check_at_most_once";
> + return -E2BIG;
> + }
> +
> + v->validated_blocks = 

Re: [dm-devel] DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[Milan Broz]

On 02/15/2018 01:07 AM, NeilBrown wrote:
> 
> And looking again at the code, it doesn't seem so bad.  I was worried
> about reassigning ci.io->orig_bio after calling
> __split_and_process_non_flush(), but the important thing is to set it
> before the last dec_pending() call, and the code gets that right.
> 
> I think I've found the problem though:
> 
>   /* done with normal IO or empty flush */
>   bio->bi_status = io_error;
> 
> When we have chained bios, there are multiple sources for error which
> need to be merged into bi_status:  all bios that were chained to this
> one, and this bio itself.
> 
> __bio_chain_endio uses:
> 
>   if (!parent->bi_status)
>   parent->bi_status = bio->bi_status;
> 
> so the new status won't over-ride the old status (unless there are
> races), but dm doesn't do that - I guess it didn't have to worry
> about chains before.
> 
> So this:
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index d6de00f367ef..68136806d365 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -903,7 +903,8 @@ static void dec_pending(struct dm_io *io, blk_status_t 
> error)
>   queue_io(md, bio);
>   } else {
>   /* done with normal IO or empty flush */
> - bio->bi_status = io_error;
> + if (io_error)
> + bio->bi_status = io_error;
>   bio_endio(bio);
>   }
>   }
> 

Hi,

well, it indeed fixes the reported problem, thanks.
But I just tested the first (dm.c) change above.

The important part is also that if the error is hits bio later, it will produce
short read (and not fail of the whole operation), this can be easily tested if 
we switch
the error and the zero target in that reproducer
  //system("echo -e \'0 1000 error\'n\'1000 100 zero\' | 
dmsetup create test");
  system("echo -e \'0 1000 zero\'n\'1000 100 error\' | dmsetup 
create test");

So it seems your patch works.

I am not sure about this part though:

> should fix it.  And __bio_chain_endio should probably be
> 
> diff --git a/block/bio.c b/block/bio.c
> index e1708db48258..ad77140edc6f 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -312,7 +312,7 @@ static struct bio *__bio_chain_endio(struct bio *bio)
>  {
>   struct bio *parent = bio->bi_private;
>  
> - if (!parent->bi_status)
> + if (bio->bi_status)
>   parent->bi_status = bio->bi_status;

Shouldn't it be
if (!parent->bi_status && bio->bi_status)
parent->bi_status = bio->bi_status;
?

Maybe one rephrased question: what about priorities for errors (bio statuses)?

For example, part of a bio fails with EILSEQ (data integrity check error, 
BLK_STS_PROTECTION),
part with EIO (media error, BLK_STS_IOERR). What should be reported for parent 
bio?
(I would expect I always get EIO here because this is hard, uncorrectable 
error.)

Anyway, thanks for the fix!

Milan


>   bio_put(bio);
>   return parent;
> 
> to remove the chance that a race will hide a real error.
> 
> Milan: could you please check that the patch fixes the dm-crypt bug?
> I've confirmed that it fixes your test case.
> When you confirm, I'll send a proper patch.
> 
> I guess I should audit all code that assigns to ->bi_status and make
> sure nothing ever assigns 0 to it.
> 
> Thanks,
> NeilBrown
> 

Re: [dm-devel] DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[Milan Broz]

On 02/15/2018 01:07 AM, NeilBrown wrote:
> 
> And looking again at the code, it doesn't seem so bad.  I was worried
> about reassigning ci.io->orig_bio after calling
> __split_and_process_non_flush(), but the important thing is to set it
> before the last dec_pending() call, and the code gets that right.
> 
> I think I've found the problem though:
> 
>   /* done with normal IO or empty flush */
>   bio->bi_status = io_error;
> 
> When we have chained bios, there are multiple sources for error which
> need to be merged into bi_status:  all bios that were chained to this
> one, and this bio itself.
> 
> __bio_chain_endio uses:
> 
>   if (!parent->bi_status)
>   parent->bi_status = bio->bi_status;
> 
> so the new status won't over-ride the old status (unless there are
> races), but dm doesn't do that - I guess it didn't have to worry
> about chains before.
> 
> So this:
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index d6de00f367ef..68136806d365 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -903,7 +903,8 @@ static void dec_pending(struct dm_io *io, blk_status_t 
> error)
>   queue_io(md, bio);
>   } else {
>   /* done with normal IO or empty flush */
> - bio->bi_status = io_error;
> + if (io_error)
> + bio->bi_status = io_error;
>   bio_endio(bio);
>   }
>   }
> 

Hi,

well, it indeed fixes the reported problem, thanks.
But I just tested the first (dm.c) change above.

The important part is also that if the error is hits bio later, it will produce
short read (and not fail of the whole operation), this can be easily tested if 
we switch
the error and the zero target in that reproducer
  //system("echo -e \'0 1000 error\'n\'1000 100 zero\' | 
dmsetup create test");
  system("echo -e \'0 1000 zero\'n\'1000 100 error\' | dmsetup 
create test");

So it seems your patch works.

I am not sure about this part though:

> should fix it.  And __bio_chain_endio should probably be
> 
> diff --git a/block/bio.c b/block/bio.c
> index e1708db48258..ad77140edc6f 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -312,7 +312,7 @@ static struct bio *__bio_chain_endio(struct bio *bio)
>  {
>   struct bio *parent = bio->bi_private;
>  
> - if (!parent->bi_status)
> + if (bio->bi_status)
>   parent->bi_status = bio->bi_status;

Shouldn't it be
if (!parent->bi_status && bio->bi_status)
parent->bi_status = bio->bi_status;
?

Maybe one rephrased question: what about priorities for errors (bio statuses)?

For example, part of a bio fails with EILSEQ (data integrity check error, 
BLK_STS_PROTECTION),
part with EIO (media error, BLK_STS_IOERR). What should be reported for parent 
bio?
(I would expect I always get EIO here because this is hard, uncorrectable 
error.)

Anyway, thanks for the fix!

Milan


>   bio_put(bio);
>   return parent;
> 
> to remove the chance that a race will hide a real error.
> 
> Milan: could you please check that the patch fixes the dm-crypt bug?
> I've confirmed that it fixes your test case.
> When you confirm, I'll send a proper patch.
> 
> I guess I should audit all code that assigns to ->bi_status and make
> sure nothing ever assigns 0 to it.
> 
> Thanks,
> NeilBrown
> 

DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[Milan Broz]

Hi,

the commit (found by bisect)

  commit 18a25da84354c6bb655320de6072c00eda6eb602
  Author: NeilBrown 
  Date:   Wed Sep 6 09:43:28 2017 +1000

dm: ensure bio submission follows a depth-first tree walk

cause serious regression while reading from DM device.

The reproducer is below, basically it tries to simulate failure we see in 
cryptsetup
regression test: we have DM device with error and zero target and try to read
"passphrase" from it (it is test for 64 bit offset error path):

Test device:
# dmsetup table test
0 1000 error 
1000 100 zero 

We try to run this operation:
  lseek64(fd, 511988, SEEK_CUR); // this should seek to error target sector
  read(fd, buf, 13); // this should fail, if we seek to error part of the device

While on 4.15 the read properly fails:
  Seek returned 511988.
  Read returned -1.

for 4.16 it actually succeeds returning some random data
(perhaps kernel memory, so this bug is even more dangerous):
  Seek returned 511988.
  Read returned 13.

Full reproducer below:

#define _GNU_SOURCE
#define _LARGEFILE64_SOURCE
#include 
#include 
#include 
#include 
#include 
#include 
#include 

int main (int argc, char *argv[])
{
char buf[13];
int fd;
//uint64_t offset64 = 511999;
uint64_t offset64 =   511988;
off64_t r;
ssize_t bytes;

system("echo -e \'0 1000 error\'n\'1000 100 zero\' | 
dmsetup create test");

fd = open("/dev/mapper/test", O_RDONLY);
if (fd == -1) {
printf("open fail\n");
return 1;
}

r = lseek64(fd, offset64, SEEK_CUR);
printf("Seek returned %" PRIu64 ".\n", r);
if (r < 0) {
printf("seek fail\n");
close(fd);
return 2;
}

bytes = read(fd, buf, 13);
printf("Read returned %d.\n", (int)bytes);

close(fd);
return 0;
}


Please let me know if you need more info to reproduce it.

Milan

DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[Milan Broz]

Hi,

the commit (found by bisect)

  commit 18a25da84354c6bb655320de6072c00eda6eb602
  Author: NeilBrown 
  Date:   Wed Sep 6 09:43:28 2017 +1000

dm: ensure bio submission follows a depth-first tree walk

cause serious regression while reading from DM device.

The reproducer is below, basically it tries to simulate failure we see in 
cryptsetup
regression test: we have DM device with error and zero target and try to read
"passphrase" from it (it is test for 64 bit offset error path):

Test device:
# dmsetup table test
0 1000 error 
1000 100 zero 

We try to run this operation:
  lseek64(fd, 511988, SEEK_CUR); // this should seek to error target sector
  read(fd, buf, 13); // this should fail, if we seek to error part of the device

While on 4.15 the read properly fails:
  Seek returned 511988.
  Read returned -1.

for 4.16 it actually succeeds returning some random data
(perhaps kernel memory, so this bug is even more dangerous):
  Seek returned 511988.
  Read returned 13.

Full reproducer below:

#define _GNU_SOURCE
#define _LARGEFILE64_SOURCE
#include 
#include 
#include 
#include 
#include 
#include 
#include 

int main (int argc, char *argv[])
{
char buf[13];
int fd;
//uint64_t offset64 = 511999;
uint64_t offset64 =   511988;
off64_t r;
ssize_t bytes;

system("echo -e \'0 1000 error\'n\'1000 100 zero\' | 
dmsetup create test");

fd = open("/dev/mapper/test", O_RDONLY);
if (fd == -1) {
printf("open fail\n");
return 1;
}

r = lseek64(fd, offset64, SEEK_CUR);
printf("Seek returned %" PRIu64 ".\n", r);
if (r < 0) {
printf("seek fail\n");
close(fd);
return 2;
}

bytes = read(fd, buf, 13);
printf("Read returned %d.\n", (int)bytes);

close(fd);
return 0;
}


Please let me know if you need more info to reproduce it.

Milan

Re: [PATCH] dm-crypt, dm-integrity: allow unaligned bv_offset

[Milan Broz]

On 11/07/2017 09:13 AM, Bruno Prémont wrote:
>> Signed-off-by: Mikulas Patocka <mpato...@redhat.com>
>> Cc: sta...@vger.kernel.org   # v4.12+
>> Fixes: 8f0009a22517 ("dm crypt: optionally support larger encryption sector 
>> size")
>> Fixes: 7eada909bfd7 ("dm: add integrity target")
> 
> Reported-by: Bruno Prémont <bonb...@sysophe.eu>
> Tested-by: Bruno Prémont <bonb...@sysophe.eu>
> 
> Successfully tested on 4.12.14, 4.13.11 and 4.14-rc8+ (e4880bc5dfb1).

Thanks!

(That alignment check was added after our discussion - we wanted
to have dm-crypt and dm-integrity to check the same thing, just to trigger
possible data corruption, but obviously it was incorrect check.)

It must go to stable well, I think there are possibly more situations
we broke for dm-crypt by this.

Reviewed-by: Milan Broz <gmazyl...@gmail.com>

m.

Re: [PATCH] dm-crypt, dm-integrity: allow unaligned bv_offset

[Milan Broz]

On 11/07/2017 09:13 AM, Bruno Prémont wrote:
>> Signed-off-by: Mikulas Patocka 
>> Cc: sta...@vger.kernel.org   # v4.12+
>> Fixes: 8f0009a22517 ("dm crypt: optionally support larger encryption sector 
>> size")
>> Fixes: 7eada909bfd7 ("dm: add integrity target")
> 
> Reported-by: Bruno Prémont 
> Tested-by: Bruno Prémont 
> 
> Successfully tested on 4.12.14, 4.13.11 and 4.14-rc8+ (e4880bc5dfb1).

Thanks!

(That alignment check was added after our discussion - we wanted
to have dm-crypt and dm-integrity to check the same thing, just to trigger
possible data corruption, but obviously it was incorrect check.)

It must go to stable well, I think there are possibly more situations
we broke for dm-crypt by this.

Reviewed-by: Milan Broz 

m.

Re: [BUG][next-20170523][Bisected cf22cd5f3a] kernel oops while running trinity fuzzer

[Milan Broz]

On 05/24/2017 11:29 AM, Abdul Haleem wrote:
> Hi
> 
> commit cf22cd5f3a: dm crypt: add cryptographic data integrity protection
> suspected to be bad.

Isn't this false positive? That commit changes only dm-crypt and that module
seems not to be even loaded...
(Moreover config has disabled block integrity so this code should
be mostly compiled out.)

Milan

> 
> Test : trinity
> Machine : Power 8 (LPAR)
> kernel : 4.12.0-rc2-next-20170523
> Config : attached
> 
> Unable to handle kernel paging request for data at address 0x
> Faulting instruction address: 0xc15d8824
> Oops: Kernel access of bad area, sig: 11 [#1]
> SMP NR_CPUS=2048 
> NUMA 
> pSeries
> Dumping ftrace buffer: 
>(ftrace buffer empty)
> Modules linked in: bridge nfnetlink rpadlpar_io stp llc rpaphp xt_tcpudp
> ipt_REJECT nf_reject_ipv4 xt_conntrack iptable_nat nf_conntrack_ipv4
> nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle
> iptable_filter vmx_crypto pseries_rng rng_core binfmt_misc nfsd
> ip_tables x_tables autofs4 [last unloaded: bridge]
> CPU: 4 PID: 2474 Comm: trinity-c4 Not tainted 
> 4.12.0-rc2-next-20170523-autotest #1
> task: c003a6ae2500 task.stack: c002bf7ec000
> NIP: c15d8824 LR: c1ef855c CTR: 
> REGS: c002bf7efb00 TRAP: 0300   Not tainted  
> (4.12.0-rc2-next-20170523-autotest)
> MSR: 80009033 
>   CR: 44002841  XER: 2000  
> CFAR: c15d8810 DAR:  DSISR: 4200 SOFTE: 1 
> GPR00: 0001 c002bf7efd80 c33d3600  
> GPR04:    c3743600 
> GPR08: c3743600 0006d123 c3773600 1000 
> GPR12: 2200 ce9c1600  10035e80 
> GPR16: 10033950  10033a50  
> GPR20: 10985cc4 7d9d 1001bae0 000185f8 
> GPR24:   fffa ffea 
> GPR28: 0008 c003a8c48850 ffea  
> NIP [c15d8824] memset+0xc4/0xfc
> LR [c1ef855c] memzero_explicit+0x3c/0x70
> Call Trace:
> [c002bf7efd80] [0008] 0x8 (unreliable)
> [c002bf7efdb0] [c1e3fb30] SyS_add_key+0x1a0/0x410
> [c002bf7efe30] [c152b7e0] system_call+0x38/0xfc
> Instruction dump:
> 41820038 409d0018 f886 f8860008 f8860010 f8860018 38c60020 409e0010 
> f886 f8860008 38c60010 409f000c  38c60008 2c05
> 7cb01120 
> ---[ end trace c454dcc1309b8479 ]---
> 
> 

Re: [BUG][next-20170523][Bisected cf22cd5f3a] kernel oops while running trinity fuzzer

[Milan Broz]

On 05/24/2017 11:29 AM, Abdul Haleem wrote:
> Hi
> 
> commit cf22cd5f3a: dm crypt: add cryptographic data integrity protection
> suspected to be bad.

Isn't this false positive? That commit changes only dm-crypt and that module
seems not to be even loaded...
(Moreover config has disabled block integrity so this code should
be mostly compiled out.)

Milan

> 
> Test : trinity
> Machine : Power 8 (LPAR)
> kernel : 4.12.0-rc2-next-20170523
> Config : attached
> 
> Unable to handle kernel paging request for data at address 0x
> Faulting instruction address: 0xc15d8824
> Oops: Kernel access of bad area, sig: 11 [#1]
> SMP NR_CPUS=2048 
> NUMA 
> pSeries
> Dumping ftrace buffer: 
>(ftrace buffer empty)
> Modules linked in: bridge nfnetlink rpadlpar_io stp llc rpaphp xt_tcpudp
> ipt_REJECT nf_reject_ipv4 xt_conntrack iptable_nat nf_conntrack_ipv4
> nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle
> iptable_filter vmx_crypto pseries_rng rng_core binfmt_misc nfsd
> ip_tables x_tables autofs4 [last unloaded: bridge]
> CPU: 4 PID: 2474 Comm: trinity-c4 Not tainted 
> 4.12.0-rc2-next-20170523-autotest #1
> task: c003a6ae2500 task.stack: c002bf7ec000
> NIP: c15d8824 LR: c1ef855c CTR: 
> REGS: c002bf7efb00 TRAP: 0300   Not tainted  
> (4.12.0-rc2-next-20170523-autotest)
> MSR: 80009033 
>   CR: 44002841  XER: 2000  
> CFAR: c15d8810 DAR:  DSISR: 4200 SOFTE: 1 
> GPR00: 0001 c002bf7efd80 c33d3600  
> GPR04:    c3743600 
> GPR08: c3743600 0006d123 c3773600 1000 
> GPR12: 2200 ce9c1600  10035e80 
> GPR16: 10033950  10033a50  
> GPR20: 10985cc4 7d9d 1001bae0 000185f8 
> GPR24:   fffa ffea 
> GPR28: 0008 c003a8c48850 ffea  
> NIP [c15d8824] memset+0xc4/0xfc
> LR [c1ef855c] memzero_explicit+0x3c/0x70
> Call Trace:
> [c002bf7efd80] [0008] 0x8 (unreliable)
> [c002bf7efdb0] [c1e3fb30] SyS_add_key+0x1a0/0x410
> [c002bf7efe30] [c152b7e0] system_call+0x38/0xfc
> Instruction dump:
> 41820038 409d0018 f886 f8860008 f8860010 f8860018 38c60020 409e0010 
> f886 f8860008 38c60010 409f000c  38c60008 2c05
> 7cb01120 
> ---[ end trace c454dcc1309b8479 ]---
> 
> 

Re: [PATCH] dm verity: fix no salt used use case

[Milan Broz]

On 05/18/2017 12:47 PM, Gilad Ben-Yossef wrote:
> DM-Verity has an (undocumented) mode where no salt is used.
> This was never handled directly by the DM-Verity code, instead working due
> to the fact that calling crypto_shash_update() with a zero length data is
> an implicit noop.
> 
> This is no longer the case now that we have switched to
> crypto_ahash_update(). Fix the issue by introducing an explicit handling
> of the no salt use case to DM-Verity.
> 
> Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
> Reported-by: Marian Csontos <mcson...@redhat.com>
> Fixes: d1ac3ff ("dm verity: switch to using asynchronous hash crypto API")
> CC: Milan Broz <gmazyl...@gmail.com>

Tested-by: Milan Broz <gmazyl...@gmail.com>

Thanks for quick fix!

Mike: this must go to 4.12rc (only).

m.


> ---
>  drivers/md/dm-verity-target.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 97de961..1ec9b2c 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -166,7 +166,7 @@ static int verity_hash_init(struct dm_verity *v, struct 
> ahash_request *req,
>   return r;
>   }
>  
> - if (likely(v->version >= 1))
> + if (likely(v->salt_size && (v->version >= 1)))
>   r = verity_hash_update(v, req, v->salt, v->salt_size, res);
>  
>   return r;
> @@ -177,7 +177,7 @@ static int verity_hash_final(struct dm_verity *v, struct 
> ahash_request *req,
>  {
>   int r;
>  
> - if (unlikely(!v->version)) {
> + if (unlikely(v->salt_size && (!v->version))) {
>   r = verity_hash_update(v, req, v->salt, v->salt_size, res);
>  
>   if (r < 0) {
> 

Re: [PATCH] dm verity: fix no salt used use case

[Milan Broz]

On 05/18/2017 12:47 PM, Gilad Ben-Yossef wrote:
> DM-Verity has an (undocumented) mode where no salt is used.
> This was never handled directly by the DM-Verity code, instead working due
> to the fact that calling crypto_shash_update() with a zero length data is
> an implicit noop.
> 
> This is no longer the case now that we have switched to
> crypto_ahash_update(). Fix the issue by introducing an explicit handling
> of the no salt use case to DM-Verity.
> 
> Signed-off-by: Gilad Ben-Yossef 
> Reported-by: Marian Csontos 
> Fixes: d1ac3ff ("dm verity: switch to using asynchronous hash crypto API")
> CC: Milan Broz 

Tested-by: Milan Broz 

Thanks for quick fix!

Mike: this must go to 4.12rc (only).

m.


> ---
>  drivers/md/dm-verity-target.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 97de961..1ec9b2c 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -166,7 +166,7 @@ static int verity_hash_init(struct dm_verity *v, struct 
> ahash_request *req,
>   return r;
>   }
>  
> - if (likely(v->version >= 1))
> + if (likely(v->salt_size && (v->version >= 1)))
>   r = verity_hash_update(v, req, v->salt, v->salt_size, res);
>  
>   return r;
> @@ -177,7 +177,7 @@ static int verity_hash_final(struct dm_verity *v, struct 
> ahash_request *req,
>  {
>   int r;
>  
> - if (unlikely(!v->version)) {
> + if (unlikely(v->salt_size && (!v->version))) {
>   r = verity_hash_update(v, req, v->salt, v->salt_size, res);
>  
>   if (r < 0) {
> 

[PATCH] dm-integrity: Add proper description of module to KConfig.

[Milan Broz]

Add more descriptive text to explain what it the dm-integrity
when it should be enabled.

Signed-off-by: Milan Broz <gmazyl...@gmail.com>
---
 drivers/md/Kconfig | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index 7468a22f9d10..3e96fccbbdb2 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -509,6 +509,20 @@ config DM_INTEGRITY
select CRYPTO
select ASYNC_XOR
---help---
-  This is the integrity target.
+ This device-mapper target emulates a block device that has additional
+ per-sector tags that can be used for storing integrity information.
+
+ The dm-integrity target is used with the dm-crypt target to provide
+ authenticated disk encryption or it can also be used as a standalone
+ target. In standalone mode it calculates and verifies the data 
integrity
+ internally.
+
+ You should enable this option if you plan to use dm-crypt target
+ in authenticated disk encryption mode.
+
+ To compile this code as a module, choose M here: the module will
+ be called dm-integrity.
+
+ If unsure, say N.
 
 endif # MD
-- 
2.11.0

[PATCH] dm-integrity: Add proper description of module to KConfig.

[Milan Broz]

Add more descriptive text to explain what it the dm-integrity
when it should be enabled.

Signed-off-by: Milan Broz 
---
 drivers/md/Kconfig | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index 7468a22f9d10..3e96fccbbdb2 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -509,6 +509,20 @@ config DM_INTEGRITY
select CRYPTO
select ASYNC_XOR
---help---
-  This is the integrity target.
+ This device-mapper target emulates a block device that has additional
+ per-sector tags that can be used for storing integrity information.
+
+ The dm-integrity target is used with the dm-crypt target to provide
+ authenticated disk encryption or it can also be used as a standalone
+ target. In standalone mode it calculates and verifies the data 
integrity
+ internally.
+
+ You should enable this option if you plan to use dm-crypt target
+ in authenticated disk encryption mode.
+
+ To compile this code as a module, choose M here: the module will
+ be called dm-integrity.
+
+ If unsure, say N.
 
 endif # MD
-- 
2.11.0

Re: DM_INTEGRITY Kconfig help (was: Re: dm: add integrity target)

[Milan Broz]

On 05/04/2017 08:03 AM, Geert Uytterhoeven wrote:
>> +config DM_INTEGRITY
>> +   tristate "Integrity target"
>> +   depends on BLK_DEV_DM
>> +   select BLK_DEV_INTEGRITY
>> +   select DM_BUFIO
>> +   select CRYPTO
>> +   select ASYNC_XOR
>> +   ---help---
>> +  This is the integrity target.
> 
> Which is...?
> 
> Can you please extend the help message for the uneducated?

Yes, sorry, this is our oversight.

Mike already promised that this text will be extended in the next patchset.

For now you can find the description in documentation
Documentation/device-mapper/dm-integrity.txt

"The dm-integrity target emulates a block device that has additional
per-sector tags that can be used for storing integrity information."

(The major designed use case is for providing metadata space for tags
for authenticated encryption in dm-crypt but it can be used also as
a standalone target.)

Thanks,
Milan

Re: DM_INTEGRITY Kconfig help (was: Re: dm: add integrity target)

[Milan Broz]

On 05/04/2017 08:03 AM, Geert Uytterhoeven wrote:
>> +config DM_INTEGRITY
>> +   tristate "Integrity target"
>> +   depends on BLK_DEV_DM
>> +   select BLK_DEV_INTEGRITY
>> +   select DM_BUFIO
>> +   select CRYPTO
>> +   select ASYNC_XOR
>> +   ---help---
>> +  This is the integrity target.
> 
> Which is...?
> 
> Can you please extend the help message for the uneducated?

Yes, sorry, this is our oversight.

Mike already promised that this text will be extended in the next patchset.

For now you can find the description in documentation
Documentation/device-mapper/dm-integrity.txt

"The dm-integrity target emulates a block device that has additional
per-sector tags that can be used for storing integrity information."

(The major designed use case is for providing metadata space for tags
for authenticated encryption in dm-crypt but it can be used also as
a standalone target.)

Thanks,
Milan

Re: [PATCH] dm verity: deferred hash checking for restart/logging mode

[Milan Broz]

On 04/24/2017 09:34 AM, Bongkyu Kim wrote:
> This patch introduces deferred hash checking for dm-verity.
> In case of restart and logging mode, I/O return first and hash checking is
> deferred. It can improve dm-verity's performance greatly.
> 
> This is my testing on qualcomm snapdragon 821 platform with UFS 2.0.
> dd if=/dev/block/dm-0 of=/dev/null bs=1024 count=100
> - vanilla: 238.14 MB/s
> - patched: 331.52 MB/s (+39.2%)
> 
> fio --rw=randread --size=64M --bs=4k --filename=/dev/block/dm-0 --name=job1
> --name=job2 --name=job3 --name=job4
> - vanilla: 325.21 MB/s
> - patched: 356.42 MB/s (+9.6%)
> 
> One major concoren is security risk.
> If data is tampered, this data will not return -EIO, and can be transferred
> to user process. But, device will be rebooted after hash verification.

Isn't one of the goal of integrity checking to PREVENT that userspace
accesses tampered data?

What happens if that corrupted part is malware that just send one packet
with confidential data in time window before it restarts?

Now I am not sure what is the Google Chromebook/Android threat model here.
If we accept patches that allows non-verified data to be returned to userspace,
dmverity just becomes way how to detect flaky hw, I cannot imagine it is 
acceptable
in "verified boot" chain.

Could someone from dm-verity authors comment this?
(I added Sami and Will to cc but not sure if there is anyone else)?

Milan

> After rebooting, device will work with EIO mode.
> In my opinion, this is enough for gurantee integrity by each power cycles.
> 
> Signed-off-by: Bongkyu Kim 
> ---
>  drivers/md/dm-verity-target.c | 58 
> ---
>  drivers/md/dm.c   | 17 +++--
>  drivers/md/dm.h   |  4 +++
>  3 files changed, 73 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 7335d8a..37c4d9c 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -16,6 +16,7 @@
>  
>  #include "dm-verity.h"
>  #include "dm-verity-fec.h"
> +#include "dm.h"
>  
>  #include 
>  #include 
> @@ -62,6 +63,9 @@ struct buffer_aux {
>   int hash_verified;
>  };
>  
> +static void verity_submit_prefetch(struct dm_verity *v,
> + struct dm_verity_io *io);
> +
>  /*
>   * Initialize struct buffer_aux for a freshly created buffer.
>   */
> @@ -462,12 +466,35 @@ static void verity_finish_io(struct dm_verity_io *io, 
> int error)
>   struct dm_verity *v = io->v;
>   struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
>  
> - bio->bi_end_io = io->orig_bi_end_io;
> - bio->bi_error = error;
> + if (v->mode == DM_VERITY_MODE_EIO) {
> + bio->bi_end_io = io->orig_bi_end_io;
> + bio->bi_error = error;
> + }
>  
>   verity_fec_finish_io(io);
>  
> - bio_endio(bio);
> + if (v->mode == DM_VERITY_MODE_EIO) {
> + bio_endio(bio);
> + } else {
> + struct dm_target_io *tio = container_of(bio,
> + struct dm_target_io, clone);
> + struct dm_io *dmio = tio->io;
> + struct bio *ori_bio = dm_io_get_bio(dmio);
> + struct bio_vec *bv;
> + int i;
> +
> + bio_put(bio);
> + free_io(dm_io_get_md(dmio), dmio);
> +
> + bio_for_each_segment_all(bv, ori_bio, i) {
> + struct page *page = bv->bv_page;
> +
> + put_page(page);
> + }
> +
> + bio_put(ori_bio);
> +
> + }
>  }
>  
>  static void verity_work(struct work_struct *w)
> @@ -486,6 +513,28 @@ static void verity_end_io(struct bio *bio)
>   return;
>   }
>  
> + if (io->v->mode != DM_VERITY_MODE_EIO) {
> + struct dm_target_io *tio = container_of(bio,
> + struct dm_target_io, clone);
> + struct bio *ori_bio = dm_io_get_bio(tio->io);
> + struct bio_vec *bv;
> + int i;
> +
> + bio_for_each_segment_all(bv, ori_bio, i) {
> + struct page *page = bv->bv_page;
> +
> + get_page(page);
> + }
> +
> + bio_get(ori_bio);
> + bio_get(bio);
> +
> + bio->bi_end_io = io->orig_bi_end_io;
> + bio_endio(bio);
> +
> + verity_submit_prefetch(io->v, io);
> + }
> +
>   INIT_WORK(>work, verity_work);
>   queue_work(io->v->verify_wq, >work);
>  }
> @@ -586,7 +635,8 @@ static int verity_map(struct dm_target *ti, struct bio 
> *bio)
>  
>   verity_fec_init_io(io);
>  
> - verity_submit_prefetch(v, io);
> + if (v->mode == DM_VERITY_MODE_EIO)
> + verity_submit_prefetch(v, io);
>  
>   generic_make_request(bio);
>  
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 8bf3977..9eca0a0 100644
> --- a/drivers/md/dm.c
> +++ 

Re: [PATCH] dm verity: deferred hash checking for restart/logging mode

[Milan Broz]

On 04/24/2017 09:34 AM, Bongkyu Kim wrote:
> This patch introduces deferred hash checking for dm-verity.
> In case of restart and logging mode, I/O return first and hash checking is
> deferred. It can improve dm-verity's performance greatly.
> 
> This is my testing on qualcomm snapdragon 821 platform with UFS 2.0.
> dd if=/dev/block/dm-0 of=/dev/null bs=1024 count=100
> - vanilla: 238.14 MB/s
> - patched: 331.52 MB/s (+39.2%)
> 
> fio --rw=randread --size=64M --bs=4k --filename=/dev/block/dm-0 --name=job1
> --name=job2 --name=job3 --name=job4
> - vanilla: 325.21 MB/s
> - patched: 356.42 MB/s (+9.6%)
> 
> One major concoren is security risk.
> If data is tampered, this data will not return -EIO, and can be transferred
> to user process. But, device will be rebooted after hash verification.

Isn't one of the goal of integrity checking to PREVENT that userspace
accesses tampered data?

What happens if that corrupted part is malware that just send one packet
with confidential data in time window before it restarts?

Now I am not sure what is the Google Chromebook/Android threat model here.
If we accept patches that allows non-verified data to be returned to userspace,
dmverity just becomes way how to detect flaky hw, I cannot imagine it is 
acceptable
in "verified boot" chain.

Could someone from dm-verity authors comment this?
(I added Sami and Will to cc but not sure if there is anyone else)?

Milan

> After rebooting, device will work with EIO mode.
> In my opinion, this is enough for gurantee integrity by each power cycles.
> 
> Signed-off-by: Bongkyu Kim 
> ---
>  drivers/md/dm-verity-target.c | 58 
> ---
>  drivers/md/dm.c   | 17 +++--
>  drivers/md/dm.h   |  4 +++
>  3 files changed, 73 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 7335d8a..37c4d9c 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -16,6 +16,7 @@
>  
>  #include "dm-verity.h"
>  #include "dm-verity-fec.h"
> +#include "dm.h"
>  
>  #include 
>  #include 
> @@ -62,6 +63,9 @@ struct buffer_aux {
>   int hash_verified;
>  };
>  
> +static void verity_submit_prefetch(struct dm_verity *v,
> + struct dm_verity_io *io);
> +
>  /*
>   * Initialize struct buffer_aux for a freshly created buffer.
>   */
> @@ -462,12 +466,35 @@ static void verity_finish_io(struct dm_verity_io *io, 
> int error)
>   struct dm_verity *v = io->v;
>   struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
>  
> - bio->bi_end_io = io->orig_bi_end_io;
> - bio->bi_error = error;
> + if (v->mode == DM_VERITY_MODE_EIO) {
> + bio->bi_end_io = io->orig_bi_end_io;
> + bio->bi_error = error;
> + }
>  
>   verity_fec_finish_io(io);
>  
> - bio_endio(bio);
> + if (v->mode == DM_VERITY_MODE_EIO) {
> + bio_endio(bio);
> + } else {
> + struct dm_target_io *tio = container_of(bio,
> + struct dm_target_io, clone);
> + struct dm_io *dmio = tio->io;
> + struct bio *ori_bio = dm_io_get_bio(dmio);
> + struct bio_vec *bv;
> + int i;
> +
> + bio_put(bio);
> + free_io(dm_io_get_md(dmio), dmio);
> +
> + bio_for_each_segment_all(bv, ori_bio, i) {
> + struct page *page = bv->bv_page;
> +
> + put_page(page);
> + }
> +
> + bio_put(ori_bio);
> +
> + }
>  }
>  
>  static void verity_work(struct work_struct *w)
> @@ -486,6 +513,28 @@ static void verity_end_io(struct bio *bio)
>   return;
>   }
>  
> + if (io->v->mode != DM_VERITY_MODE_EIO) {
> + struct dm_target_io *tio = container_of(bio,
> + struct dm_target_io, clone);
> + struct bio *ori_bio = dm_io_get_bio(tio->io);
> + struct bio_vec *bv;
> + int i;
> +
> + bio_for_each_segment_all(bv, ori_bio, i) {
> + struct page *page = bv->bv_page;
> +
> + get_page(page);
> + }
> +
> + bio_get(ori_bio);
> + bio_get(bio);
> +
> + bio->bi_end_io = io->orig_bi_end_io;
> + bio_endio(bio);
> +
> + verity_submit_prefetch(io->v, io);
> + }
> +
>   INIT_WORK(>work, verity_work);
>   queue_work(io->v->verify_wq, >work);
>  }
> @@ -586,7 +635,8 @@ static int verity_map(struct dm_target *ti, struct bio 
> *bio)
>  
>   verity_fec_init_io(io);
>  
> - verity_submit_prefetch(v, io);
> + if (v->mode == DM_VERITY_MODE_EIO)
> + verity_submit_prefetch(v, io);
>  
>   generic_make_request(bio);
>  
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 8bf3977..9eca0a0 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -164,6 

Re: [PATCH 5/5] dm verity: add support for version 0 of the on-disk format

[Milan Broz]

On 04/11/2017 05:33 PM, Enric Balletbo i Serra wrote:
> From: Will Drewry 
> 
> Version 0 of the on-disk format is compatible with the format used in the
> Chromium OS. This patch adds support for this version.
> 
> Format type 0 is the original Chrome OS version, whereas the format type 1
> is current version, but 0, the original format used in the Chromium OS is
> still used in most devices. This patch adds support for the original
> on-disk format so you can decide which want you want to use.

NACK!

What the ...  is this patch doing?

Isn't the format 0 already there? According to
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/device-mapper/verity.txt


This is the type of the on-disk hash format.

0 is the original format used in the Chromium OS.
  The salt is appended when hashing, digests are stored continuously and
  the rest of the block is padded with zeroes.

Reading this patch, it does something completely different than it describes
in header.

How is superblock format related to:
+MODULE_PARM_DESC(dev_wait, "Wait forever for a backing device");
...
not mentioning complete rewrite of verity table parsing... why?

Also please note that there is userspace (veritysetup) that have to work with
the old format as well (I think it does already).
I think we solved the compatibility years ago.

If not, please describe properly what is missing.
I do not see any on-disk format changes here...

Milan

> 
> Signed-off-by: Will Drewry 
> Signed-off-by: Enric Balletbo i Serra 
> ---
>  drivers/md/dm-verity-target.c | 279 
> --
>  init/do_mounts_dm.c   |  16 +--
>  2 files changed, 220 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 7335d8a..c098d22 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -17,6 +17,7 @@
>  #include "dm-verity.h"
>  #include "dm-verity-fec.h"
>  
> +#include 
>  #include 
>  #include 
>  
> @@ -28,6 +29,7 @@
>  #define DM_VERITY_DEFAULT_PREFETCH_SIZE  262144
>  
>  #define DM_VERITY_MAX_CORRUPTED_ERRS 100
> +#define DM_VERITY_NUM_POSITIONAL_ARGS10
>  
>  #define DM_VERITY_OPT_LOGGING"ignore_corruption"
>  #define DM_VERITY_OPT_RESTART"restart_on_corruption"
> @@ -46,6 +48,11 @@ struct dm_verity_prefetch_work {
>   unsigned n_blocks;
>  };
>  
> +/* Controls whether verity_get_device will wait forever for a device. */
> +static int dev_wait;
> +module_param(dev_wait, int, 0444);
> +MODULE_PARM_DESC(dev_wait, "Wait forever for a backing device");
> +
>  /*
>   * Auxiliary structure appended to each dm-bufio buffer. If the value
>   * hash_verified is nonzero, hash of the block has been verified.
> @@ -803,6 +810,183 @@ static int verity_parse_opt_args(struct dm_arg_set *as, 
> struct dm_verity *v)
>   return r;
>  }
>  
> +static int verity_get_device(struct dm_target *ti, const char *devname,
> +  struct dm_dev **dm_dev)
> +{
> + do {
> + /* Try the normal path first since if everything is ready, it
> +  * will be the fastest.
> +  */
> + if (!dm_get_device(ti, devname, /*FMODE_READ*/
> +dm_table_get_mode(ti->table), dm_dev))
> + return 0;
> +
> + if (!dev_wait)
> + break;
> +
> + /* No need to be too aggressive since this is a slow path. */
> + msleep(500);
> + } while (driver_probe_done() != 0 || !(*dm_dev));
> + return -1;
> +}
> +
> +struct verity_args {
> + int version;
> + char *data_device;
> + char *hash_device;
> + int data_block_size_bits;
> + int hash_block_size_bits;
> + u64 num_data_blocks;
> + u64 hash_start_block;
> + char *algorithm;
> + char *digest;
> + char *salt;
> +};
> +
> +static void pr_args(struct verity_args *args)
> +{
> + printk(KERN_INFO "VERITY args: version=%d data_device=%s hash_device=%s"
> + " data_block_size_bits=%d hash_block_size_bits=%d"
> + " num_data_blocks=%lld hash_start_block=%lld"
> + " algorithm=%s digest=%s salt=%s\n",
> + args->version,
> + args->data_device,
> + args->hash_device,
> + args->data_block_size_bits,
> + args->hash_block_size_bits,
> + args->num_data_blocks,
> + args->hash_start_block,
> + args->algorithm,
> + args->digest,
> + args->salt);
> +}
> +
> +/*
> + * positional_args - collects the argments using the positional
> + * parameters.
> + * arg# - parameter
> + *0 - version
> + *1 - data device
> + *2 - hash device - may be same as data device
> + *3 - data block size log2
> + *4 - hash 

Re: [PATCH 5/5] dm verity: add support for version 0 of the on-disk format

[Milan Broz]

On 04/11/2017 05:33 PM, Enric Balletbo i Serra wrote:
> From: Will Drewry 
> 
> Version 0 of the on-disk format is compatible with the format used in the
> Chromium OS. This patch adds support for this version.
> 
> Format type 0 is the original Chrome OS version, whereas the format type 1
> is current version, but 0, the original format used in the Chromium OS is
> still used in most devices. This patch adds support for the original
> on-disk format so you can decide which want you want to use.

NACK!

What the ...  is this patch doing?

Isn't the format 0 already there? According to
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/device-mapper/verity.txt


This is the type of the on-disk hash format.

0 is the original format used in the Chromium OS.
  The salt is appended when hashing, digests are stored continuously and
  the rest of the block is padded with zeroes.

Reading this patch, it does something completely different than it describes
in header.

How is superblock format related to:
+MODULE_PARM_DESC(dev_wait, "Wait forever for a backing device");
...
not mentioning complete rewrite of verity table parsing... why?

Also please note that there is userspace (veritysetup) that have to work with
the old format as well (I think it does already).
I think we solved the compatibility years ago.

If not, please describe properly what is missing.
I do not see any on-disk format changes here...

Milan

> 
> Signed-off-by: Will Drewry 
> Signed-off-by: Enric Balletbo i Serra 
> ---
>  drivers/md/dm-verity-target.c | 279 
> --
>  init/do_mounts_dm.c   |  16 +--
>  2 files changed, 220 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 7335d8a..c098d22 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -17,6 +17,7 @@
>  #include "dm-verity.h"
>  #include "dm-verity-fec.h"
>  
> +#include 
>  #include 
>  #include 
>  
> @@ -28,6 +29,7 @@
>  #define DM_VERITY_DEFAULT_PREFETCH_SIZE  262144
>  
>  #define DM_VERITY_MAX_CORRUPTED_ERRS 100
> +#define DM_VERITY_NUM_POSITIONAL_ARGS10
>  
>  #define DM_VERITY_OPT_LOGGING"ignore_corruption"
>  #define DM_VERITY_OPT_RESTART"restart_on_corruption"
> @@ -46,6 +48,11 @@ struct dm_verity_prefetch_work {
>   unsigned n_blocks;
>  };
>  
> +/* Controls whether verity_get_device will wait forever for a device. */
> +static int dev_wait;
> +module_param(dev_wait, int, 0444);
> +MODULE_PARM_DESC(dev_wait, "Wait forever for a backing device");
> +
>  /*
>   * Auxiliary structure appended to each dm-bufio buffer. If the value
>   * hash_verified is nonzero, hash of the block has been verified.
> @@ -803,6 +810,183 @@ static int verity_parse_opt_args(struct dm_arg_set *as, 
> struct dm_verity *v)
>   return r;
>  }
>  
> +static int verity_get_device(struct dm_target *ti, const char *devname,
> +  struct dm_dev **dm_dev)
> +{
> + do {
> + /* Try the normal path first since if everything is ready, it
> +  * will be the fastest.
> +  */
> + if (!dm_get_device(ti, devname, /*FMODE_READ*/
> +dm_table_get_mode(ti->table), dm_dev))
> + return 0;
> +
> + if (!dev_wait)
> + break;
> +
> + /* No need to be too aggressive since this is a slow path. */
> + msleep(500);
> + } while (driver_probe_done() != 0 || !(*dm_dev));
> + return -1;
> +}
> +
> +struct verity_args {
> + int version;
> + char *data_device;
> + char *hash_device;
> + int data_block_size_bits;
> + int hash_block_size_bits;
> + u64 num_data_blocks;
> + u64 hash_start_block;
> + char *algorithm;
> + char *digest;
> + char *salt;
> +};
> +
> +static void pr_args(struct verity_args *args)
> +{
> + printk(KERN_INFO "VERITY args: version=%d data_device=%s hash_device=%s"
> + " data_block_size_bits=%d hash_block_size_bits=%d"
> + " num_data_blocks=%lld hash_start_block=%lld"
> + " algorithm=%s digest=%s salt=%s\n",
> + args->version,
> + args->data_device,
> + args->hash_device,
> + args->data_block_size_bits,
> + args->hash_block_size_bits,
> + args->num_data_blocks,
> + args->hash_start_block,
> + args->algorithm,
> + args->digest,
> + args->salt);
> +}
> +
> +/*
> + * positional_args - collects the argments using the positional
> + * parameters.
> + * arg# - parameter
> + *0 - version
> + *1 - data device
> + *2 - hash device - may be same as data device
> + *3 - data block size log2
> + *4 - hash block size log2
> + *5 - number of data blocks
> + *6 - hash 

Re: [RFC PATCH v5] IV Generation algorithms for dm-crypt

[Milan Broz]

On 04/07/2017 12:47 PM, Binoy Jayan wrote:
> ===
> dm-crypt optimization for larger block sizes
> ===
...
> Tests with dd [direct i/o]
> 
> Sequential read-0.134 %
> Sequential Write   +0.091 %
> 
> Tests with fio [Aggregate bandwidth - aggrb]
> 
> Random Read+0.358 %
> Random Write   +0.010 %
> 
> Tests with bonnie++ [768 MiB File, 384 MiB Ram]
> after mounting dm-crypt target as ext4
> 
> Sequential o/p [per-char]  -2.876 %
> Sequential o/p [per-blk]   +0.992 %
> Sequential o/p [re-write]  +4.465 %
> 
> Sequential i/p [per-char] -0.453 %
> Sequential i/p [per-blk]  -0.740 %
> 
> Sequential create -0.255 %
> Sequential delete +0.042 %
> Random create -0.007 %
> Random delete +0.454 %
> 
> NB: The '+' sign shows improvement and '-' shows degradation.
> The tests were performed with minimal cpu load.
> Tests with higher cpu load to be done

Well, it is good that there is no performance degradation but it
would be nice to have some user of it that proves it is really
working for your hw.

FYI - with patch that increases dmcrypt sector size to 4k
I can see improvement in speed usually in 5-15% with sync AES-NI
(depends on access pattern), with dmcrypt mapped to memory
it is even close to 20% speed up (but such a configuration is
completely artificial).

I wonder why increased dmcrypt sector size does not work for your hw,
it should help as well (and can be combiuned later with this IV approach).
(For native 4k drives this should be used in future anyway...)

Milan

Re: [RFC PATCH v5] IV Generation algorithms for dm-crypt

[Milan Broz]

On 04/07/2017 12:47 PM, Binoy Jayan wrote:
> ===
> dm-crypt optimization for larger block sizes
> ===
...
> Tests with dd [direct i/o]
> 
> Sequential read-0.134 %
> Sequential Write   +0.091 %
> 
> Tests with fio [Aggregate bandwidth - aggrb]
> 
> Random Read+0.358 %
> Random Write   +0.010 %
> 
> Tests with bonnie++ [768 MiB File, 384 MiB Ram]
> after mounting dm-crypt target as ext4
> 
> Sequential o/p [per-char]  -2.876 %
> Sequential o/p [per-blk]   +0.992 %
> Sequential o/p [re-write]  +4.465 %
> 
> Sequential i/p [per-char] -0.453 %
> Sequential i/p [per-blk]  -0.740 %
> 
> Sequential create -0.255 %
> Sequential delete +0.042 %
> Random create -0.007 %
> Random delete +0.454 %
> 
> NB: The '+' sign shows improvement and '-' shows degradation.
> The tests were performed with minimal cpu load.
> Tests with higher cpu load to be done

Well, it is good that there is no performance degradation but it
would be nice to have some user of it that proves it is really
working for your hw.

FYI - with patch that increases dmcrypt sector size to 4k
I can see improvement in speed usually in 5-15% with sync AES-NI
(depends on access pattern), with dmcrypt mapped to memory
it is even close to 20% speed up (but such a configuration is
completely artificial).

I wonder why increased dmcrypt sector size does not work for your hw,
it should help as well (and can be combiuned later with this IV approach).
(For native 4k drives this should be used in future anyway...)

Milan

Re: [RFC PATCH v4] IV Generation algorithms for dm-crypt

[Milan Broz]

On 03/01/2017 02:04 PM, Milan Broz wrote:
> On 03/01/2017 01:42 PM, Gilad Ben-Yossef wrote:
> ...
> 
>> I can certainly understand if you don't wont to take the patch until
>> we have results with
>> dm-crypt itself but the difference between 8 separate invocation of
>> the engine for 512
>> bytes of XTS and a single invocation for 4KB are pretty big.
> 
> Yes, I know it. But the same can be achieved if we just implement
> 4k sector encryption in dmcrypt. It is incompatible with LUKS1
> (but next LUKS version will support it) but I think this is not
> a problem for now.
> 
> If the underlying device supports atomic write of 4k sectors, then
> there should not be a problem.
> 
> This is one of the speed-up I would like to compare with the IV approach,
> because everyone should benefit from 4k sectors in the end.
> And no crypto API changes are needed here.
> 
> (I have an old patch for this, so I will try to revive it.)

If anyone interested, simple experimental patch for larger sector size
(up to the page size) for dmcrypt is in this branch:

http://git.kernel.org/cgit/linux/kernel/git/mbroz/linux.git/log/?h=dm-crypt-4k-sector

It would be nice to check what performance gain could be provided
by this simple approach.

Milan

Re: [RFC PATCH v4] IV Generation algorithms for dm-crypt

[Milan Broz]

On 03/01/2017 02:04 PM, Milan Broz wrote:
> On 03/01/2017 01:42 PM, Gilad Ben-Yossef wrote:
> ...
> 
>> I can certainly understand if you don't wont to take the patch until
>> we have results with
>> dm-crypt itself but the difference between 8 separate invocation of
>> the engine for 512
>> bytes of XTS and a single invocation for 4KB are pretty big.
> 
> Yes, I know it. But the same can be achieved if we just implement
> 4k sector encryption in dmcrypt. It is incompatible with LUKS1
> (but next LUKS version will support it) but I think this is not
> a problem for now.
> 
> If the underlying device supports atomic write of 4k sectors, then
> there should not be a problem.
> 
> This is one of the speed-up I would like to compare with the IV approach,
> because everyone should benefit from 4k sectors in the end.
> And no crypto API changes are needed here.
> 
> (I have an old patch for this, so I will try to revive it.)

If anyone interested, simple experimental patch for larger sector size
(up to the page size) for dmcrypt is in this branch:

http://git.kernel.org/cgit/linux/kernel/git/mbroz/linux.git/log/?h=dm-crypt-4k-sector

It would be nice to check what performance gain could be provided
by this simple approach.

Milan

Re: [RFC PATCH v4] IV Generation algorithms for dm-crypt

[Milan Broz]

On 03/01/2017 01:42 PM, Gilad Ben-Yossef wrote:
...

> I can certainly understand if you don't wont to take the patch until
> we have results with
> dm-crypt itself but the difference between 8 separate invocation of
> the engine for 512
> bytes of XTS and a single invocation for 4KB are pretty big.

Yes, I know it. But the same can be achieved if we just implement
4k sector encryption in dmcrypt. It is incompatible with LUKS1
(but next LUKS version will support it) but I think this is not
a problem for now.

If the underlying device supports atomic write of 4k sectors, then
there should not be a problem.

This is one of the speed-up I would like to compare with the IV approach,
because everyone should benefit from 4k sectors in the end.
And no crypto API changes are needed here.

(I have an old patch for this, so I will try to revive it.)

Milan

Re: [RFC PATCH v4] IV Generation algorithms for dm-crypt

[Milan Broz]

On 03/01/2017 01:42 PM, Gilad Ben-Yossef wrote:
...

> I can certainly understand if you don't wont to take the patch until
> we have results with
> dm-crypt itself but the difference between 8 separate invocation of
> the engine for 512
> bytes of XTS and a single invocation for 4KB are pretty big.

Yes, I know it. But the same can be achieved if we just implement
4k sector encryption in dmcrypt. It is incompatible with LUKS1
(but next LUKS version will support it) but I think this is not
a problem for now.

If the underlying device supports atomic write of 4k sectors, then
there should not be a problem.

This is one of the speed-up I would like to compare with the IV approach,
because everyone should benefit from 4k sectors in the end.
And no crypto API changes are needed here.

(I have an old patch for this, so I will try to revive it.)

Milan

Re: [RFC PATCH v4] IV Generation algorithms for dm-crypt

[Milan Broz]

On 03/01/2017 09:30 AM, Gilad Ben-Yossef wrote:
> On Tue, Feb 28, 2017 at 11:05 PM, Milan Broz <gmazyl...@gmail.com> wrote:
>>
>> On 02/22/2017 07:12 AM, Binoy Jayan wrote:
>>>
>>> I was wondering if this is near to be ready for submission (apart from
>>> the testmgr.c
>>> changes) or I need to make some changes to make it similar to the IPSec 
>>> offload?
>>
>> I just tried this and except it registers the IV for every new device again, 
>> it works...
>> (After a while you have many duplicate entries in /proc/crypto.)
>>
>> But I would like to see some summary why such a big patch is needed in the 
>> first place.
>> (During an internal discussions seems that people are already lost in mails 
>> and
>> patches here, so Ondra promised me to send some summary mail soon here.)
>>
>> IIRC the first initial problem was dmcrypt performance on some embedded
>> crypto processors that are not able to cope with small crypto requests 
>> effectively.
>>
>>
>> Do you have some real performance numbers that proves that such a patch is 
>> adequate?
>>
>> I would really like to see the performance issue fixed but I am really not 
>> sure
>> this approach works for everyone. It would be better to avoid repeating this 
>> exercise later.
>> IIRC Ondra's "bulk" mode, despite rejected, shows that there is a potential
>> to speedup things even for crypt drivers that do not support own IV 
>> generators.
>>
> 
> AFAIK the problem that we are trying to solve is that if the IV is
> generated outside the crypto API
> domain than you are forced to have an invocation of the crypto API per
> each block because you
> need to provide the IV for each block.
> 
> By putting the IV generation responsibility in the Crypto API we open
> the way to do a single invocation
> of the crypto API for a whole sequence of blocks.

Sure, but this is theory. Does it really work on some hw already?
Do you have performance measurements or comparison?

> For software implementation of XTS this doesn't matter much - but for
> hardware based XTS providers

It is not only embedded crypto, we have some more reports in the past
that 512B sectors are not ideal even for other systems.
(IIRC it was also with AES-NI that represents really big group of users).

> This lead some vendors to ship hacked up versions of dm-crypt to match
> the specific crypto hardware
> they were using, or so I've heard at least - didn't see the code myself.

I saw few version of that. There was a very hacky way to provide request-based 
dmcrypt
(see old "Introduce the request handling for dm-crypt" thread on dm-devel).
This is not the acceptable way but definitely it points to the same problem.

> I believe Binoy is trying to address this in a generic upstream worthy
> way instead.

IIRC the problem is performance, if we can solve it by some generic way,
good, but for now it seems to be a big change and just hope it helps later...

> Anyway, you are only supposed to see s difference when using a
> hardware based XTS provider algo
> that supports IV generation.
> 
>> I like the patch is now contained inside dmcrypt, but it still exposes IVs 
>> that
>> are designed just for old, insecure, compatibility-only containers.
>>
>> I really do not think every compatible crap must be accessible through 
>> crypto API.
>> (I wrote the dmcrypt lrw and tcw compatibility IVs and I would never do that 
>> this way
>> if I know it is accessible outside of dmcrypt internals...)
>> Even the ESSIV is something that was born to fix predictive IVs (CBC 
>> watermarking
>> attacks) for disk encryption only, no reason to expose it outside of disk 
>> encryption.
>>
> 
> The point is that you have more than one implementation of these
> "compatible crap" - the
> software implementation that you wrote and potentially multiple
> hardware implementations
> and putting this in the crypto API domain is the way to abstract this
> so you use the one
> that works best of your platform.

For XTS you need just simple linear IV. No problem with that, implementation
in crypto API and hw is trivial.

But for compatible IV (that provides compatibility with loopAES and very old 
TrueCrypt),
these should be never ever implemented again anywhere.

Specifically "tcw" is broken, insecure and provided here just to help people to 
migrate
from old Truecrypt containers. Even Truecrypt followers removed it from the 
codebase.
(It is basically combination of IV and slight modification of CBC mode. All
recent version switched to XTS and plain IV.)

So building abstraction over something known

Re: [RFC PATCH v4] IV Generation algorithms for dm-crypt

[Milan Broz]

On 03/01/2017 09:30 AM, Gilad Ben-Yossef wrote:
> On Tue, Feb 28, 2017 at 11:05 PM, Milan Broz  wrote:
>>
>> On 02/22/2017 07:12 AM, Binoy Jayan wrote:
>>>
>>> I was wondering if this is near to be ready for submission (apart from
>>> the testmgr.c
>>> changes) or I need to make some changes to make it similar to the IPSec 
>>> offload?
>>
>> I just tried this and except it registers the IV for every new device again, 
>> it works...
>> (After a while you have many duplicate entries in /proc/crypto.)
>>
>> But I would like to see some summary why such a big patch is needed in the 
>> first place.
>> (During an internal discussions seems that people are already lost in mails 
>> and
>> patches here, so Ondra promised me to send some summary mail soon here.)
>>
>> IIRC the first initial problem was dmcrypt performance on some embedded
>> crypto processors that are not able to cope with small crypto requests 
>> effectively.
>>
>>
>> Do you have some real performance numbers that proves that such a patch is 
>> adequate?
>>
>> I would really like to see the performance issue fixed but I am really not 
>> sure
>> this approach works for everyone. It would be better to avoid repeating this 
>> exercise later.
>> IIRC Ondra's "bulk" mode, despite rejected, shows that there is a potential
>> to speedup things even for crypt drivers that do not support own IV 
>> generators.
>>
> 
> AFAIK the problem that we are trying to solve is that if the IV is
> generated outside the crypto API
> domain than you are forced to have an invocation of the crypto API per
> each block because you
> need to provide the IV for each block.
> 
> By putting the IV generation responsibility in the Crypto API we open
> the way to do a single invocation
> of the crypto API for a whole sequence of blocks.

Sure, but this is theory. Does it really work on some hw already?
Do you have performance measurements or comparison?

> For software implementation of XTS this doesn't matter much - but for
> hardware based XTS providers

It is not only embedded crypto, we have some more reports in the past
that 512B sectors are not ideal even for other systems.
(IIRC it was also with AES-NI that represents really big group of users).

> This lead some vendors to ship hacked up versions of dm-crypt to match
> the specific crypto hardware
> they were using, or so I've heard at least - didn't see the code myself.

I saw few version of that. There was a very hacky way to provide request-based 
dmcrypt
(see old "Introduce the request handling for dm-crypt" thread on dm-devel).
This is not the acceptable way but definitely it points to the same problem.

> I believe Binoy is trying to address this in a generic upstream worthy
> way instead.

IIRC the problem is performance, if we can solve it by some generic way,
good, but for now it seems to be a big change and just hope it helps later...

> Anyway, you are only supposed to see s difference when using a
> hardware based XTS provider algo
> that supports IV generation.
> 
>> I like the patch is now contained inside dmcrypt, but it still exposes IVs 
>> that
>> are designed just for old, insecure, compatibility-only containers.
>>
>> I really do not think every compatible crap must be accessible through 
>> crypto API.
>> (I wrote the dmcrypt lrw and tcw compatibility IVs and I would never do that 
>> this way
>> if I know it is accessible outside of dmcrypt internals...)
>> Even the ESSIV is something that was born to fix predictive IVs (CBC 
>> watermarking
>> attacks) for disk encryption only, no reason to expose it outside of disk 
>> encryption.
>>
> 
> The point is that you have more than one implementation of these
> "compatible crap" - the
> software implementation that you wrote and potentially multiple
> hardware implementations
> and putting this in the crypto API domain is the way to abstract this
> so you use the one
> that works best of your platform.

For XTS you need just simple linear IV. No problem with that, implementation
in crypto API and hw is trivial.

But for compatible IV (that provides compatibility with loopAES and very old 
TrueCrypt),
these should be never ever implemented again anywhere.

Specifically "tcw" is broken, insecure and provided here just to help people to 
migrate
from old Truecrypt containers. Even Truecrypt followers removed it from the 
codebase.
(It is basically combination of IV and slight modification of CBC mode. All
recent version switched to XTS and plain IV.)

So building abstraction over something known to be broken and that is now

Re: [RFC PATCH v4] IV Generation algorithms for dm-crypt

[Milan Broz]

On 02/22/2017 07:12 AM, Binoy Jayan wrote:
> 
> I was wondering if this is near to be ready for submission (apart from
> the testmgr.c
> changes) or I need to make some changes to make it similar to the IPSec 
> offload?

I just tried this and except it registers the IV for every new device again, it 
works...
(After a while you have many duplicate entries in /proc/crypto.)

But I would like to see some summary why such a big patch is needed in the 
first place.
(During an internal discussions seems that people are already lost in mails and
patches here, so Ondra promised me to send some summary mail soon here.)

IIRC the first initial problem was dmcrypt performance on some embedded
crypto processors that are not able to cope with small crypto requests 
effectively.

Do you have some real performance numbers that proves that such a patch is 
adequate?

I would really like to see the performance issue fixed but I am really not sure
this approach works for everyone. It would be better to avoid repeating this 
exercise later.
IIRC Ondra's "bulk" mode, despite rejected, shows that there is a potential
to speedup things even for crypt drivers that do not support own IV generators.

I like the patch is now contained inside dmcrypt, but it still exposes IVs that
are designed just for old, insecure, compatibility-only containers.

I really do not think every compatible crap must be accessible through crypto 
API.
(I wrote the dmcrypt lrw and tcw compatibility IVs and I would never do that 
this way
if I know it is accessible outside of dmcrypt internals...)
Even the ESSIV is something that was born to fix predictive IVs (CBC 
watermarking
attacks) for disk encryption only, no reason to expose it outside of disk 
encryption.

Milan

Re: [RFC PATCH v4] IV Generation algorithms for dm-crypt

[Milan Broz]

On 02/22/2017 07:12 AM, Binoy Jayan wrote:
> 
> I was wondering if this is near to be ready for submission (apart from
> the testmgr.c
> changes) or I need to make some changes to make it similar to the IPSec 
> offload?

I just tried this and except it registers the IV for every new device again, it 
works...
(After a while you have many duplicate entries in /proc/crypto.)

But I would like to see some summary why such a big patch is needed in the 
first place.
(During an internal discussions seems that people are already lost in mails and
patches here, so Ondra promised me to send some summary mail soon here.)

IIRC the first initial problem was dmcrypt performance on some embedded
crypto processors that are not able to cope with small crypto requests 
effectively.

Do you have some real performance numbers that proves that such a patch is 
adequate?

I would really like to see the performance issue fixed but I am really not sure
this approach works for everyone. It would be better to avoid repeating this 
exercise later.
IIRC Ondra's "bulk" mode, despite rejected, shows that there is a potential
to speedup things even for crypt drivers that do not support own IV generators.

I like the patch is now contained inside dmcrypt, but it still exposes IVs that
are designed just for old, insecure, compatibility-only containers.

I really do not think every compatible crap must be accessible through crypto 
API.
(I wrote the dmcrypt lrw and tcw compatibility IVs and I would never do that 
this way
if I know it is accessible outside of dmcrypt internals...)
Even the ESSIV is something that was born to fix predictive IVs (CBC 
watermarking
attacks) for disk encryption only, no reason to expose it outside of disk 
encryption.

Milan

Re: dm-crypt optimization

[Milan Broz]

On 12/20/2016 10:41 AM, Binoy Jayan wrote:
> At a high level the goal is to maximize the size of data blocks that get 
> passed
> to hardware accelerators, minimizing the overhead from setting up and tearing
> down operations in the hardware. Currently dm-crypt itself is a big blocker as
> it manually implements ESSIV and similar algorithms which allow per-block
> encryption of the data so the low level operations from the crypto API can
> only operate on a single block. This is done because currently the crypto API
> doesn't have software implementations of these algorithms itself so dm-crypt
> can't rely on it being able to provide the functionality. The plan to address
> this was to provide some software implementations in the crypto API, then
> update dm-crypt to rely on those. Even for a pure software implementation
> with no hardware acceleration that should hopefully provide a small
> optimization as we need to call into the crypto API less often but it's likely
> to be marginal given the overhead of crypto, the real win would be on a system
> that has an accelerator that can replace the software implementation.
> 
> Currently dm-crypt handles data only in single blocks. This means that it 
> can't
> make good use of hardware cryptography engines since there is an overhead to
> each transaction with the engine but transfers must be split into block sized
> chunks. Allowing the transfer of larger blocks e.g. 'struct bio', could
> mitigate against these costs and could improve performance in operating 
> systems
> with encrypted filesystems. Although qualcomm chipsets support another variant
> of the device-mapper dm-req-crypt, it is not something generic and in
> mainline-able state. Also, it only supports 'XTS-AES' mode of encryption and
> is not compatible with other modes supported by dm-crypt.

So the core problem is that your crypto accelerator can operate efficiently only
with bigger batch sizes.

How big blocks your crypto hw need to be able to operate more efficiently?
What about 4k blocks (no batches), could it be usable trade-off?

With some (backward incompatible) changes in LUKS format I would like to see 
support
for encryption blocks equivalent to sectors size, so it basically means for 4k 
drive 4k
encryption block.
(This should decrease overhead, now is everything processed on 512 blocks only.)

Support of bigger block sizes would be unsafe without additional mechanism that 
provides
atomic writes of multiple sectors. Maybe it applies to 4k as well on some 
devices though...)

The above is not going against your proposal, I am just curious if this is 
enough
to provide better performance on your hw accelerator or not.

Milan

> However, there are some challenges and a few possibilities to address this. I
> request you to provide your suggestions on whether the points mentioned below
> makes sense and if it could be done differently.
> 
> 1. Move the 'real' IV generation algorithms to crypto layer (e.g. essiv)
> 2. Increase the 'length' of the scatterlist nodes used in the crypto api. It
>can be made equal to the size of a main memory segment (as defined in
>'struct bio') as they are physcially contiguous.
> 3. Multiple segments in 'struct bio' can be represented as scatterlist of all
>segments in a 'struct bio'.
> 
> 4. Move algorithms 'lmk' and 'tcw' (which are IV combined with hacks to the
>cbc mode) to create a customized cbc algorithm, implemented in a seperate
>file (e.g. cbc_lmk.c/cbc_tcw.c). As Milan suggested, these can't be treated
>as real IVs as these include hacks to the cbc mode (and directly manipulate
>encrypted data).
> 
> 5. Move key selection logic to user space or always assume keycount as '1'
>(as mentioned in the dm-crypt param format below) so that the key selection
>logic does not have to be dependent on the sector number. This is necessary
>as the key is selected otherwise based on sector number:
> 
>key_index = sector & (key_count - 1)
> 
>If block size for scatterlist nodes are increased beyond sector boundary
>(which is what we plan to achieve, for performance), the key set for every
>cipher operation cannot be changed at the sector level.
> 
>dm-crypt param format : cipher[:keycount]-mode-iv:ivopts
>Example   : aes:2-cbc-essiv:sha256
> 
>Also as Milan suggested, it is not wise to move the key selection logic to
>the crypto layer as it will prevent any changes to the key structure later.
> 
> The following is a reference to an earlier patchset. It had the cipher mode
> 'cbc' mixed up with the IV algorithms and is usually not the preferred way.
> 
> Reference:
> https://lkml.org/lkml/2016/12/13/65
> https://lkml.org/lkml/2016/12/13/66
> 

Re: dm-crypt optimization

[Milan Broz]

On 12/20/2016 10:41 AM, Binoy Jayan wrote:
> At a high level the goal is to maximize the size of data blocks that get 
> passed
> to hardware accelerators, minimizing the overhead from setting up and tearing
> down operations in the hardware. Currently dm-crypt itself is a big blocker as
> it manually implements ESSIV and similar algorithms which allow per-block
> encryption of the data so the low level operations from the crypto API can
> only operate on a single block. This is done because currently the crypto API
> doesn't have software implementations of these algorithms itself so dm-crypt
> can't rely on it being able to provide the functionality. The plan to address
> this was to provide some software implementations in the crypto API, then
> update dm-crypt to rely on those. Even for a pure software implementation
> with no hardware acceleration that should hopefully provide a small
> optimization as we need to call into the crypto API less often but it's likely
> to be marginal given the overhead of crypto, the real win would be on a system
> that has an accelerator that can replace the software implementation.
> 
> Currently dm-crypt handles data only in single blocks. This means that it 
> can't
> make good use of hardware cryptography engines since there is an overhead to
> each transaction with the engine but transfers must be split into block sized
> chunks. Allowing the transfer of larger blocks e.g. 'struct bio', could
> mitigate against these costs and could improve performance in operating 
> systems
> with encrypted filesystems. Although qualcomm chipsets support another variant
> of the device-mapper dm-req-crypt, it is not something generic and in
> mainline-able state. Also, it only supports 'XTS-AES' mode of encryption and
> is not compatible with other modes supported by dm-crypt.

So the core problem is that your crypto accelerator can operate efficiently only
with bigger batch sizes.

How big blocks your crypto hw need to be able to operate more efficiently?
What about 4k blocks (no batches), could it be usable trade-off?

With some (backward incompatible) changes in LUKS format I would like to see 
support
for encryption blocks equivalent to sectors size, so it basically means for 4k 
drive 4k
encryption block.
(This should decrease overhead, now is everything processed on 512 blocks only.)

Support of bigger block sizes would be unsafe without additional mechanism that 
provides
atomic writes of multiple sectors. Maybe it applies to 4k as well on some 
devices though...)

The above is not going against your proposal, I am just curious if this is 
enough
to provide better performance on your hw accelerator or not.

Milan

> However, there are some challenges and a few possibilities to address this. I
> request you to provide your suggestions on whether the points mentioned below
> makes sense and if it could be done differently.
> 
> 1. Move the 'real' IV generation algorithms to crypto layer (e.g. essiv)
> 2. Increase the 'length' of the scatterlist nodes used in the crypto api. It
>can be made equal to the size of a main memory segment (as defined in
>'struct bio') as they are physcially contiguous.
> 3. Multiple segments in 'struct bio' can be represented as scatterlist of all
>segments in a 'struct bio'.
> 
> 4. Move algorithms 'lmk' and 'tcw' (which are IV combined with hacks to the
>cbc mode) to create a customized cbc algorithm, implemented in a seperate
>file (e.g. cbc_lmk.c/cbc_tcw.c). As Milan suggested, these can't be treated
>as real IVs as these include hacks to the cbc mode (and directly manipulate
>encrypted data).
> 
> 5. Move key selection logic to user space or always assume keycount as '1'
>(as mentioned in the dm-crypt param format below) so that the key selection
>logic does not have to be dependent on the sector number. This is necessary
>as the key is selected otherwise based on sector number:
> 
>key_index = sector & (key_count - 1)
> 
>If block size for scatterlist nodes are increased beyond sector boundary
>(which is what we plan to achieve, for performance), the key set for every
>cipher operation cannot be changed at the sector level.
> 
>dm-crypt param format : cipher[:keycount]-mode-iv:ivopts
>Example   : aes:2-cbc-essiv:sha256
> 
>Also as Milan suggested, it is not wise to move the key selection logic to
>the crypto layer as it will prevent any changes to the key structure later.
> 
> The following is a reference to an earlier patchset. It had the cipher mode
> 'cbc' mixed up with the IV algorithms and is usually not the preferred way.
> 
> Reference:
> https://lkml.org/lkml/2016/12/13/65
> https://lkml.org/lkml/2016/12/13/66
> 

Re: [RFC PATCH v2] crypto: Add IV generation algorithms

[Milan Broz]

On 12/13/2016 09:49 AM, Binoy Jayan wrote:
> Currently, the iv generation algorithms are implemented in dm-crypt.c.
> The goal is to move these algorithms from the dm layer to the kernel
> crypto layer by implementing them as template ciphers so they can be
> implemented in hardware for performance. As part of this patchset, the
> iv-generation code is moved from the dm layer to the crypto layer and
> adapt the dm-layer to send a whole 'bio' (as defined in the block layer)
> at a time. Each bio contains the in memory representation of physically
> contiguous disk blocks. The dm layer sets up a chained scatterlist of
> these blocks split into physically contiguous segments in memory so that
> DMA can be performed. The iv generation algorithms implemented in geniv.c
> include plain, plain64, essiv, benbi, null, lmk and tcw.
...

Just few notes...

The lmk (loopAES) and tcw (old TrueCrypt mode) IVs are in fact hacks,
it is combination of IV and modification of CBC mode (in post calls).

It is used only in these disk-encryption systems, it does not make sense
to use it elsewhere (moreover tcw IV is not safe, it is here only
for compatibility reasons).

I think that IV generators should not modify or read encrypted data directly,
it should only generate IV.

By the move everything to cryptoAPI we are basically introducing some strange 
mix
of IV and modes there, I wonder how this is going to be maintained.
Anyway, Herbert should say if it is ok...

But I would imagine that cryptoAPI should implement only "real" IV generators
and these disk-encryption add-ons keep inside dmcrypt.
(and dmcrypt should probably use normal IVs through crypto API and
build some wrapper around for hacks...)

...

> 
> When using multiple keys with the original dm-crypt, the key selection is
> made based on the sector number as:
> 
> key_index = sector & (key_count - 1)
> 
> This restricts the usage of the same key for encrypting/decrypting a
> single bio. One way to solve this is to move the key management code from
> dm-crypt to cryto layer. But this seems tricky when using template ciphers
> because, when multiple ciphers are instantiated from dm layer, each cipher
> instance set with a unique subkey (part of the bigger master key) and
> these instances themselves do not have access to each other's instances
> or contexts. This way, a single instance cannot encryt/decrypt a whole bio.
> This has to be fixed.

I really do not think the disk encryption key management should be moved
outside of dm-crypt. We cannot then change key structure later easily.

But it is not only key management, you are basically moving much more internals
outside of dm-crypt:

> +struct geniv_ctx {
> + struct crypto_skcipher *child;
> + unsigned int tfms_count;
> + char *ivmode;
> + unsigned int iv_size;
> + char *ivopts;
> + char *cipher;
> + struct geniv_operations *iv_gen_ops;
> + union {
> + struct geniv_essiv_private essiv;
> + struct geniv_benbi_private benbi;
> + struct geniv_lmk_private lmk;
> + struct geniv_tcw_private tcw;
> + } iv_gen_private;
> + void *iv_private;
> + struct crypto_skcipher *tfm;
> + unsigned int key_size;
> + unsigned int key_extra_size;
> + unsigned int key_parts;  /* independent parts in key buffer */

^^^ these key sizes you probably mean by key management.
It is based on way how the key is currently sent into kernel
(one hexa string in ioctl that needs to be split) and have to be changed in 
future.

> + enum setkey_op keyop;
> + char *msg;
> + u8 *key;

Milan

Re: [RFC PATCH v2] crypto: Add IV generation algorithms

[Milan Broz]

On 12/13/2016 09:49 AM, Binoy Jayan wrote:
> Currently, the iv generation algorithms are implemented in dm-crypt.c.
> The goal is to move these algorithms from the dm layer to the kernel
> crypto layer by implementing them as template ciphers so they can be
> implemented in hardware for performance. As part of this patchset, the
> iv-generation code is moved from the dm layer to the crypto layer and
> adapt the dm-layer to send a whole 'bio' (as defined in the block layer)
> at a time. Each bio contains the in memory representation of physically
> contiguous disk blocks. The dm layer sets up a chained scatterlist of
> these blocks split into physically contiguous segments in memory so that
> DMA can be performed. The iv generation algorithms implemented in geniv.c
> include plain, plain64, essiv, benbi, null, lmk and tcw.
...

Just few notes...

The lmk (loopAES) and tcw (old TrueCrypt mode) IVs are in fact hacks,
it is combination of IV and modification of CBC mode (in post calls).

It is used only in these disk-encryption systems, it does not make sense
to use it elsewhere (moreover tcw IV is not safe, it is here only
for compatibility reasons).

I think that IV generators should not modify or read encrypted data directly,
it should only generate IV.

By the move everything to cryptoAPI we are basically introducing some strange 
mix
of IV and modes there, I wonder how this is going to be maintained.
Anyway, Herbert should say if it is ok...

But I would imagine that cryptoAPI should implement only "real" IV generators
and these disk-encryption add-ons keep inside dmcrypt.
(and dmcrypt should probably use normal IVs through crypto API and
build some wrapper around for hacks...)

...

> 
> When using multiple keys with the original dm-crypt, the key selection is
> made based on the sector number as:
> 
> key_index = sector & (key_count - 1)
> 
> This restricts the usage of the same key for encrypting/decrypting a
> single bio. One way to solve this is to move the key management code from
> dm-crypt to cryto layer. But this seems tricky when using template ciphers
> because, when multiple ciphers are instantiated from dm layer, each cipher
> instance set with a unique subkey (part of the bigger master key) and
> these instances themselves do not have access to each other's instances
> or contexts. This way, a single instance cannot encryt/decrypt a whole bio.
> This has to be fixed.

I really do not think the disk encryption key management should be moved
outside of dm-crypt. We cannot then change key structure later easily.

But it is not only key management, you are basically moving much more internals
outside of dm-crypt:

> +struct geniv_ctx {
> + struct crypto_skcipher *child;
> + unsigned int tfms_count;
> + char *ivmode;
> + unsigned int iv_size;
> + char *ivopts;
> + char *cipher;
> + struct geniv_operations *iv_gen_ops;
> + union {
> + struct geniv_essiv_private essiv;
> + struct geniv_benbi_private benbi;
> + struct geniv_lmk_private lmk;
> + struct geniv_tcw_private tcw;
> + } iv_gen_private;
> + void *iv_private;
> + struct crypto_skcipher *tfm;
> + unsigned int key_size;
> + unsigned int key_extra_size;
> + unsigned int key_parts;  /* independent parts in key buffer */

^^^ these key sizes you probably mean by key management.
It is based on way how the key is currently sent into kernel
(one hexa string in ioctl that needs to be split) and have to be changed in 
future.

> + enum setkey_op keyop;
> + char *msg;
> + u8 *key;

Milan

Re: dm-crypt accepts '+' in the key

[Milan Broz]

On 11/12/2016 09:20 PM, Mikulas Patocka wrote:
> Hi
> 
> dm-crypt uses the function kstrtou8 to decode the encryption key. kstrtou8 
> calls kstrtoull and kstrtoull skips the first character if it is '+'.
> 
> Consequently, it is possible to load keys with '+' in it. For example, 
> this is possible:
> 
> dmsetup create cr --table "0 131072 crypt aes-cbc-essiv:sha256 
> +0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0 0 
> /dev/debian/tmptest 0"
> 
> Should this be fixed in dm-crypt or in kstrtou8? A fix in kstrtou8 could 
> be more appropriate, but we don't know how many other kernel parts depend 
> on this "skip plus" behavior...

I would way it should be checked in both places...
For dmcrypt, it should validate input here and should
not accept anything in key field in dm table that is not in hexa representation.

(Is this regression since code switched from simple_strtoul to  kstrtou8
or this bug was there always?)

Milan

Re: dm-crypt accepts '+' in the key

[Milan Broz]

On 11/12/2016 09:20 PM, Mikulas Patocka wrote:
> Hi
> 
> dm-crypt uses the function kstrtou8 to decode the encryption key. kstrtou8 
> calls kstrtoull and kstrtoull skips the first character if it is '+'.
> 
> Consequently, it is possible to load keys with '+' in it. For example, 
> this is possible:
> 
> dmsetup create cr --table "0 131072 crypt aes-cbc-essiv:sha256 
> +0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0 0 
> /dev/debian/tmptest 0"
> 
> Should this be fixed in dm-crypt or in kstrtou8? A fix in kstrtou8 could 
> be more appropriate, but we don't know how many other kernel parts depend 
> on this "skip plus" behavior...

I would way it should be checked in both places...
For dmcrypt, it should validate input here and should
not accept anything in key field in dm table that is not in hexa representation.

(Is this regression since code switched from simple_strtoul to  kstrtou8
or this bug was there always?)

Milan

Re: System freezes after OOM

[Milan Broz]

On 07/14/2016 11:09 AM, Michal Hocko wrote:
> On Wed 13-07-16 11:21:41, Mikulas Patocka wrote:
>>
>>
>> On Wed, 13 Jul 2016, Milan Broz wrote:
>>
>>> On 07/13/2016 02:50 PM, Michal Hocko wrote:
>>>> On Wed 13-07-16 13:10:06, Michal Hocko wrote:
>>>>> On Tue 12-07-16 19:44:11, Mikulas Patocka wrote:
>>>> [...]
>>>>>> As long as swapping is in progress, the free memory is below the limit 
>>>>>> (because the swapping activity itself consumes any memory over the 
>>>>>> limit). 
>>>>>> And that triggered the OOM killer prematurely.
>>>>>
>>>>> I am not sure I understand the last part. Are you saing that we trigger
>>>>> OOM because the initiated swapout will not be able to finish the IO thus
>>>>> release the page in time?
>>>>>
>>>>> The oom detection checks waits for an ongoing writeout if there is no
>>>>> reclaim progress and at least half of the reclaimable memory is either
>>>>> dirty or under writeback. Pages under swaout are marked as under
>>>>> writeback AFAIR. The writeout path (dm-crypt worker in this case) should
>>>>> be able to allocate a memory from the mempool, hand over to the crypt
>>>>> layer and finish the IO. Is it possible this might take a lot of time?
>>>>
>>>> I am not familiar with the crypto API but from what I understood from
>>>> crypt_convert the encryption is done asynchronously. Then I got lost in
>>>> the indirection. Who is completing the request and from what kind of
>>>> context? Is it possible it wouldn't be runable for a long time?
>>>
>>> If you mean crypt_convert in dm-crypt, then it can do asynchronous 
>>> completion
>>> but usually (with AES-NI ans sw implementations) it run the operation 
>>> completely
>>> synchronously.
>>> Asynchronous processing is quite rare, usually only on some specific 
>>> hardware
>>> crypto accelerators.
>>>
>>> Once the encryption is finished, the cloned bio is sent to the block
>>> layer for processing.
>>> (There is also some magic with sorting writes but Mikulas knows this 
>>> better.)
>>
>> dm-crypt receives requests in crypt_map, then it distributes write 
>> requests to multiple encryption threads. Encryption is done usually 
>> synchronously; asynchronous completion is used only when using some PCI 
>> cards that accelerate encryption. When encryption finishes, the encrypted 
>> pages are submitted to a thread dmcrypt_write that sorts the requests 
>> using rbtree and submits them.
> 
> OK. I was worried that the async context would depend on WQ and a lack
> of workers could lead to long stalls. Dedicated kernel threads seem
> sufficient.

Just for the record - if there is a suspicion that some crypto operation
causes problem, dmcrypt can use null cipher. This degrades encryption/decryption
to just plain memcpy inside crypto API but leaves all workqueues and
tooling around the same.
(I added it to cryptsetup to easily configure it and it was intended to test 
dmcrypt
non-crypto overherad in fact.)

Anyway, thanks for looking into this!
Milan

Re: System freezes after OOM

[Milan Broz]

On 07/14/2016 11:09 AM, Michal Hocko wrote:
> On Wed 13-07-16 11:21:41, Mikulas Patocka wrote:
>>
>>
>> On Wed, 13 Jul 2016, Milan Broz wrote:
>>
>>> On 07/13/2016 02:50 PM, Michal Hocko wrote:
>>>> On Wed 13-07-16 13:10:06, Michal Hocko wrote:
>>>>> On Tue 12-07-16 19:44:11, Mikulas Patocka wrote:
>>>> [...]
>>>>>> As long as swapping is in progress, the free memory is below the limit 
>>>>>> (because the swapping activity itself consumes any memory over the 
>>>>>> limit). 
>>>>>> And that triggered the OOM killer prematurely.
>>>>>
>>>>> I am not sure I understand the last part. Are you saing that we trigger
>>>>> OOM because the initiated swapout will not be able to finish the IO thus
>>>>> release the page in time?
>>>>>
>>>>> The oom detection checks waits for an ongoing writeout if there is no
>>>>> reclaim progress and at least half of the reclaimable memory is either
>>>>> dirty or under writeback. Pages under swaout are marked as under
>>>>> writeback AFAIR. The writeout path (dm-crypt worker in this case) should
>>>>> be able to allocate a memory from the mempool, hand over to the crypt
>>>>> layer and finish the IO. Is it possible this might take a lot of time?
>>>>
>>>> I am not familiar with the crypto API but from what I understood from
>>>> crypt_convert the encryption is done asynchronously. Then I got lost in
>>>> the indirection. Who is completing the request and from what kind of
>>>> context? Is it possible it wouldn't be runable for a long time?
>>>
>>> If you mean crypt_convert in dm-crypt, then it can do asynchronous 
>>> completion
>>> but usually (with AES-NI ans sw implementations) it run the operation 
>>> completely
>>> synchronously.
>>> Asynchronous processing is quite rare, usually only on some specific 
>>> hardware
>>> crypto accelerators.
>>>
>>> Once the encryption is finished, the cloned bio is sent to the block
>>> layer for processing.
>>> (There is also some magic with sorting writes but Mikulas knows this 
>>> better.)
>>
>> dm-crypt receives requests in crypt_map, then it distributes write 
>> requests to multiple encryption threads. Encryption is done usually 
>> synchronously; asynchronous completion is used only when using some PCI 
>> cards that accelerate encryption. When encryption finishes, the encrypted 
>> pages are submitted to a thread dmcrypt_write that sorts the requests 
>> using rbtree and submits them.
> 
> OK. I was worried that the async context would depend on WQ and a lack
> of workers could lead to long stalls. Dedicated kernel threads seem
> sufficient.

Just for the record - if there is a suspicion that some crypto operation
causes problem, dmcrypt can use null cipher. This degrades encryption/decryption
to just plain memcpy inside crypto API but leaves all workqueues and
tooling around the same.
(I added it to cryptsetup to easily configure it and it was intended to test 
dmcrypt
non-crypto overherad in fact.)

Anyway, thanks for looking into this!
Milan

Re: System freezes after OOM

[Milan Broz]

On 07/13/2016 02:50 PM, Michal Hocko wrote:
> On Wed 13-07-16 13:10:06, Michal Hocko wrote:
>> On Tue 12-07-16 19:44:11, Mikulas Patocka wrote:
> [...]
>>> As long as swapping is in progress, the free memory is below the limit 
>>> (because the swapping activity itself consumes any memory over the limit). 
>>> And that triggered the OOM killer prematurely.
>>
>> I am not sure I understand the last part. Are you saing that we trigger
>> OOM because the initiated swapout will not be able to finish the IO thus
>> release the page in time?
>>
>> The oom detection checks waits for an ongoing writeout if there is no
>> reclaim progress and at least half of the reclaimable memory is either
>> dirty or under writeback. Pages under swaout are marked as under
>> writeback AFAIR. The writeout path (dm-crypt worker in this case) should
>> be able to allocate a memory from the mempool, hand over to the crypt
>> layer and finish the IO. Is it possible this might take a lot of time?
> 
> I am not familiar with the crypto API but from what I understood from
> crypt_convert the encryption is done asynchronously. Then I got lost in
> the indirection. Who is completing the request and from what kind of
> context? Is it possible it wouldn't be runable for a long time?

If you mean crypt_convert in dm-crypt, then it can do asynchronous completion
but usually (with AES-NI ans sw implementations) it run the operation completely
synchronously.
Asynchronous processing is quite rare, usually only on some specific hardware
crypto accelerators.

Once the encryption is finished, the cloned bio is sent to the block
layer for processing.
(There is also some magic with sorting writes but Mikulas knows this better.)

Milan
p.s. I added cc to dm-devel, some dmcrypt people reads only this list.

Re: System freezes after OOM

[Milan Broz]

On 07/13/2016 02:50 PM, Michal Hocko wrote:
> On Wed 13-07-16 13:10:06, Michal Hocko wrote:
>> On Tue 12-07-16 19:44:11, Mikulas Patocka wrote:
> [...]
>>> As long as swapping is in progress, the free memory is below the limit 
>>> (because the swapping activity itself consumes any memory over the limit). 
>>> And that triggered the OOM killer prematurely.
>>
>> I am not sure I understand the last part. Are you saing that we trigger
>> OOM because the initiated swapout will not be able to finish the IO thus
>> release the page in time?
>>
>> The oom detection checks waits for an ongoing writeout if there is no
>> reclaim progress and at least half of the reclaimable memory is either
>> dirty or under writeback. Pages under swaout are marked as under
>> writeback AFAIR. The writeout path (dm-crypt worker in this case) should
>> be able to allocate a memory from the mempool, hand over to the crypt
>> layer and finish the IO. Is it possible this might take a lot of time?
> 
> I am not familiar with the crypto API but from what I understood from
> crypt_convert the encryption is done asynchronously. Then I got lost in
> the indirection. Who is completing the request and from what kind of
> context? Is it possible it wouldn't be runable for a long time?

If you mean crypt_convert in dm-crypt, then it can do asynchronous completion
but usually (with AES-NI ans sw implementations) it run the operation completely
synchronously.
Asynchronous processing is quite rare, usually only on some specific hardware
crypto accelerators.

Once the encryption is finished, the cloned bio is sent to the block
layer for processing.
(There is also some magic with sorting writes but Mikulas knows this better.)

Milan
p.s. I added cc to dm-devel, some dmcrypt people reads only this list.

Re: [RFC 2/3] crypto: Introduce CRYPTO_ALG_BULK flag

[Milan Broz]

On 05/27/2016 09:04 AM, Baolin Wang wrote:
> Hi Milan,
> 
> On 27 May 2016 at 14:31, Milan Broz <gmazyl...@gmail.com> wrote:
>> On 05/25/2016 08:12 AM, Baolin Wang wrote:
>>> Now some cipher hardware engines prefer to handle bulk block rather than one
>>> sector (512 bytes) created by dm-crypt, cause these cipher engines can 
>>> handle
>>> the intermediate values (IV) by themselves in one bulk block. This means we
>>> can increase the size of the request by merging request rather than always 
>>> 512
>>> bytes and thus increase the hardware engine processing speed.
>>
>> Hi,
>>
>> could you please elaborate how exactly you are processing independently
>> encrypted sectors? For example with XTS mode. Do you play internally with
>> tweak calculation? Does this keep 512 bytes sector encryption blocks 
>> independent?
>>
>> (If not, it is breaking compatibility everywhere and you are reinventing
>> disk encryption logic here - just for performance reason for some hw
>> not designed for this task... But that was said several times already.)
> 
> These are what the cipher hardware engine and engine driver should do,
> for software we just need send one initial IV and bulk data to crypto
> layer, which is enough.

Hi,

Thanks for answer.

So this is just doing some kind of batch processing optimization inside the 
driver?
I still do not understand why it is not possible to "batch" these requests 
inside
you driver (with async API) then and process them in one go without any changes
to dmcrypt though. (But I am not familiar with these drivers.)

If I understand it correctly, subsequent IVs are calculated inside
your driver just based on some initial value?
This can work only for sequential IVs (or, better said, for predictable
IVs like plain64, implemented inside your hw/driver).

It cannot work for IVs/tweaks that are randomized or keyed (like ESSIV).
Yes, I know that using ESSIV for XTS is considered overkill but I do not
see you are checking this condition anywhere in code (we can definitely
configure such a mapping).

>>> So introduce 'CRYPTO_ALG_BULK' flag to indicate this cipher can support bulk
>>> mode.
>>
>> What exactly skcipher will do if this flag is set?
> 
> I think that depends on how to implement the cipher engine driver.

I do not care about implementation details, I just would like to see
some high-level description for the new API flag.
(Or at least read the code that implements it :)

>> Which drivers it should use? I do not see any posted patch that uses this 
>> flag yet.
>> How we can test it?
> 
> Some cipher engine drivers which support bulk mode should use this
> flag. Yeah, we need upstream one cipher driver with this flag for
> testing.

I think if you introduce new flag, you should also post drivers that uses it.
Otherwise it is just unused code for mainline.

And I definitely would like to test it somewhere and see real
performance data (not just simple dd).

Thanks,
Milan

> 
>>
>> Milan
>>
>>>
>>> Signed-off-by: Baolin Wang <baolin.w...@linaro.org>
>>> ---
>>>  include/crypto/skcipher.h |7 +++
>>>  include/linux/crypto.h|6 ++
>>>  2 files changed, 13 insertions(+)
>>>
>>> diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
>>> index 0f987f5..d89d29a 100644
>>> --- a/include/crypto/skcipher.h
>>> +++ b/include/crypto/skcipher.h
>>> @@ -519,5 +519,12 @@ static inline void skcipher_request_set_crypt(
>>>   req->iv = iv;
>>>  }
>>>
>>> +static inline unsigned int skcipher_is_bulk_mode(struct crypto_skcipher 
>>> *sk_tfm)
>>> +{
>>> + struct crypto_tfm *tfm = crypto_skcipher_tfm(sk_tfm);
>>> +
>>> + return crypto_tfm_alg_bulk(tfm);
>>> +}
>>> +
>>>  #endif   /* _CRYPTO_SKCIPHER_H */
>>>
>>> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
>>> index 6e28c89..a315487 100644
>>> --- a/include/linux/crypto.h
>>> +++ b/include/linux/crypto.h
>>> @@ -63,6 +63,7 @@
>>>  #define CRYPTO_ALG_DEAD  0x0020
>>>  #define CRYPTO_ALG_DYING 0x0040
>>>  #define CRYPTO_ALG_ASYNC 0x0080
>>> +#define CRYPTO_ALG_BULK  0x0100
>>>
>>>  /*
>>>   * Set this bit if and only if the algorithm requires another algorithm of
>>> @@ -623,6 +624,11 @@ static inline u32 crypto_tfm_alg_type(struct 
>>> crypto_tfm *tfm)
>>>   return tfm->__crt_alg->cra_flags & CRYPTO_ALG_TYPE_MASK;
>>>  }
>>>
>>> +static inline unsigned int crypto_tfm_alg_bulk(struct crypto_tfm *tfm)
>>> +{
>>> + return tfm->__crt_alg->cra_flags & CRYPTO_ALG_BULK;
>>> +}
>>> +
>>>  static inline unsigned int crypto_tfm_alg_blocksize(struct crypto_tfm *tfm)
>>>  {
>>>   return tfm->__crt_alg->cra_blocksize;
>>>
>>
> 
> 
> 

Re: [RFC 2/3] crypto: Introduce CRYPTO_ALG_BULK flag

[Milan Broz]

On 05/27/2016 09:04 AM, Baolin Wang wrote:
> Hi Milan,
> 
> On 27 May 2016 at 14:31, Milan Broz  wrote:
>> On 05/25/2016 08:12 AM, Baolin Wang wrote:
>>> Now some cipher hardware engines prefer to handle bulk block rather than one
>>> sector (512 bytes) created by dm-crypt, cause these cipher engines can 
>>> handle
>>> the intermediate values (IV) by themselves in one bulk block. This means we
>>> can increase the size of the request by merging request rather than always 
>>> 512
>>> bytes and thus increase the hardware engine processing speed.
>>
>> Hi,
>>
>> could you please elaborate how exactly you are processing independently
>> encrypted sectors? For example with XTS mode. Do you play internally with
>> tweak calculation? Does this keep 512 bytes sector encryption blocks 
>> independent?
>>
>> (If not, it is breaking compatibility everywhere and you are reinventing
>> disk encryption logic here - just for performance reason for some hw
>> not designed for this task... But that was said several times already.)
> 
> These are what the cipher hardware engine and engine driver should do,
> for software we just need send one initial IV and bulk data to crypto
> layer, which is enough.

Hi,

Thanks for answer.

So this is just doing some kind of batch processing optimization inside the 
driver?
I still do not understand why it is not possible to "batch" these requests 
inside
you driver (with async API) then and process them in one go without any changes
to dmcrypt though. (But I am not familiar with these drivers.)

If I understand it correctly, subsequent IVs are calculated inside
your driver just based on some initial value?
This can work only for sequential IVs (or, better said, for predictable
IVs like plain64, implemented inside your hw/driver).

It cannot work for IVs/tweaks that are randomized or keyed (like ESSIV).
Yes, I know that using ESSIV for XTS is considered overkill but I do not
see you are checking this condition anywhere in code (we can definitely
configure such a mapping).

>>> So introduce 'CRYPTO_ALG_BULK' flag to indicate this cipher can support bulk
>>> mode.
>>
>> What exactly skcipher will do if this flag is set?
> 
> I think that depends on how to implement the cipher engine driver.

I do not care about implementation details, I just would like to see
some high-level description for the new API flag.
(Or at least read the code that implements it :)

>> Which drivers it should use? I do not see any posted patch that uses this 
>> flag yet.
>> How we can test it?
> 
> Some cipher engine drivers which support bulk mode should use this
> flag. Yeah, we need upstream one cipher driver with this flag for
> testing.

I think if you introduce new flag, you should also post drivers that uses it.
Otherwise it is just unused code for mainline.

And I definitely would like to test it somewhere and see real
performance data (not just simple dd).

Thanks,
Milan

> 
>>
>> Milan
>>
>>>
>>> Signed-off-by: Baolin Wang 
>>> ---
>>>  include/crypto/skcipher.h |7 +++
>>>  include/linux/crypto.h|6 ++
>>>  2 files changed, 13 insertions(+)
>>>
>>> diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
>>> index 0f987f5..d89d29a 100644
>>> --- a/include/crypto/skcipher.h
>>> +++ b/include/crypto/skcipher.h
>>> @@ -519,5 +519,12 @@ static inline void skcipher_request_set_crypt(
>>>   req->iv = iv;
>>>  }
>>>
>>> +static inline unsigned int skcipher_is_bulk_mode(struct crypto_skcipher 
>>> *sk_tfm)
>>> +{
>>> + struct crypto_tfm *tfm = crypto_skcipher_tfm(sk_tfm);
>>> +
>>> + return crypto_tfm_alg_bulk(tfm);
>>> +}
>>> +
>>>  #endif   /* _CRYPTO_SKCIPHER_H */
>>>
>>> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
>>> index 6e28c89..a315487 100644
>>> --- a/include/linux/crypto.h
>>> +++ b/include/linux/crypto.h
>>> @@ -63,6 +63,7 @@
>>>  #define CRYPTO_ALG_DEAD  0x0020
>>>  #define CRYPTO_ALG_DYING 0x0040
>>>  #define CRYPTO_ALG_ASYNC 0x0080
>>> +#define CRYPTO_ALG_BULK  0x0100
>>>
>>>  /*
>>>   * Set this bit if and only if the algorithm requires another algorithm of
>>> @@ -623,6 +624,11 @@ static inline u32 crypto_tfm_alg_type(struct 
>>> crypto_tfm *tfm)
>>>   return tfm->__crt_alg->cra_flags & CRYPTO_ALG_TYPE_MASK;
>>>  }
>>>
>>> +static inline unsigned int crypto_tfm_alg_bulk(struct crypto_tfm *tfm)
>>> +{
>>> + return tfm->__crt_alg->cra_flags & CRYPTO_ALG_BULK;
>>> +}
>>> +
>>>  static inline unsigned int crypto_tfm_alg_blocksize(struct crypto_tfm *tfm)
>>>  {
>>>   return tfm->__crt_alg->cra_blocksize;
>>>
>>
> 
> 
> 

Re: [RFC 2/3] crypto: Introduce CRYPTO_ALG_BULK flag

[Milan Broz]

On 05/25/2016 08:12 AM, Baolin Wang wrote:
> Now some cipher hardware engines prefer to handle bulk block rather than one
> sector (512 bytes) created by dm-crypt, cause these cipher engines can handle
> the intermediate values (IV) by themselves in one bulk block. This means we
> can increase the size of the request by merging request rather than always 512
> bytes and thus increase the hardware engine processing speed.

Hi,

could you please elaborate how exactly you are processing independently
encrypted sectors? For example with XTS mode. Do you play internally with
tweak calculation? Does this keep 512 bytes sector encryption blocks 
independent?

(If not, it is breaking compatibility everywhere and you are reinventing
disk encryption logic here - just for performance reason for some hw
not designed for this task... But that was said several times already.)

> So introduce 'CRYPTO_ALG_BULK' flag to indicate this cipher can support bulk
> mode.

What exactly skcipher will do if this flag is set?

Which drivers it should use? I do not see any posted patch that uses this flag 
yet.
How we can test it?

Milan

> 
> Signed-off-by: Baolin Wang 
> ---
>  include/crypto/skcipher.h |7 +++
>  include/linux/crypto.h|6 ++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
> index 0f987f5..d89d29a 100644
> --- a/include/crypto/skcipher.h
> +++ b/include/crypto/skcipher.h
> @@ -519,5 +519,12 @@ static inline void skcipher_request_set_crypt(
>   req->iv = iv;
>  }
>  
> +static inline unsigned int skcipher_is_bulk_mode(struct crypto_skcipher 
> *sk_tfm)
> +{
> + struct crypto_tfm *tfm = crypto_skcipher_tfm(sk_tfm);
> +
> + return crypto_tfm_alg_bulk(tfm);
> +}
> +
>  #endif   /* _CRYPTO_SKCIPHER_H */
>  
> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> index 6e28c89..a315487 100644
> --- a/include/linux/crypto.h
> +++ b/include/linux/crypto.h
> @@ -63,6 +63,7 @@
>  #define CRYPTO_ALG_DEAD  0x0020
>  #define CRYPTO_ALG_DYING 0x0040
>  #define CRYPTO_ALG_ASYNC 0x0080
> +#define CRYPTO_ALG_BULK  0x0100
>  
>  /*
>   * Set this bit if and only if the algorithm requires another algorithm of
> @@ -623,6 +624,11 @@ static inline u32 crypto_tfm_alg_type(struct crypto_tfm 
> *tfm)
>   return tfm->__crt_alg->cra_flags & CRYPTO_ALG_TYPE_MASK;
>  }
>  
> +static inline unsigned int crypto_tfm_alg_bulk(struct crypto_tfm *tfm)
> +{
> + return tfm->__crt_alg->cra_flags & CRYPTO_ALG_BULK;
> +}
> +
>  static inline unsigned int crypto_tfm_alg_blocksize(struct crypto_tfm *tfm)
>  {
>   return tfm->__crt_alg->cra_blocksize;
> 

Re: [RFC 2/3] crypto: Introduce CRYPTO_ALG_BULK flag

[Milan Broz]

On 05/25/2016 08:12 AM, Baolin Wang wrote:
> Now some cipher hardware engines prefer to handle bulk block rather than one
> sector (512 bytes) created by dm-crypt, cause these cipher engines can handle
> the intermediate values (IV) by themselves in one bulk block. This means we
> can increase the size of the request by merging request rather than always 512
> bytes and thus increase the hardware engine processing speed.

Hi,

could you please elaborate how exactly you are processing independently
encrypted sectors? For example with XTS mode. Do you play internally with
tweak calculation? Does this keep 512 bytes sector encryption blocks 
independent?

(If not, it is breaking compatibility everywhere and you are reinventing
disk encryption logic here - just for performance reason for some hw
not designed for this task... But that was said several times already.)

> So introduce 'CRYPTO_ALG_BULK' flag to indicate this cipher can support bulk
> mode.

What exactly skcipher will do if this flag is set?

Which drivers it should use? I do not see any posted patch that uses this flag 
yet.
How we can test it?

Milan

> 
> Signed-off-by: Baolin Wang 
> ---
>  include/crypto/skcipher.h |7 +++
>  include/linux/crypto.h|6 ++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
> index 0f987f5..d89d29a 100644
> --- a/include/crypto/skcipher.h
> +++ b/include/crypto/skcipher.h
> @@ -519,5 +519,12 @@ static inline void skcipher_request_set_crypt(
>   req->iv = iv;
>  }
>  
> +static inline unsigned int skcipher_is_bulk_mode(struct crypto_skcipher 
> *sk_tfm)
> +{
> + struct crypto_tfm *tfm = crypto_skcipher_tfm(sk_tfm);
> +
> + return crypto_tfm_alg_bulk(tfm);
> +}
> +
>  #endif   /* _CRYPTO_SKCIPHER_H */
>  
> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> index 6e28c89..a315487 100644
> --- a/include/linux/crypto.h
> +++ b/include/linux/crypto.h
> @@ -63,6 +63,7 @@
>  #define CRYPTO_ALG_DEAD  0x0020
>  #define CRYPTO_ALG_DYING 0x0040
>  #define CRYPTO_ALG_ASYNC 0x0080
> +#define CRYPTO_ALG_BULK  0x0100
>  
>  /*
>   * Set this bit if and only if the algorithm requires another algorithm of
> @@ -623,6 +624,11 @@ static inline u32 crypto_tfm_alg_type(struct crypto_tfm 
> *tfm)
>   return tfm->__crt_alg->cra_flags & CRYPTO_ALG_TYPE_MASK;
>  }
>  
> +static inline unsigned int crypto_tfm_alg_bulk(struct crypto_tfm *tfm)
> +{
> + return tfm->__crt_alg->cra_flags & CRYPTO_ALG_BULK;
> +}
> +
>  static inline unsigned int crypto_tfm_alg_blocksize(struct crypto_tfm *tfm)
>  {
>   return tfm->__crt_alg->cra_blocksize;
> 

Re: [PATCH v2 0/4] Introduce bulk mode for crypto engine framework

[Milan Broz]

On 04/18/2016 03:36 PM, Mike Snitzer wrote:
> On Mon, Apr 18 2016 at  1:31am -0400,
> Baolin Wang <baolin.w...@linaro.org> wrote:
> 
>> Hi Herbert,
>>
>> On 15 April 2016 at 21:48, Herbert Xu <herb...@gondor.apana.org.au> wrote:
>>> On Tue, Mar 15, 2016 at 03:47:58PM +0800, Baolin Wang wrote:
>>>> Now some cipher hardware engines prefer to handle bulk block by merging 
>>>> requests
>>>> to increase the block size and thus increase the hardware engine 
>>>> processing speed.
>>>>
>>>> This patchset introduces request bulk mode to help the crypto hardware 
>>>> drivers
>>>> improve in efficiency.
>>>
>>> Could you please explain why this merging can't be done in dm-crypt
>>> instead?
>>
>> We've tried to do this in dm-crypt, but it failed.
>> The dm-crypt maintainer explained to me that I should optimize the
>> driver, not add strange hw-dependent crypto modes to dm-crypt, this is
>> not the first crypto accelerator that is just not suited for this kind
>> of use.
> 
> As a DM mainatiner my only contribution to this line of discussion was
> relative to your proposal to train the dm-crypt target (which is
> bio-based) to also provide request-based support, see:
> https://www.redhat.com/archives/dm-devel/2015-November/msg00112.html
> 
> But follow-up discussion occured, primarily with Milan Broz, which led
> to this bulk mode support in the crypto layer.  Pretty strange Milan
> wasn't cc'd on your patchset posting (I've now cc'd him).

My complaint was mainly that the proposed dmcrypt based version just did
not work properly.
https://lkml.org/lkml/2016/1/2/109

(I did not test the new version we are replying here. I wonder how the problem
I mentioned is fixed though.
Also see Mikulas' comments 
https://www.redhat.com/archives/dm-devel/2016-January/msg00145.html)
 
Anyway, all this seems to optimize case for specific crypto hw, that
is not able to perform optimally with pattern dmcrypt produces.

I do not think dmcrypt should do optimizations for specific hw.

But if we decide that it is needed there, it should not cause any performance
or compatibility problems elsewhere...

Milan

Re: [PATCH v2 0/4] Introduce bulk mode for crypto engine framework

[Milan Broz]

On 04/18/2016 03:36 PM, Mike Snitzer wrote:
> On Mon, Apr 18 2016 at  1:31am -0400,
> Baolin Wang  wrote:
> 
>> Hi Herbert,
>>
>> On 15 April 2016 at 21:48, Herbert Xu  wrote:
>>> On Tue, Mar 15, 2016 at 03:47:58PM +0800, Baolin Wang wrote:
>>>> Now some cipher hardware engines prefer to handle bulk block by merging 
>>>> requests
>>>> to increase the block size and thus increase the hardware engine 
>>>> processing speed.
>>>>
>>>> This patchset introduces request bulk mode to help the crypto hardware 
>>>> drivers
>>>> improve in efficiency.
>>>
>>> Could you please explain why this merging can't be done in dm-crypt
>>> instead?
>>
>> We've tried to do this in dm-crypt, but it failed.
>> The dm-crypt maintainer explained to me that I should optimize the
>> driver, not add strange hw-dependent crypto modes to dm-crypt, this is
>> not the first crypto accelerator that is just not suited for this kind
>> of use.
> 
> As a DM mainatiner my only contribution to this line of discussion was
> relative to your proposal to train the dm-crypt target (which is
> bio-based) to also provide request-based support, see:
> https://www.redhat.com/archives/dm-devel/2015-November/msg00112.html
> 
> But follow-up discussion occured, primarily with Milan Broz, which led
> to this bulk mode support in the crypto layer.  Pretty strange Milan
> wasn't cc'd on your patchset posting (I've now cc'd him).

My complaint was mainly that the proposed dmcrypt based version just did
not work properly.
https://lkml.org/lkml/2016/1/2/109

(I did not test the new version we are replying here. I wonder how the problem
I mentioned is fixed though.
Also see Mikulas' comments 
https://www.redhat.com/archives/dm-devel/2016-January/msg00145.html)
 
Anyway, all this seems to optimize case for specific crypto hw, that
is not able to perform optimally with pattern dmcrypt produces.

I do not think dmcrypt should do optimizations for specific hw.

But if we decide that it is needed there, it should not cause any performance
or compatibility problems elsewhere...

Milan

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_nok

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_nok

Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

[Milan Broz]

On 01/04/2016 05:35 AM, Herbert Xu wrote:
> On Sun, Jan 03, 2016 at 10:42:28AM +0100, Milan Broz wrote:
>>
>> yes, basically it prepares socket()/bind()/accept() and then it calls setkey 
>> once.
>> (I'll try to fix in next releases to call setkey first though.)
> 
> OK please try these two patches (warning, totally untested).

Well, it is not much better.

I had to apply another two patches that are not mentioned and are not in your 
tree yet
before it:
  crypto: af_alg - Disallow bind_setkey_... after accept(2)
  crypto: use-after-free in alg_bind

then it at least compiles correctly.

skcipher works, But I still see two compatibility problems:

- hmac() is now failing the same way (SETKEY after accept())
(I initially tested without two patches above, these are not in linux-next yet.)
This breaks all cryptsetup TrueCrypt support (and moreover all systems if
kernel crypto API is set as a default vcrypto backend - but that's not default).

- cipher_null before worked without setkey, now it requires to set key
(either before or after accept().
This was actually probably bad workaround in cryptsetup, anyway it will now 
cause
old cryptsetup-reencrypt tool failing with your patches.
(Try "cryptsetup benchmark -c cipher_null-ecb". I am not sure what to do here,
but not requiring setkey for "cipher" that has no key internally seems ok for 
me...)

As I said, I'll fix it in cryptsetup upstream but we are breaking  a lot of 
existing systems.
Isn't there better way, how to fix it?

Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

[Milan Broz]

On 01/04/2016 05:35 AM, Herbert Xu wrote:
> On Sun, Jan 03, 2016 at 10:42:28AM +0100, Milan Broz wrote:
>>
>> yes, basically it prepares socket()/bind()/accept() and then it calls setkey 
>> once.
>> (I'll try to fix in next releases to call setkey first though.)
> 
> OK please try these two patches (warning, totally untested).

Well, it is not much better.

I had to apply another two patches that are not mentioned and are not in your 
tree yet
before it:
  crypto: af_alg - Disallow bind_setkey_... after accept(2)
  crypto: use-after-free in alg_bind

then it at least compiles correctly.

skcipher works, But I still see two compatibility problems:

- hmac() is now failing the same way (SETKEY after accept())
(I initially tested without two patches above, these are not in linux-next yet.)
This breaks all cryptsetup TrueCrypt support (and moreover all systems if
kernel crypto API is set as a default vcrypto backend - but that's not default).

- cipher_null before worked without setkey, now it requires to set key
(either before or after accept().
This was actually probably bad workaround in cryptsetup, anyway it will now 
cause
old cryptsetup-reencrypt tool failing with your patches.
(Try "cryptsetup benchmark -c cipher_null-ecb". I am not sure what to do here,
but not requiring setkey for "cipher" that has no key internally seems ok for 
me...)

As I said, I'll fix it in cryptsetup upstream but we are breaking  a lot of 
existing systems.
Isn't there better way, how to fix it?

Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: next-20151231 - aes crypto algorithm went missing?

[Milan Broz]

On 01/03/2016 06:34 AM, Valdis Kletnieks wrote:
> So booting into a next-20151222 kernel, I can mount an external drive
> that uses cryptLuks.  I try -1231, and I get this failure:
> 
> Failed to setup dm-crypt key mapping for device /dev/sdb2.
> Check that kernel supports aes-cbc-essiv:sha256 cipher (check syslog for more 
> info).
> 
> Tracked it down to this difference in /proc/crypto between the 12/22 and 
> 12/31:

...
> This ringing any bells, before I start the New Year with a bisect? :)

Perhaps see the discussion in thread
[PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

Could you try to revert this patch?
http://git.kernel.org/cgit/linux/kernel/git/next/linux-next-history.git/commit/crypto?id=9f47e11b9e3169ff4cb35b3cdac0e0f7c2fcfe27

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

[Milan Broz]

On 01/03/2016 02:31 AM, Herbert Xu wrote:
> On Sat, Jan 02, 2016 at 09:18:30PM +0100, Milan Broz wrote:
>>
>> But I cannot change thousands of cryptsetup installations that are actively 
>> using that code.
>> This is clear userspace breakage which should not happen this way.
> 
> I'll try to add some compatibility code for your case, assuming
> your modus operandi is accept(2) followed by a single setkey before
> proceeding to encryption/decryption.

Hi,

yes, basically it prepares socket()/bind()/accept() and then it calls setkey 
once.
(I'll try to fix in next releases to call setkey first though.)

I am doing exactly the same for AF_ALG HMAC (hmac() key,
does this requirement for order if accept/setkey applies there as well?
(It is not enforced yet.)

Anyway, you can easily simulate that skcipher API call just with running 
"cryptsetup benchmark"
(with accept() patch it will print N/A for all ciphers while without patch it 
measures some
more-or-less magic performance numbers :)

> 
>> (Moreover it still doesn't work for cipher_null that has min/max key size 0.)
> 
> Setkey works just fine on cipher_null.

Yes, it works if ALG_SET_KEY is set to zero-length key.
I just re-introduced old bug to code, sorry.

Thanks!
Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

[Milan Broz]

On 01/03/2016 02:31 AM, Herbert Xu wrote:
> On Sat, Jan 02, 2016 at 09:18:30PM +0100, Milan Broz wrote:
>>
>> But I cannot change thousands of cryptsetup installations that are actively 
>> using that code.
>> This is clear userspace breakage which should not happen this way.
> 
> I'll try to add some compatibility code for your case, assuming
> your modus operandi is accept(2) followed by a single setkey before
> proceeding to encryption/decryption.

Hi,

yes, basically it prepares socket()/bind()/accept() and then it calls setkey 
once.
(I'll try to fix in next releases to call setkey first though.)

I am doing exactly the same for AF_ALG HMAC (hmac() key,
does this requirement for order if accept/setkey applies there as well?
(It is not enforced yet.)

Anyway, you can easily simulate that skcipher API call just with running 
"cryptsetup benchmark"
(with accept() patch it will print N/A for all ciphers while without patch it 
measures some
more-or-less magic performance numbers :)

> 
>> (Moreover it still doesn't work for cipher_null that has min/max key size 0.)
> 
> Setkey works just fine on cipher_null.

Yes, it works if ALG_SET_KEY is set to zero-length key.
I just re-introduced old bug to code, sorry.

Thanks!
Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: next-20151231 - aes crypto algorithm went missing?

[Milan Broz]

On 01/03/2016 06:34 AM, Valdis Kletnieks wrote:
> So booting into a next-20151222 kernel, I can mount an external drive
> that uses cryptLuks.  I try -1231, and I get this failure:
> 
> Failed to setup dm-crypt key mapping for device /dev/sdb2.
> Check that kernel supports aes-cbc-essiv:sha256 cipher (check syslog for more 
> info).
> 
> Tracked it down to this difference in /proc/crypto between the 12/22 and 
> 12/31:

...
> This ringing any bells, before I start the New Year with a bisect? :)

Perhaps see the discussion in thread
[PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

Could you try to revert this patch?
http://git.kernel.org/cgit/linux/kernel/git/next/linux-next-history.git/commit/crypto?id=9f47e11b9e3169ff4cb35b3cdac0e0f7c2fcfe27

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/2] Introduce the bulk IV mode for improving the crypto engine efficiency

[Milan Broz]

On 12/17/2015 08:37 AM, Baolin Wang wrote:
> Hi Milan,
> 
> On 16 December 2015 at 16:08, Milan Broz  wrote:
>> On 12/16/2015 04:18 AM, Baolin Wang wrote:
>>> From the dm-crypt performance report, we found it shows low efficiency
>>> with crypto engine for some mode (like ecb or xts mode). Because in dm
>>> crypt, it will map the IO data buffer with scatterlist, and send the
>>> scatterlist of one bio to the encryption engine, if send more scatterlists
>>> with bigger size at one time, that helps the engine palys best performance,
>>> which means a high encryption speed.
>>>
>>> But now the dm-crypt only map one segment (always one sector) of one bio
>>> with one scatterlist to the crypto engine at one time. which is more
>>> time-consuming and ineffective for the crypto engine. Especially for some
>>> modes which don't need different IV for each sector, we can map the whole
>>> bio with multiple scatterlists to improve the engine performance.
>>>
>>> But this optimization is not support some ciphers and IV modes which should
>>> do sector by sector and need different IV for each sector.
>>>
>>> Change since v1:
>>>  - Introduce one different IV mode.
>>>  - Change the conditions for bulk mode.
>>
>> I tried the patchset on 32bit Intel VM and kernel immediately OOPsed (just 
>> tried aes-ecb)...
>>
> 
> I've checked the code and I guess some macros I used with different
> definitions on different arch. Could you please try the new patchset
> with some optimization on your platform? It can work well on my arm
> board. Thanks.

Sorry for delay, I tried to compile it.
It doesn't crash now, but it also does not work.

You usage of IV in XTS mode is not correct - it cannot just work this way,
you have to initialize IV after each block. And just one write not aligned
to your large XTS block will corrupt it.

Did you tried to _read_ data you write to the device?

See this test :

# create  device with your patch
$ echo "test"|cryptsetup create -s 512 -c aes-xts-bulk tst /dev/sdg

# prepare random test file
$ dd if=/dev/urandom of=/src.img bs=1M count=16

# now copy the file to the plaintext device and drop caches
$ dd if=/src.img of=/dev/mapper/tst bs=1M count=16

$ echo 3 > /proc/sys/vm/drop_caches

# and verify that we are (not) reading the same data ...

$ dd if=/dev/mapper/tst of=/dst1.img bs=1M count=16

$ sha256sum /src.img /dst1.img 
5401119fa9975bbeebac58e0b2598bc87247a29e62417f9f58fe200b531602ad  /src.img
e9bf5efa95031fdb5adf618db141f48ed23f71b12c017b8a0cbe0a694f18b979  /dst1.img

(I think only first page-sized block is correct, because without direct-io
it writes in page-sized IOs.)


... or just try to mkfs and mount it
$ mkfs -t ext4  /dev/mapper/tst 

mke2fs 1.42.13 (17-May-2015)
Creating filesystem with 262144 4k blocks and 65536 inodes
...

$ mount /dev/mapper/tst /mnt/tst
mount: wrong fs type, bad option, bad superblock on /dev/mapper/tst,
   missing codepage or helper program, or other error


You approach simply does not work. (It will probably work for ECB mode but it is
unusable in real world.)


Anyway, I think that you should optimize driver, not add strange hw-dependent
crypto modes to dmcrypt. This is not the first crypto accelerator that is just 
not
suited for this kind of use.

(If it can process batch of chunks of data each with own IV, then it can work
with dmcrypt, but I think such optimized code should be inside crypto API,
not in dmcrypt.)

Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

[Milan Broz]

On 01/02/2016 09:03 PM, Stephan Mueller wrote:
> Am Samstag, 2. Januar 2016, 15:41:34 schrieb Milan Broz:
> 
> Hi Milan,
> 

...
>>> Hi Herbert,
>>>
>>> this patch breaks userspace in cryptsetup...
>>>
>>> We use algif_skcipher in cryptsetup (for years, even before
>>> there was Stephan's library) and with this patch applied
>>> I see fail in ALG_SET_IV call (patch from your git).
>>
>> (Obviously this was because of failing accept() call here, not set_iv.)
>>
>>> I can fix it upstream, but for thousands of installations it will
>>> be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
>>> it will be unusable. Also people who configured kernel crypto API as
>>> default backend will have non-working cryptsetup).
>>>
>>> Is it really thing for stable branch?
>>
>> Also how it is supposed to work for cipher_null, where there is no key?
>> Why it should call set_key if it is noop? (and set key length 0 is not
>> possible).
>>
>> (We are using cipher_null for testing and for offline re-encryption tool
>> to create temporary "fake" header for not-yet encrypted device...)
> 
> The change implies that any setkey or set IV operations (i.e. any operations 
> on the tfmfd) are done before the opfd(s) are created with one or more accept 
> calls.
> 
> Thus, after a bind that returns the tfmfd, the setkey and setiv operations 
> shall be called. This is followed by accept. If you change the order of 
> invocations in your code, it should work.

Hi,

I already changed it in cryptsetup upstream this way.

But I cannot change thousands of cryptsetup installations that are actively 
using that code.
This is clear userspace breakage which should not happen this way.

(Moreover it still doesn't work for cipher_null that has min/max key size 0.)

Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

[Milan Broz]

On 01/02/2016 12:52 PM, Milan Broz wrote:
> On 12/25/2015 08:40 AM, Herbert Xu wrote:
>> Dmitry Vyukov  wrote:
>>>
>>> I am testing with your two patches:
>>> crypto: algif_skcipher - Use new skcipher interface
>>> crypto: algif_skcipher - Require setkey before accept(2)
>>> on top of a88164345b81292b55a8d4829fdd35c8d611cd7d (Dec 23).
>>
>> You sent the email to everyone on the original CC list except me.
>> Please don't do that.
>>
>>> Now the following program causes a bunch of use-after-frees and them
>>> kills kernel:
>>
>> Yes there is an obvious bug in the patch that Julia Lawall has
>> responded to in another thread.  Here is a fixed version.
>>
>> ---8<--
>> 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.
> 
> 
> Hi Herbert,
> 
> this patch breaks userspace in cryptsetup...
> 
> We use algif_skcipher in cryptsetup (for years, even before
> there was Stephan's library) and with this patch applied
> I see fail in ALG_SET_IV call (patch from your git).

(Obviously this was because of failing accept() call here, not set_iv.)

> 
> I can fix it upstream, but for thousands of installations it will
> be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
> it will be unusable. Also people who configured kernel crypto API as default
> backend will have non-working cryptsetup).
> 
> Is it really thing for stable branch?

Also how it is supposed to work for cipher_null, where there is no key?
Why it should call set_key if it is noop? (and set key length 0 is not 
possible).

(We are using cipher_null for testing and for offline re-encryption tool
to create temporary "fake" header for not-yet encrypted device...)

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

[Milan Broz]

On 12/25/2015 08:40 AM, Herbert Xu wrote:
> Dmitry Vyukov  wrote:
>>
>> I am testing with your two patches:
>> crypto: algif_skcipher - Use new skcipher interface
>> crypto: algif_skcipher - Require setkey before accept(2)
>> on top of a88164345b81292b55a8d4829fdd35c8d611cd7d (Dec 23).
> 
> You sent the email to everyone on the original CC list except me.
> Please don't do that.
> 
>> Now the following program causes a bunch of use-after-frees and them
>> kills kernel:
> 
> Yes there is an obvious bug in the patch that Julia Lawall has
> responded to in another thread.  Here is a fixed version.
> 
> ---8<--
> 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.


Hi Herbert,

this patch breaks userspace in cryptsetup...

We use algif_skcipher in cryptsetup (for years, even before
there was Stephan's library) and with this patch applied
I see fail in ALG_SET_IV call (patch from your git).

I can fix it upstream, but for thousands of installations it will
be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
it will be unusable. Also people who configured kernel crypto API as default
backend will have non-working cryptsetup).

Is it really thing for stable branch?

Milan

> 
> Cc: sta...@vger.kernel.org
> Reported-by: Dmitry Vyukov 
> Signed-off-by: Herbert Xu 
> 
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index 5c756b3..f4431bc 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_skcipher *skcipher;
> + bool has_key;
> +};
> +
>  struct skcipher_ctx {
>   struct list_head tsgl;
>   struct af_alg_sgl rsgl;
> @@ -750,17 +755,41 @@ static struct proto_ops algif_skcipher_ops = {
>  
>  static void *skcipher_bind(const char *name, u32 type, u32 mask)
>  {
> - return crypto_alloc_skcipher(name, type, mask);
> + struct skcipher_tfm *tfm;
> + struct crypto_skcipher *skcipher;
> +
> + tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
> + if (!tfm)
> + return ERR_PTR(-ENOMEM);
> +
> + skcipher = crypto_alloc_skcipher(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_skcipher(private);
> + struct skcipher_tfm *tfm = private;
> +
> + crypto_free_skcipher(tfm->skcipher);
> + kfree(tfm);
>  }
>  
>  static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
>  {
> - return crypto_skcipher_setkey(private, key, keylen);
> + struct skcipher_tfm *tfm = private;
> + int err;
> +
> + err = crypto_skcipher_setkey(tfm->skcipher, key, keylen);
> + tfm->has_key = !err;
> +
> + return err;
>  }
>  
>  static void skcipher_wait(struct sock *sk)
> @@ -792,20 +821,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_skcipher_reqsize(private);
> + struct skcipher_tfm *tfm = private;
> + struct crypto_skcipher *skcipher = tfm->skcipher;
> + unsigned int len = sizeof(*ctx) + crypto_skcipher_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_skcipher_ivsize(private),
> + ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(skcipher),
>  GFP_KERNEL);
>   if (!ctx->iv) {
>   sock_kfree_s(sk, ctx, len);
>   return -ENOMEM;
>   }
>  
> - memset(ctx->iv, 0, crypto_skcipher_ivsize(private));
> + memset(ctx->iv, 0, crypto_skcipher_ivsize(skcipher));
>  
>   INIT_LIST_HEAD(>tsgl);
>   ctx->len = len;
> @@ -818,7 +852,7 @@ static int skcipher_accept_parent(void *private, struct 
> sock *sk)
>  
>   ask->private = ctx;
>  
> - skcipher_request_set_tfm(>req, private);
> + skcipher_request_set_tfm(>req, skcipher);
>   skcipher_request_set_callback(>req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> af_alg_complete, >completion);
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

[Milan Broz]

On 01/02/2016 12:52 PM, Milan Broz wrote:
> On 12/25/2015 08:40 AM, Herbert Xu wrote:
>> Dmitry Vyukov <dvyu...@google.com> wrote:
>>>
>>> I am testing with your two patches:
>>> crypto: algif_skcipher - Use new skcipher interface
>>> crypto: algif_skcipher - Require setkey before accept(2)
>>> on top of a88164345b81292b55a8d4829fdd35c8d611cd7d (Dec 23).
>>
>> You sent the email to everyone on the original CC list except me.
>> Please don't do that.
>>
>>> Now the following program causes a bunch of use-after-frees and them
>>> kills kernel:
>>
>> Yes there is an obvious bug in the patch that Julia Lawall has
>> responded to in another thread.  Here is a fixed version.
>>
>> ---8<--
>> 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.
> 
> 
> Hi Herbert,
> 
> this patch breaks userspace in cryptsetup...
> 
> We use algif_skcipher in cryptsetup (for years, even before
> there was Stephan's library) and with this patch applied
> I see fail in ALG_SET_IV call (patch from your git).

(Obviously this was because of failing accept() call here, not set_iv.)

> 
> I can fix it upstream, but for thousands of installations it will
> be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
> it will be unusable. Also people who configured kernel crypto API as default
> backend will have non-working cryptsetup).
> 
> Is it really thing for stable branch?

Also how it is supposed to work for cipher_null, where there is no key?
Why it should call set_key if it is noop? (and set key length 0 is not 
possible).

(We are using cipher_null for testing and for offline re-encryption tool
to create temporary "fake" header for not-yet encrypted device...)

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

[Milan Broz]

On 12/25/2015 08:40 AM, Herbert Xu wrote:
> Dmitry Vyukov  wrote:
>>
>> I am testing with your two patches:
>> crypto: algif_skcipher - Use new skcipher interface
>> crypto: algif_skcipher - Require setkey before accept(2)
>> on top of a88164345b81292b55a8d4829fdd35c8d611cd7d (Dec 23).
> 
> You sent the email to everyone on the original CC list except me.
> Please don't do that.
> 
>> Now the following program causes a bunch of use-after-frees and them
>> kills kernel:
> 
> Yes there is an obvious bug in the patch that Julia Lawall has
> responded to in another thread.  Here is a fixed version.
> 
> ---8<--
> 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.


Hi Herbert,

this patch breaks userspace in cryptsetup...

We use algif_skcipher in cryptsetup (for years, even before
there was Stephan's library) and with this patch applied
I see fail in ALG_SET_IV call (patch from your git).

I can fix it upstream, but for thousands of installations it will
be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
it will be unusable. Also people who configured kernel crypto API as default
backend will have non-working cryptsetup).

Is it really thing for stable branch?

Milan

> 
> Cc: sta...@vger.kernel.org
> Reported-by: Dmitry Vyukov 
> Signed-off-by: Herbert Xu 
> 
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index 5c756b3..f4431bc 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_skcipher *skcipher;
> + bool has_key;
> +};
> +
>  struct skcipher_ctx {
>   struct list_head tsgl;
>   struct af_alg_sgl rsgl;
> @@ -750,17 +755,41 @@ static struct proto_ops algif_skcipher_ops = {
>  
>  static void *skcipher_bind(const char *name, u32 type, u32 mask)
>  {
> - return crypto_alloc_skcipher(name, type, mask);
> + struct skcipher_tfm *tfm;
> + struct crypto_skcipher *skcipher;
> +
> + tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
> + if (!tfm)
> + return ERR_PTR(-ENOMEM);
> +
> + skcipher = crypto_alloc_skcipher(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_skcipher(private);
> + struct skcipher_tfm *tfm = private;
> +
> + crypto_free_skcipher(tfm->skcipher);
> + kfree(tfm);
>  }
>  
>  static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
>  {
> - return crypto_skcipher_setkey(private, key, keylen);
> + struct skcipher_tfm *tfm = private;
> + int err;
> +
> + err = crypto_skcipher_setkey(tfm->skcipher, key, keylen);
> + tfm->has_key = !err;
> +
> + return err;
>  }
>  
>  static void skcipher_wait(struct sock *sk)
> @@ -792,20 +821,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_skcipher_reqsize(private);
> + struct skcipher_tfm *tfm = private;
> + struct crypto_skcipher *skcipher = tfm->skcipher;
> + unsigned int len = sizeof(*ctx) + crypto_skcipher_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_skcipher_ivsize(private),
> + ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(skcipher),
>  GFP_KERNEL);
>   if (!ctx->iv) {
>   sock_kfree_s(sk, ctx, len);
>   return -ENOMEM;
>   }
>  
> - memset(ctx->iv, 0, crypto_skcipher_ivsize(private));
> + memset(ctx->iv, 0, crypto_skcipher_ivsize(skcipher));
>  
>   INIT_LIST_HEAD(>tsgl);
>   ctx->len = len;
> @@ -818,7 +852,7 @@ static int skcipher_accept_parent(void *private, struct 
> sock *sk)
>  
>   ask->private = ctx;
>  
> - skcipher_request_set_tfm(>req, private);
> + skcipher_request_set_tfm(>req, skcipher);
>   skcipher_request_set_callback(>req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> af_alg_complete, >completion);
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

[Milan Broz]

On 01/02/2016 09:03 PM, Stephan Mueller wrote:
> Am Samstag, 2. Januar 2016, 15:41:34 schrieb Milan Broz:
> 
> Hi Milan,
> 

...
>>> Hi Herbert,
>>>
>>> this patch breaks userspace in cryptsetup...
>>>
>>> We use algif_skcipher in cryptsetup (for years, even before
>>> there was Stephan's library) and with this patch applied
>>> I see fail in ALG_SET_IV call (patch from your git).
>>
>> (Obviously this was because of failing accept() call here, not set_iv.)
>>
>>> I can fix it upstream, but for thousands of installations it will
>>> be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
>>> it will be unusable. Also people who configured kernel crypto API as
>>> default backend will have non-working cryptsetup).
>>>
>>> Is it really thing for stable branch?
>>
>> Also how it is supposed to work for cipher_null, where there is no key?
>> Why it should call set_key if it is noop? (and set key length 0 is not
>> possible).
>>
>> (We are using cipher_null for testing and for offline re-encryption tool
>> to create temporary "fake" header for not-yet encrypted device...)
> 
> The change implies that any setkey or set IV operations (i.e. any operations 
> on the tfmfd) are done before the opfd(s) are created with one or more accept 
> calls.
> 
> Thus, after a bind that returns the tfmfd, the setkey and setiv operations 
> shall be called. This is followed by accept. If you change the order of 
> invocations in your code, it should work.

Hi,

I already changed it in cryptsetup upstream this way.

But I cannot change thousands of cryptsetup installations that are actively 
using that code.
This is clear userspace breakage which should not happen this way.

(Moreover it still doesn't work for cipher_null that has min/max key size 0.)

Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/2] Introduce the bulk IV mode for improving the crypto engine efficiency

[Milan Broz]

On 12/17/2015 08:37 AM, Baolin Wang wrote:
> Hi Milan,
> 
> On 16 December 2015 at 16:08, Milan Broz <gmazyl...@gmail.com> wrote:
>> On 12/16/2015 04:18 AM, Baolin Wang wrote:
>>> From the dm-crypt performance report, we found it shows low efficiency
>>> with crypto engine for some mode (like ecb or xts mode). Because in dm
>>> crypt, it will map the IO data buffer with scatterlist, and send the
>>> scatterlist of one bio to the encryption engine, if send more scatterlists
>>> with bigger size at one time, that helps the engine palys best performance,
>>> which means a high encryption speed.
>>>
>>> But now the dm-crypt only map one segment (always one sector) of one bio
>>> with one scatterlist to the crypto engine at one time. which is more
>>> time-consuming and ineffective for the crypto engine. Especially for some
>>> modes which don't need different IV for each sector, we can map the whole
>>> bio with multiple scatterlists to improve the engine performance.
>>>
>>> But this optimization is not support some ciphers and IV modes which should
>>> do sector by sector and need different IV for each sector.
>>>
>>> Change since v1:
>>>  - Introduce one different IV mode.
>>>  - Change the conditions for bulk mode.
>>
>> I tried the patchset on 32bit Intel VM and kernel immediately OOPsed (just 
>> tried aes-ecb)...
>>
> 
> I've checked the code and I guess some macros I used with different
> definitions on different arch. Could you please try the new patchset
> with some optimization on your platform? It can work well on my arm
> board. Thanks.

Sorry for delay, I tried to compile it.
It doesn't crash now, but it also does not work.

You usage of IV in XTS mode is not correct - it cannot just work this way,
you have to initialize IV after each block. And just one write not aligned
to your large XTS block will corrupt it.

Did you tried to _read_ data you write to the device?

See this test :

# create  device with your patch
$ echo "test"|cryptsetup create -s 512 -c aes-xts-bulk tst /dev/sdg

# prepare random test file
$ dd if=/dev/urandom of=/src.img bs=1M count=16

# now copy the file to the plaintext device and drop caches
$ dd if=/src.img of=/dev/mapper/tst bs=1M count=16

$ echo 3 > /proc/sys/vm/drop_caches

# and verify that we are (not) reading the same data ...

$ dd if=/dev/mapper/tst of=/dst1.img bs=1M count=16

$ sha256sum /src.img /dst1.img 
5401119fa9975bbeebac58e0b2598bc87247a29e62417f9f58fe200b531602ad  /src.img
e9bf5efa95031fdb5adf618db141f48ed23f71b12c017b8a0cbe0a694f18b979  /dst1.img

(I think only first page-sized block is correct, because without direct-io
it writes in page-sized IOs.)


... or just try to mkfs and mount it
$ mkfs -t ext4  /dev/mapper/tst 

mke2fs 1.42.13 (17-May-2015)
Creating filesystem with 262144 4k blocks and 65536 inodes
...

$ mount /dev/mapper/tst /mnt/tst
mount: wrong fs type, bad option, bad superblock on /dev/mapper/tst,
   missing codepage or helper program, or other error


You approach simply does not work. (It will probably work for ECB mode but it is
unusable in real world.)


Anyway, I think that you should optimize driver, not add strange hw-dependent
crypto modes to dmcrypt. This is not the first crypto accelerator that is just 
not
suited for this kind of use.

(If it can process batch of chunks of data each with own IV, then it can work
with dmcrypt, but I think such optimized code should be inside crypto API,
not in dmcrypt.)

Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/2] Introduce the bulk IV mode for improving the crypto engine efficiency

[Milan Broz]

On 12/16/2015 04:18 AM, Baolin Wang wrote:
> From the dm-crypt performance report, we found it shows low efficiency
> with crypto engine for some mode (like ecb or xts mode). Because in dm
> crypt, it will map the IO data buffer with scatterlist, and send the
> scatterlist of one bio to the encryption engine, if send more scatterlists
> with bigger size at one time, that helps the engine palys best performance,
> which means a high encryption speed. 
> 
> But now the dm-crypt only map one segment (always one sector) of one bio
> with one scatterlist to the crypto engine at one time. which is more
> time-consuming and ineffective for the crypto engine. Especially for some
> modes which don't need different IV for each sector, we can map the whole
> bio with multiple scatterlists to improve the engine performance.
> 
> But this optimization is not support some ciphers and IV modes which should
> do sector by sector and need different IV for each sector.
> 
> Change since v1:
>  - Introduce one different IV mode.
>  - Change the conditions for bulk mode.

I tried the patchset on 32bit Intel VM and kernel immediately OOPsed (just 
tried aes-ecb)...

Crash log below.

Milan


[   40.989759] BUG: unable to handle kernel NULL pointer dereference at   (null)
[   40.990736] IP: [] crypt_sg_entry+0x186/0x270 [dm_crypt]
[   40.990800] *pde =  
[   40.990844] Oops:  [#1] PREEMPT SMP 
[   40.990961] Modules linked in: dm_crypt loop rpcsec_gss_krb5 dm_mod 
crc32_pclmul crc32c_intel ata_piix aesni_intel aes_i586 lrw ablk_helper cryptd
[   40.991412] CPU: 2 PID: 6 Comm: kworker/u8:0 Not tainted 4.4.0-rc5+ #44
[   40.991460] Hardware name: VMware, Inc. VMware Virtual Platform/440BX 
Desktop Reference Platform, BIOS 6.00 07/02/2015
[   40.991531] Workqueue: kcryptd kcryptd_crypt [dm_crypt]
[   40.991587] task: f4c04180 ti: f4c06000 task.ti: f4c06000
[   40.991629] EIP: 0060:[] EFLAGS: 00010246 CPU: 2
[   40.991672] EIP is at crypt_sg_entry+0x186/0x270 [dm_crypt]
[   40.991725] EAX: 1000 EBX: 1000 ECX: f73e85c0 EDX: 
[   40.991772] ESI:  EDI: 1000 EBP: f4c07e28 ESP: f4c07de8
[   40.991819]  DS: 007b ES: 007b FS: 00d8 GS:  SS: 0068
[   40.991862] CR0: 8005003b CR2:  CR3: 018c1000 CR4: 001406d0
[   40.991949] Stack:
[   40.991976]  f49967c8     0158  
f73e85c0
[   40.992173]   f4baf170 1000 f4ba7290  f4baf030 f4baf170 
f4baf0c0
[   40.992387]  f4c07e60 f8710e8a f4baf170 f4c07e4c f4baf170 f4baf114 f4baf160 
f8713fe8
[   40.992599] Call Trace:
[   40.992647]  [] crypt_convert_io+0x7a/0x360 [dm_crypt]
[   40.992715]  [] kcryptd_crypt+0x395/0x3da [dm_crypt]
[   40.992781]  [] process_one_work+0x153/0x420
[   40.992842]  [] ? process_one_work+0x100/0x420
[   40.992905]  [] worker_thread+0x37/0x470
[   40.992964]  [] ? process_one_work+0x420/0x420
[   40.993026]  [] kthread+0x96/0xb0
[   40.993083]  [] ret_from_kernel_thread+0x21/0x38
[   40.993146]  [] ? kthread_worker_fn+0xf0/0xf0
[   40.993207] Code: c2 01 31 f6 85 db 75 d1 89 55 e0 85 ff 0f 85 41 ff ff ff 
8b 55 d8 8d 65 f4 89 d0 5b 5e 5f 5d c3 90 8d 74 26 00 8b 55 c8 8b 4d dc <8b> 02 
89 4d dc 89 45 c8 c1 e8 1a c1 e0 04 8b 80 80 a2 0b c2 83
[   40.995405] EIP: [] crypt_sg_entry+0x186/0x270 [dm_crypt] SS:ESP 
0068:f4c07de8
[   40.995604] CR2: 
[   40.995703] ---[ end trace d78b89aae913dc1f ]---
[   40.995825] [ cut here ]
[   40.995930] WARNING: CPU: 2 PID: 6 at kernel/softirq.c:150 
__local_bh_enable_ip+0x88/0xd0()
[   40.996118] Modules linked in: dm_crypt loop rpcsec_gss_krb5 dm_mod 
crc32_pclmul
[   40.996352] BUG: unable to handle kernel NULL pointer dereference at   (null)
[   40.996354] IP: [] crypt_sg_entry+0x186/0x270 [dm_crypt]
[   40.996357] *pde =  
[   40.996359] Oops:  [#2] PREEMPT SMP 
[   40.996361] Modules linked in: dm_crypt loop rpcsec_gss_krb5 dm_mod 
crc32_pclmul crc32c_intel ata_piix aesni_intel aes_i586 lrw ablk_helper cryptd
[   40.996368] CPU: 3 PID: 53 Comm: kworker/u8:1 Tainted: G  D 
4.4.0-rc5+ #44
[   40.996369] Hardware name: VMware, Inc. VMware Virtual Platform/440BX 
Desktop Reference Platform, BIOS 6.00 07/02/2015
[   40.996371] Workqueue: kcryptd kcryptd_crypt [dm_crypt]
[   40.996372] task: f489c0c0 ti: f489e000 task.ti: f489e000
[   40.996373] EIP: 0060:[] EFLAGS: 00010246 CPU: 3
[   40.996375] EIP is at crypt_sg_entry+0x186/0x270 [dm_crypt]
[   40.996375] EAX: 1000 EBX: 1000 ECX: f71e3a20 EDX: 
[   40.996376] ESI:  EDI: 0001 EBP: f489fe28 ESP: f489fde8
[   40.996377]  DS: 007b ES: 007b FS: 00d8 GS:  SS: 0068
[   40.996377] CR0: 8005003b CR2:  CR3: 33c26000 CR4: 001406d0
[   40.996410] Stack:
[   40.996411]  f49967c8     0158  
f71e3a20
[   40.996413]   f4bed370 1000 f4b2e7c0  f4bed230 f4bed370 
f4bed2c0
[   40.996415]  f489fe60 f8710e8a f4bed370 f489fe4c f4bed370 

Re: [PATCH v2 0/2] Introduce the bulk IV mode for improving the crypto engine efficiency

[Milan Broz]

On 12/16/2015 04:18 AM, Baolin Wang wrote:
> From the dm-crypt performance report, we found it shows low efficiency
> with crypto engine for some mode (like ecb or xts mode). Because in dm
> crypt, it will map the IO data buffer with scatterlist, and send the
> scatterlist of one bio to the encryption engine, if send more scatterlists
> with bigger size at one time, that helps the engine palys best performance,
> which means a high encryption speed. 
> 
> But now the dm-crypt only map one segment (always one sector) of one bio
> with one scatterlist to the crypto engine at one time. which is more
> time-consuming and ineffective for the crypto engine. Especially for some
> modes which don't need different IV for each sector, we can map the whole
> bio with multiple scatterlists to improve the engine performance.
> 
> But this optimization is not support some ciphers and IV modes which should
> do sector by sector and need different IV for each sector.
> 
> Change since v1:
>  - Introduce one different IV mode.
>  - Change the conditions for bulk mode.

I tried the patchset on 32bit Intel VM and kernel immediately OOPsed (just 
tried aes-ecb)...

Crash log below.

Milan


[   40.989759] BUG: unable to handle kernel NULL pointer dereference at   (null)
[   40.990736] IP: [] crypt_sg_entry+0x186/0x270 [dm_crypt]
[   40.990800] *pde =  
[   40.990844] Oops:  [#1] PREEMPT SMP 
[   40.990961] Modules linked in: dm_crypt loop rpcsec_gss_krb5 dm_mod 
crc32_pclmul crc32c_intel ata_piix aesni_intel aes_i586 lrw ablk_helper cryptd
[   40.991412] CPU: 2 PID: 6 Comm: kworker/u8:0 Not tainted 4.4.0-rc5+ #44
[   40.991460] Hardware name: VMware, Inc. VMware Virtual Platform/440BX 
Desktop Reference Platform, BIOS 6.00 07/02/2015
[   40.991531] Workqueue: kcryptd kcryptd_crypt [dm_crypt]
[   40.991587] task: f4c04180 ti: f4c06000 task.ti: f4c06000
[   40.991629] EIP: 0060:[] EFLAGS: 00010246 CPU: 2
[   40.991672] EIP is at crypt_sg_entry+0x186/0x270 [dm_crypt]
[   40.991725] EAX: 1000 EBX: 1000 ECX: f73e85c0 EDX: 
[   40.991772] ESI:  EDI: 1000 EBP: f4c07e28 ESP: f4c07de8
[   40.991819]  DS: 007b ES: 007b FS: 00d8 GS:  SS: 0068
[   40.991862] CR0: 8005003b CR2:  CR3: 018c1000 CR4: 001406d0
[   40.991949] Stack:
[   40.991976]  f49967c8     0158  
f73e85c0
[   40.992173]   f4baf170 1000 f4ba7290  f4baf030 f4baf170 
f4baf0c0
[   40.992387]  f4c07e60 f8710e8a f4baf170 f4c07e4c f4baf170 f4baf114 f4baf160 
f8713fe8
[   40.992599] Call Trace:
[   40.992647]  [] crypt_convert_io+0x7a/0x360 [dm_crypt]
[   40.992715]  [] kcryptd_crypt+0x395/0x3da [dm_crypt]
[   40.992781]  [] process_one_work+0x153/0x420
[   40.992842]  [] ? process_one_work+0x100/0x420
[   40.992905]  [] worker_thread+0x37/0x470
[   40.992964]  [] ? process_one_work+0x420/0x420
[   40.993026]  [] kthread+0x96/0xb0
[   40.993083]  [] ret_from_kernel_thread+0x21/0x38
[   40.993146]  [] ? kthread_worker_fn+0xf0/0xf0
[   40.993207] Code: c2 01 31 f6 85 db 75 d1 89 55 e0 85 ff 0f 85 41 ff ff ff 
8b 55 d8 8d 65 f4 89 d0 5b 5e 5f 5d c3 90 8d 74 26 00 8b 55 c8 8b 4d dc <8b> 02 
89 4d dc 89 45 c8 c1 e8 1a c1 e0 04 8b 80 80 a2 0b c2 83
[   40.995405] EIP: [] crypt_sg_entry+0x186/0x270 [dm_crypt] SS:ESP 
0068:f4c07de8
[   40.995604] CR2: 
[   40.995703] ---[ end trace d78b89aae913dc1f ]---
[   40.995825] [ cut here ]
[   40.995930] WARNING: CPU: 2 PID: 6 at kernel/softirq.c:150 
__local_bh_enable_ip+0x88/0xd0()
[   40.996118] Modules linked in: dm_crypt loop rpcsec_gss_krb5 dm_mod 
crc32_pclmul
[   40.996352] BUG: unable to handle kernel NULL pointer dereference at   (null)
[   40.996354] IP: [] crypt_sg_entry+0x186/0x270 [dm_crypt]
[   40.996357] *pde =  
[   40.996359] Oops:  [#2] PREEMPT SMP 
[   40.996361] Modules linked in: dm_crypt loop rpcsec_gss_krb5 dm_mod 
crc32_pclmul crc32c_intel ata_piix aesni_intel aes_i586 lrw ablk_helper cryptd
[   40.996368] CPU: 3 PID: 53 Comm: kworker/u8:1 Tainted: G  D 
4.4.0-rc5+ #44
[   40.996369] Hardware name: VMware, Inc. VMware Virtual Platform/440BX 
Desktop Reference Platform, BIOS 6.00 07/02/2015
[   40.996371] Workqueue: kcryptd kcryptd_crypt [dm_crypt]
[   40.996372] task: f489c0c0 ti: f489e000 task.ti: f489e000
[   40.996373] EIP: 0060:[] EFLAGS: 00010246 CPU: 3
[   40.996375] EIP is at crypt_sg_entry+0x186/0x270 [dm_crypt]
[   40.996375] EAX: 1000 EBX: 1000 ECX: f71e3a20 EDX: 
[   40.996376] ESI:  EDI: 0001 EBP: f489fe28 ESP: f489fde8
[   40.996377]  DS: 007b ES: 007b FS: 00d8 GS:  SS: 0068
[   40.996377] CR0: 8005003b CR2:  CR3: 33c26000 CR4: 001406d0
[   40.996410] Stack:
[   40.996411]  f49967c8     0158  
f71e3a20
[   40.996413]   f4bed370 1000 f4b2e7c0  f4bed230 f4bed370 
f4bed2c0
[   40.996415]  f489fe60 f8710e8a f4bed370 f489fe4c f4bed370 

Re: [PATCH 2/2] md: dm-crypt: Optimize the dm-crypt for XTS mode

[Milan Broz]

On 12/15/2015 03:56 AM, Baolin Wang wrote:
>>> + /*
>>> +  * Here we need to check if it can be encrypted or decrypted with
>>> +  * bulk block, which means these encryption modes don't need IV or
>>> +  * just need one initial IV. For bulk mode, we can expand the
>>> +  * scatterlist entries to map the bio, then send all the scatterlists
>>> +  * to the hardware engine at one time to improve the crypto engine
>>> +  * efficiency. But it does not fit for other encryption modes, it has
>>> +  * to do encryption and decryption sector by sector because every
>>> +  * sector has different IV.
>>> +  */
>>> + if (!strcmp(chainmode, "ecb") || !strcmp(chainmode, "xts"))
>>> + cc->bulk_crypto = 1;
>>> + else
>>> + cc->bulk_crypto = 0;n
>>
>> It is perfectly fine to use another IV even for XTS mode (despite it is not 
>> needed).
>> You can use ESSIV for example, or benbi (big-endian variant of plain IV).
>> (And it is really used for some LUKS devices.)
>>
>> How it is supposed to work in this case?
>> If I read this patch correctly, it will completely corrupt data in this case 
>> because
>> it expects plain (consecutive) IV...
> 
> The XTS mode can limit maximum size of each encrypted data unit
> (typically a sector or disk block) to 2^20 AES blocks, so we can use
> one bio as one encrypted data unit (we don't do it sector by sector).
> It can generate one IV for each separate encrypted data unit. Please
> correct me if I misunderstand something. Thanks.

How this will help XTS-ESSIV, if you have to recalculate IV on every fixed
encrypted block?

TBH I think the whole patch here is doing something more than optimization
and seriously touches cryptography part.

Isn't in de-facto reinventing how the full disk encryption works today?

All currently used systems use always disk encrypted block size (usually fixed
to 512 bytes sectors like in dmcrypt) and these are encrypted independently 
with own IV.
(We can talk about supporting larger disk encrypted block sizes but the 
principle
it is still the same.
And we played with it before already  
http://www.saout.de/pipermail/dm-crypt/2013-January/003125.html)

Every encrypted disk block here has independent initialization vector - IV (or 
tweak).
For some modes (like XTS, LRW) this vector can produce just predictable linear 
offset,
usually just block number (it must not repeat though; exceptions are 
legacy/compatible IVs).
But it can be also something else - pseudorandom sequence like in ESSIV or so.

If I understand your patch correctly, you are trying to use the XTS mode for
the whole bulk request (there is only one IV for the bulk request and this bulk 
request
is larger than currently fixed disk block size).

Isn't it even incompatible with the current XTS (aes-xts-plain64) encrypted disk
per-sector where for every sectors new IV initialization is used?
In fact I think that your approach would need to implement some different
IV name (aes-xts-bulk) or something like that so userspace (cryptsetup)
can be backward compatible.

BTW the 2^20 block limit requirement (the XTS block size) is strict limit in 
some specifications.
For example, NIST SP800-38E (XTS-AES) says:
"The length of the data unit for any instance of an implementation of XTS-AES 
shall not exceed
2^20 AES blocks."

There are a lot of badly implemented hw crypto accelerators where 
initialization cost
is quite big and that performs better if it encrypts large blocks of data
(it applies even for CBC mode). These are simply not designed for FDE use 
case...

But IMHO this is not correct reason to patch it in kernel, moreover on dmcrypt 
layer
with side effect of hardcoding another kind of crypto logic into dmcrypt.

DMcrypt should be completely independent of used symmetric block encryption mode
(that's the crypto API job). There will be new modes in future, there can be 
requests
to switch to different IVs (similar to preventing CBC watermarking attack with
predictable IV) etc.

Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] md: dm-crypt: Optimize the dm-crypt for XTS mode

[Milan Broz]

On 12/15/2015 03:56 AM, Baolin Wang wrote:
>>> + /*
>>> +  * Here we need to check if it can be encrypted or decrypted with
>>> +  * bulk block, which means these encryption modes don't need IV or
>>> +  * just need one initial IV. For bulk mode, we can expand the
>>> +  * scatterlist entries to map the bio, then send all the scatterlists
>>> +  * to the hardware engine at one time to improve the crypto engine
>>> +  * efficiency. But it does not fit for other encryption modes, it has
>>> +  * to do encryption and decryption sector by sector because every
>>> +  * sector has different IV.
>>> +  */
>>> + if (!strcmp(chainmode, "ecb") || !strcmp(chainmode, "xts"))
>>> + cc->bulk_crypto = 1;
>>> + else
>>> + cc->bulk_crypto = 0;n
>>
>> It is perfectly fine to use another IV even for XTS mode (despite it is not 
>> needed).
>> You can use ESSIV for example, or benbi (big-endian variant of plain IV).
>> (And it is really used for some LUKS devices.)
>>
>> How it is supposed to work in this case?
>> If I read this patch correctly, it will completely corrupt data in this case 
>> because
>> it expects plain (consecutive) IV...
> 
> The XTS mode can limit maximum size of each encrypted data unit
> (typically a sector or disk block) to 2^20 AES blocks, so we can use
> one bio as one encrypted data unit (we don't do it sector by sector).
> It can generate one IV for each separate encrypted data unit. Please
> correct me if I misunderstand something. Thanks.

How this will help XTS-ESSIV, if you have to recalculate IV on every fixed
encrypted block?

TBH I think the whole patch here is doing something more than optimization
and seriously touches cryptography part.

Isn't in de-facto reinventing how the full disk encryption works today?

All currently used systems use always disk encrypted block size (usually fixed
to 512 bytes sectors like in dmcrypt) and these are encrypted independently 
with own IV.
(We can talk about supporting larger disk encrypted block sizes but the 
principle
it is still the same.
And we played with it before already  
http://www.saout.de/pipermail/dm-crypt/2013-January/003125.html)

Every encrypted disk block here has independent initialization vector - IV (or 
tweak).
For some modes (like XTS, LRW) this vector can produce just predictable linear 
offset,
usually just block number (it must not repeat though; exceptions are 
legacy/compatible IVs).
But it can be also something else - pseudorandom sequence like in ESSIV or so.

If I understand your patch correctly, you are trying to use the XTS mode for
the whole bulk request (there is only one IV for the bulk request and this bulk 
request
is larger than currently fixed disk block size).

Isn't it even incompatible with the current XTS (aes-xts-plain64) encrypted disk
per-sector where for every sectors new IV initialization is used?
In fact I think that your approach would need to implement some different
IV name (aes-xts-bulk) or something like that so userspace (cryptsetup)
can be backward compatible.

BTW the 2^20 block limit requirement (the XTS block size) is strict limit in 
some specifications.
For example, NIST SP800-38E (XTS-AES) says:
"The length of the data unit for any instance of an implementation of XTS-AES 
shall not exceed
2^20 AES blocks."

There are a lot of badly implemented hw crypto accelerators where 
initialization cost
is quite big and that performs better if it encrypts large blocks of data
(it applies even for CBC mode). These are simply not designed for FDE use 
case...

But IMHO this is not correct reason to patch it in kernel, moreover on dmcrypt 
layer
with side effect of hardcoding another kind of crypto logic into dmcrypt.

DMcrypt should be completely independent of used symmetric block encryption mode
(that's the crypto API job). There will be new modes in future, there can be 
requests
to switch to different IVs (similar to preventing CBC watermarking attack with
predictable IV) etc.

Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] md: dm-crypt: Optimize the dm-crypt for XTS mode

[Milan Broz]

On 12/14/2015 09:23 AM, Baolin Wang wrote:
> In now dm-crypt code, it is ineffective to map one bio with just only
> one scatterlist at one time for XTS mode. We can use multiple scatterlists
> to map the whole bio and send all scatterlists of one bio to crypto engine
> to encrypt or decrypt, which can improve the hardware engine's efficiency.

...

> + /*
> +  * Here we need to check if it can be encrypted or decrypted with
> +  * bulk block, which means these encryption modes don't need IV or
> +  * just need one initial IV. For bulk mode, we can expand the
> +  * scatterlist entries to map the bio, then send all the scatterlists
> +  * to the hardware engine at one time to improve the crypto engine
> +  * efficiency. But it does not fit for other encryption modes, it has
> +  * to do encryption and decryption sector by sector because every
> +  * sector has different IV.
> +  */
> + if (!strcmp(chainmode, "ecb") || !strcmp(chainmode, "xts"))
> + cc->bulk_crypto = 1;
> + else
> + cc->bulk_crypto = 0;n

It is perfectly fine to use another IV even for XTS mode (despite it is not 
needed).
You can use ESSIV for example, or benbi (big-endian variant of plain IV).
(And it is really used for some LUKS devices.)

How it is supposed to work in this case?
If I read this patch correctly, it will completely corrupt data in this case 
because
it expects plain (consecutive) IV...

Also how it handles 32bit plain IV (that restart after 2TB)? (IOW plain IV, not 
plain64).

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] md: dm-crypt: Optimize the dm-crypt for XTS mode

[Milan Broz]

On 12/14/2015 09:23 AM, Baolin Wang wrote:
> In now dm-crypt code, it is ineffective to map one bio with just only
> one scatterlist at one time for XTS mode. We can use multiple scatterlists
> to map the whole bio and send all scatterlists of one bio to crypto engine
> to encrypt or decrypt, which can improve the hardware engine's efficiency.

...

> + /*
> +  * Here we need to check if it can be encrypted or decrypted with
> +  * bulk block, which means these encryption modes don't need IV or
> +  * just need one initial IV. For bulk mode, we can expand the
> +  * scatterlist entries to map the bio, then send all the scatterlists
> +  * to the hardware engine at one time to improve the crypto engine
> +  * efficiency. But it does not fit for other encryption modes, it has
> +  * to do encryption and decryption sector by sector because every
> +  * sector has different IV.
> +  */
> + if (!strcmp(chainmode, "ecb") || !strcmp(chainmode, "xts"))
> + cc->bulk_crypto = 1;
> + else
> + cc->bulk_crypto = 0;n

It is perfectly fine to use another IV even for XTS mode (despite it is not 
needed).
You can use ESSIV for example, or benbi (big-endian variant of plain IV).
(And it is really used for some LUKS devices.)

How it is supposed to work in this case?
If I read this patch correctly, it will completely corrupt data in this case 
because
it expects plain (consecutive) IV...

Also how it handles 32bit plain IV (that restart after 2TB)? (IOW plain IV, not 
plain64).

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/2] dm verity: add support for error correction

[Milan Broz]

On 12/07/2015 05:31 PM, Sami Tolvanen wrote:
> On Mon, Dec 07, 2015 at 09:58:14AM -0500, Mike Snitzer wrote:
>> Great.  Moving forward it'd be awesome if you could work to get your
>> verity FEC support regression tests into cryptsetup's tests.
> 
> Sure. These tests would basically involve generating a valid disk
> image, corrupting it up to the theoretical maximum, and verifying that
> dm-verity can correct it.

Does the code handle/report even the same type of data corruption
for RS blocks? Or this is just silently ignored until
data area corruption happens?
Sorry, I did not study the code here, I am just curious :)

(IOW I mean if data are ok but recovery metadata is corrupted.)

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/2] dm verity: add support for error correction

[Milan Broz]

On 12/07/2015 05:31 PM, Sami Tolvanen wrote:
> On Mon, Dec 07, 2015 at 09:58:14AM -0500, Mike Snitzer wrote:
>> Great.  Moving forward it'd be awesome if you could work to get your
>> verity FEC support regression tests into cryptsetup's tests.
> 
> Sure. These tests would basically involve generating a valid disk
> image, corrupting it up to the theoretical maximum, and verifying that
> dm-verity can correct it.

Does the code handle/report even the same type of data corruption
for RS blocks? Or this is just silently ignored until
data area corruption happens?
Sorry, I did not study the code here, I am just curious :)

(IOW I mean if data are ok but recovery metadata is corrupted.)

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] dm verity: add support for error correction

[Milan Broz]

On 11/09/2015 08:19 PM, Sami Tolvanen wrote:
...
> We don't see actual I/O errors very often. Most corruption we've seen
> is caused by flaky hardware that doesn't return errors. However, I can
> certainly change to code to attempt recovery in this case too.

So if I understand it correctly, there is a simplified flash controller
that can send data with bit flips without detection?

(IOW in "real" SSD this kind of error should be detected internally
by some bad block management?)

This is why I asked about some statistics of real visible types of errors.

For this use case it makes sense to have error correction here but then
we should clearly said that it makes no sense to switch it on for "real" hw
that does internal integrity check or error correction
(but not authenticated integrity check as dm-verity).

...

>> If this error correction feature is going to go upstream we really
>> should see any associated userspace enablement also included in
>> veritysetup.
> 
> I can look into this.

Yes, please, patches do not to be production ready (I can integrate
it to veritysetup upstream myself) but it would be very nice that
released veritysetup can configure all dm-verity features in the same time
the mainline kernel is marked stable.
(The same applies for dm-crypt & cryptsetup.)

Thanks,
Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] dm verity: add support for error correction

[Milan Broz]

On 11/09/2015 08:19 PM, Sami Tolvanen wrote:
...
> We don't see actual I/O errors very often. Most corruption we've seen
> is caused by flaky hardware that doesn't return errors. However, I can
> certainly change to code to attempt recovery in this case too.

So if I understand it correctly, there is a simplified flash controller
that can send data with bit flips without detection?

(IOW in "real" SSD this kind of error should be detected internally
by some bad block management?)

This is why I asked about some statistics of real visible types of errors.

For this use case it makes sense to have error correction here but then
we should clearly said that it makes no sense to switch it on for "real" hw
that does internal integrity check or error correction
(but not authenticated integrity check as dm-verity).

...

>> If this error correction feature is going to go upstream we really
>> should see any associated userspace enablement also included in
>> veritysetup.
> 
> I can look into this.

Yes, please, patches do not to be production ready (I can integrate
it to veritysetup upstream myself) but it would be very nice that
released veritysetup can configure all dm-verity features in the same time
the mainline kernel is marked stable.
(The same applies for dm-crypt & cryptsetup.)

Thanks,
Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] dm verity: add support for error correction

[Milan Broz]

On 11/05/2015 03:02 AM, Sami Tolvanen wrote:
> This patch set adds error correction support to dm-verity, which
> makes it possible to recover from data corruption in exchange of
> increased space overhead.
> 
> The feature is implemented as part of dm-verity to take advantage
> of the existing hash tree to improve performance and locate known
> erasures.

Hi,

could you please elaborate why is all this needed? To extend support
of some faulty flash chips?

Do you have some statistics that there are really such correctable errors
in real devices?

Anyway, I really do not understand layer separation here. Either we have
cryptographically strong data integrity checking or we have
error-correction. Are we sure this combination does not create some unintended
gap in integrity checking? Why the integrity check should even try to do some
error correction if there is an intentional integrity attack?

IMO if you need an error correction, this should be placed as a separate
layer below the crypto integrity check, the same as RAID operates.

The second question - why are you writing another separate tool
for maintenance for dm-verity when there is veritysetup?

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] dm verity: add support for error correction

[Milan Broz]

On 11/05/2015 03:02 AM, Sami Tolvanen wrote:
> This patch set adds error correction support to dm-verity, which
> makes it possible to recover from data corruption in exchange of
> increased space overhead.
> 
> The feature is implemented as part of dm-verity to take advantage
> of the existing hash tree to improve performance and locate known
> erasures.

Hi,

could you please elaborate why is all this needed? To extend support
of some faulty flash chips?

Do you have some statistics that there are really such correctable errors
in real devices?

Anyway, I really do not understand layer separation here. Either we have
cryptographically strong data integrity checking or we have
error-correction. Are we sure this combination does not create some unintended
gap in integrity checking? Why the integrity check should even try to do some
error correction if there is an intentional integrity attack?

IMO if you need an error correction, this should be placed as a separate
layer below the crypto integrity check, the same as RAID operates.

The second question - why are you writing another separate tool
for maintenance for dm-verity when there is veritysetup?

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] dm-crypt: never use write same (was Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME)

[Milan Broz]

(sorry, resending again, not sure if it was sent correctly)

On 07/13/2015 06:33 PM, Joseph Salisbury wrote:
> Hi Milan,
> 
> The Ubuntu kernel has been carrying this patch since the discussion[0]
> we were having about the bug.  I don't see that patch was ever included
> in mainline.  Do you happen to know if this patch is still needed or was
> the bug we were seeing fixed in some other way?

I think it was superseded by later Mike's approach to reverse the logic
- disable write same for all targets and require to explicitly allow it.

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/md/dm-table.c?id=c1a94672a830e01d58c7c7e8de530c3f136d6ff2
(+later patches)

So I think we do no need this patch upstream anymore.

Mike, am I right here?

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] dm-crypt: never use write same (was Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME)

[Milan Broz]

On 07/13/2015 06:33 PM, Joseph Salisbury wrote:
> Hi Milan,
> 
> The Ubuntu kernel has been carrying this patch since the discussion[0]
> we were having about the bug.  I don't see that patch was ever included
> in mainline.  Do you happen to know if this patch is still needed or was
> the bug we were seeing fixed in some other way?

I think it was superseded by later Mike's approach to reverse the logic
- disable write same for all targets and require to explicitly allow it.

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/md/dm-table.c?id=c1a94672a830e01d58c7c7e8de530c3f136d6ff2
(+later patches)

So I think we do no need this patch upstream anymore.

Mike, am I right here?

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/