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 crypt_report_zon

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 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->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->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: [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, &io->iter);
> + continue;
> + }
> +
>   r = verity_hash_for_block(v, io, io->block + b,
> verity_io_want_digest(v, io),
> &is_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, &start) == 0)
> +cur_block, NULL, &start) == 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 = kvz

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

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: [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 
---
 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: [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(&io->work, verity_work);
>   queue_work(io->v->verify_wq, &io->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
> @@

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 star

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 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  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 br

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: [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: 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: [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: [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_sendpag

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 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(&ctx->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(&ctx->req, private);
> + skcipher_request_set_tfm(&ctx->req, skcipher);
>   skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> af_alg_complete, &ctx->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 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/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 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] 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/

Re: [PATCH 3.14 73/92] dm crypt: fix deadlock when async crypto algorithm returns -EBUSY

[Milan Broz]

On 05/05/2015 05:22 AM, Mike Snitzer wrote:
> On Mon, May 04 2015 at  5:32pm -0400,
> Rabin Vincent  wrote:
> 
>> On Sat, May 02, 2015 at 09:03:28PM +0200, Greg Kroah-Hartman wrote:
>>> 3.14-stable review patch.  If anyone has any objections, please let me know.
>>>
>>> --
>>>
>>> From: Ben Collins 
>>>
>>> commit 0618764cb25f6fa9fb31152995de42a8a0496475 upstream.
>>
>> The commit in question was recently merged to mainline and is now being
>> sent to various stable kernels.  But I think the patch is wrong and the
>> real problem lies in the Freescale CAAM driver which is described as
>> having being tested with.
...

>> dm-crypt uses CRYPTO_TFM_REQ_MAY_BACKLOG.  This means the the crypto
>> driver should internally backlog requests which arrive when the queue is
>> full and process them later.  Until the crypto hw's queue becomes full,
>> the driver returns -EINPROGRESS.  When the crypto hw's queue if full,
>> the driver returns -EBUSY, and if CRYPTO_TFM_REQ_MAY_BACKLOG is set, is
>> expected to backlog the request and process it when the hardware has
>> queue space.  At the point when the driver takes the request from the
>> backlog and starts processing it, it calls the completion function with
>> a status of -EINPROGRESS.  The completion function is called (for a
>> second time, in the case of backlogged requests) with a status/err of 0
>> when a request is done.


Mike, I thought there was a discussion with crypto API maintainer before
merging this patch, I think there were some comments that the patch seems
to be not correct...
This API (EBUSY/EINPROGESS codes) use was there since dmcrypt switched
to async crypto API.

There should tests for it (some years ago we tested the async path by forcing
to use cryptd, this was able to clearly simulate the BUSY path as well),
but not sure if it is still possible so easily.

> Any chance you'd be willing to provide in-code comments in the
> appropriate places in dm-crypt.c (after having reverted this patch in
> question)?
> 
> I'd be happy to take the combination of the revert and documentation in
> a single patch and get it to Linus for 4.0-rc3.

Please, do not mix revert patches with additions (even if it is just comment),
someone could be even more confused what's going on here.

If nobody volunteers, I'll try to add some comments later to dmcrypt code,
but that is definitely not for stable.

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 5/8 v2] dm: replace memset by memzero_explicit

[Milan Broz]

On 11/30/2014 06:03 PM, Julia Lawall wrote:
> From: Julia Lawall 
> 
> Memset on a local variable may be removed when it is called just before the
> variable goes out of scope.  Using memzero_explicit defeats this
> optimization.  A simplified version of the semantic patch that makes this
> change is as follows: (http://coccinelle.lip6.fr/)

Ack, but I submitted the same patch a week ago

https://www.redhat.com/archives/dm-devel/2014-November/msg00084.html

Mike, please could you add this to linux-next tree or you want this
to go through Herbert's tree?
(I do not think it is good idea for DM patches.)

Thanks,
Milan

> 
> // 
> @@
> identifier x;
> type T;
> @@
> 
> {
> ... when any
> T x[...];
> ... when any
> when exists
> - memset
> + memzero_explicit
>   (x,
> -0,
>   ...)
> ... when != x
> when strict
> }
> // 
> 
> This change was suggested by Daniel Borkmann 
> 
> Signed-off-by: Julia Lawall 
> 
> ---
> Daniel Borkmann suggested that these patches could go through Herbert Xu's
> cryptodev tree.
> 
> v2: fixed email address
> 
>  drivers/md/dm-crypt.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index fc93b93..08981be 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -705,7 +705,7 @@ static int crypt_iv_tcw_whitening(struct crypt_config *cc,
>   for (i = 0; i < ((1 << SECTOR_SHIFT) / 8); i++)
>   crypto_xor(data + i * 8, buf, 8);
>  out:
> - memset(buf, 0, sizeof(buf));
> + memzero_explicit(buf, sizeof(buf));
>   return r;
>  }
>  
> 

--
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, dm: LLVMLinux: Remove VLAIS usage from dm-crypt

[Milan Broz]

On 09/06/2014 01:02 AM, beh...@converseincode.com wrote:
> From: Jan-Simon Möller 
> 
> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
> precludes the use of compilers which don't implement VLAIS (for instance the
> Clang compiler). This patch instead allocates the appropriate amount of memory
> using an char array.

Well, if clang (or C99 code) is now preferred for kernel, why not.

Just please commit the patch series en bloc (together with the patches
removing VLAIS from crypto code you posted to cryptoapi list).

Dmcrypt should always use the same coding practices as crypto part of the kernel
so we should not divert here.

> - struct {
> - struct shash_desc desc;
> - char ctx[crypto_shash_descsize(lmk->hash_tfm)];
> - } sdesc;
> + char sdesc[sizeof(struct shash_desc) +
> + crypto_shash_descsize(lmk->hash_tfm)] CRYPTO_MINALIGN_ATTR;
> + struct shash_desc *desc = (struct shash_desc *)sdesc;

TBH, this looks even more uglier that the previous code :-)

(But tglx already complained on different patch and I fully agree that crypto 
code
should not use this kind of construction in the first place...
It would be very nice to introduce at least some macro hiding these crazy
stack allocations later.)

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: scsi_debug module deadlock on 3.17-rc2

[Milan Broz]

On 08/31/2014 01:14 AM, Douglas Gilbert wrote:
> On 14-08-30 04:56 PM, Milan Broz wrote:
>> Hi,
>>
>> I am using scsi_debug in cryptsetup testsuite and with recent 3.17-rc kernel
>> it deadlocks on rmmod of scsi_debug module.
>>
>> For me even this simple reproducer causes deadlock:
>>modprobe scsi_debug dev_size_mb=16 sector_size=512 num_tgts=1
>>DEV="/dev/"$(grep -l -e scsi_debug /sys/block/*/device/model | cut -f4 -d 
>> /)
>>mkfs -t ext4 $DEV
>>rmmod scsi_debug
>>
>> (adding small delay before rmmod obviously helps here)
> 
> So I used this slight variation for testing:
> 
> modprobe scsi_debug dev_size_mb=16 sector_size=512 num_tgts=1 num_parts=1
> DEV="/dev/"$(grep -l -e scsi_debug /sys/block/*/device/model | cut -f4 -d 
> /)"1"
> echo "mkfs -t ext4 ${DEV}"
> mkfs -t ext4 ${DEV}
> sleep 0.1
> rmmod scsi_debug
> 
>> Bisect tracked it to commit
>>commit cbf67842c3d9e7af8ccc031332b79e88d9cca592
>>Author: Douglas Gilbert 
>>Date:   Sat Jul 26 11:55:35 2014 -0400
>>scsi_debug: support scsi-mq, queues and locks
>>
>> I guess that with introducing mq the del_timer_sync() must not be called
>> with acquired queued_arr_lock.
>> (to me it looks like situation described in comment before
>> del_timer_sync() in kernel/time/timer.c...)
> 
> Looks like something a lawyer would write.

:-)

...

> Anyway the attached patch removes the lock(queued_arr_lock)
> from around the del_timer calls. Could you try it and report
> back.

Yes, the patch fixes the deadlock.
Thanks!

Tested-and-reported-by: Milan Broz 

However, removing the device (rmmod scsi_debug) now take up to 20 or more 
seconds.
(Nothing like flush buffers or udev settle helps seems...)

It is somehow related to udev events processing (killing udev process
will remove this delay).

I have full process trace (let me know if you want to send it),
this is just where rmmod is while waiting (note the delay in the end):

[  132.918141] sd 3:0:0:0: [sdh] Attached SCSI disk
[  137.476448] SysRq : Show State
...
[  137.485711] kworker/0:2 D c16ce000 0   274  2 0x
[  137.485724] Workqueue: events_freezable_power_ disk_events_workfn
[  137.485728]  dc915c2c 0046 dc915bcc c16ce000 debad340 001f de15e250 
c00530f0
[  137.485741]  0002 c00530f0 0001 c00530f0 c0053698 dc915c0c c106e934 
c1465cdd
[  137.485753]  c00530f0 dc915c44 dc915c0c c128925f dc915c1c c106ea49 0282 
c1ff7d80
[  137.485765] Call Trace:
[  137.485772]  [] ? mark_held_locks+0x64/0x90
[  137.485779]  [] ? _raw_spin_unlock_irqrestore+0x4d/0x70
[  137.485785]  [] ? __this_cpu_preempt_check+0xf/0x20
[  137.485792]  [] ? trace_hardirqs_on_caller+0xe9/0x220
[  137.485798]  [] schedule+0x1e/0x60
[  137.485804]  [] schedule_timeout+0x11f/0x1b0
[  137.485811]  [] ? internal_add_timer+0x60/0x60
[  137.485817]  [] ? trace_hardirqs_on_caller+0xe9/0x220
[  137.485824]  [] io_schedule_timeout+0x46/0x60
[  137.485831]  [] wait_for_common_io.constprop.1+0x7b/0xd0
[  137.485836]  [] ? wake_up_process+0x40/0x40
[  137.485843]  [] wait_for_completion_io_timeout+0x8/0x10
[  137.485849]  [] blk_execute_rq+0x81/0x100
[  137.485856]  [] ? wait_for_common_io.constprop.1+0x17/0xd0
[  137.485863]  [] ? cfq_exit_icq+0x40/0x40
[  137.485870]  [] ? elv_set_request+0x15/0x30
[  137.485877]  [] ? get_request+0x2ea/0x490
[  137.485884]  [] ? __wake_up_sync+0x10/0x10
[  137.485891]  [] ? blk_get_request+0x69/0xd0
[  137.485897]  [] scsi_execute+0xfc/0x190
[  137.485904]  [] scsi_execute_req_flags+0x53/0xb0
[  137.485912]  [] scsi_test_unit_ready+0x5e/0x100
[  137.485918]  [] sd_check_events+0xe3/0x120
[  137.485925]  [] disk_check_events+0x37/0x100
[  137.485932]  [] disk_events_workfn+0x13/0x20
[  137.485939]  [] process_one_work+0x156/0x390
[  137.485945]  [] ? process_one_work+0x103/0x390
[  137.485951]  [] worker_thread+0x39/0x440
[  137.485958]  [] ? execute_in_process_context+0x70/0x70
[  137.485965]  [] kthread+0xa3/0xc0
[  137.485970]  [] ? put_lock_stats.isra.21+0xd/0x30
[  137.485976]  [] ? trace_hardirqs_on+0xb/0x10
[  137.485982]  [] ret_from_kernel_thread+0x21/0x30
[  137.485989]  [] ? kthread_create_on_node+0x160/0x160
...
[  137.493862] rmmod   D c16ce000 0  1434   1422 0x
[  137.493869]  da1bbca4 0046 deb5e050 c16ce000 199fe46e 001f df11c190 
deb5e050
[  137.493881]  b02da1a3 da1bbcc0 c10708cb c1c0f9c8 0004  c1c75a90 

[  137.493893]  0ac0a782 b02da1a3 0003 deb5e670 0001 deb5e050 deb5e050 
deb5e670
[  137.493905] Call Trace:
[  137.493911]  [] ? __lock_acquire+0x48b/0x1c30
[  137.493917]  [] schedule+0x1e/0x60
[  137.493924]  [] schedule_timeout+0x14d/0x1b0
[  137.493930]  [] ? mark_held_locks+0x64/0x90
[  137.493936]  [] ? _raw_spin_unlock_irq+0x22/0x50
[  137.493942

scsi_debug module deadlock on 3.17-rc2

[Milan Broz]

Hi,

I am using scsi_debug in cryptsetup testsuite and with recent 3.17-rc kernel
it deadlocks on rmmod of scsi_debug module.

For me even this simple reproducer causes deadlock:
  modprobe scsi_debug dev_size_mb=16 sector_size=512 num_tgts=1
  DEV="/dev/"$(grep -l -e scsi_debug /sys/block/*/device/model | cut -f4 -d /)
  mkfs -t ext4 $DEV
  rmmod scsi_debug

(adding small delay before rmmod obviously helps here)

Bisect tracked it to commit
  commit cbf67842c3d9e7af8ccc031332b79e88d9cca592
  Author: Douglas Gilbert 
  Date:   Sat Jul 26 11:55:35 2014 -0400
  scsi_debug: support scsi-mq, queues and locks

I guess that with introducing mq the del_timer_sync() must not be called
with acquired queued_arr_lock.
(to me it looks like situation described in comment before
del_timer_sync() in kernel/time/timer.c...)

Here is the log (running on vmware VM and i686 arch):

[   67.916472] scsi_debug: host protection
[   67.916483] scsi host3: scsi_debug, version 1.84 [20140706], dev_size_mb=16, 
opts=0x0
[   67.917446] scsi 3:0:0:0: Direct-Access Linuxscsi_debug   0184 
PQ: 0 ANSI: 5
[   67.920539] sd 3:0:0:0: Attached scsi generic sg8 type 0
[   67.940542] sd 3:0:0:0: [sdh] 32768 512-byte logical blocks: (16.7 MB/16.0 
MiB)
[   67.940548] sd 3:0:0:0: [sdh] 4096-byte physical blocks
[   67.950705] sd 3:0:0:0: [sdh] Write Protect is off
[   67.950715] sd 3:0:0:0: [sdh] Mode Sense: 73 00 10 08
[   67.970514] sd 3:0:0:0: [sdh] Write cache: enabled, read cache: enabled, 
supports DPO and FUA
[   68.040566]  sdh: unknown partition table
[   68.090618] sd 3:0:0:0: [sdh] Attached SCSI disk
[   68.799699]  sdh: unknown partition table
[   69.072314] 
[   69.072387] ==
[   69.072433] [ INFO: possible circular locking dependency detected ]
[   69.072487] 3.17.0-rc2+ #80 Not tainted
[   69.072518] ---
[   69.072560] rmmod/2890 is trying to acquire lock:
[   69.072595]  ((sqcp->cmnd_timerp)){+.-...}, at: [] 
del_timer_sync+0x0/0xb0
[   69.072704] 
[   69.072704] but task is already holding lock:
[   69.072743]  (queued_arr_lock){..-...}, at: [] 
stop_all_queued+0x17/0xc0 [scsi_debug]
[   69.072852] 
[   69.072852] which lock already depends on the new lock.
[   69.072852] 
[   69.072902] 
[   69.072902] the existing dependency chain (in reverse order) is:
[   69.072949] 
[   69.072949] -> #1 (queued_arr_lock){..-...}:
[   69.073045][] lock_acquire+0x59/0xa0
[   69.073114][] _raw_spin_lock_irqsave+0x31/0x70
[   69.073438][] sdebug_q_cmd_complete+0x27/0x190 [scsi_debug]
[   69.073515][] call_timer_fn+0x5b/0xd0
[   69.073581][] run_timer_softirq+0x161/0x200
[   69.073649][] __do_softirq+0x119/0x230
[   69.073726][] do_softirq_own_stack+0x27/0x30
[   69.073811][] irq_exit+0x7e/0xa0
[   69.073889][] smp_apic_timer_interrupt+0x33/0x40
[   69.073969][] apic_timer_interrupt+0x32/0x38
[   69.074254][] arch_cpu_idle+0x9/0x10
[   69.074318][] cpu_startup_entry+0x22c/0x280
[   69.074381][] rest_init+0x9c/0xb0
[   69.074441][] start_kernel+0x2e9/0x2ee
[   69.074504][] i386_start_kernel+0x79/0x7d
[   69.074567] 
[   69.074567] -> #0 ((sqcp->cmnd_timerp)){+.-...}:
[   69.074794][] __lock_acquire+0x16e4/0x1c30
[   69.074859][] lock_acquire+0x59/0xa0
[   69.074919][] del_timer_sync+0x29/0xb0
[   69.074981][] stop_all_queued+0x8a/0xc0 [scsi_debug]
[   69.075050][] scsi_debug_exit+0x16/0xac [scsi_debug]
[   69.075117][] SyS_delete_module+0xfd/0x180
[   69.075181][] syscall_after_call+0x0/0x4
[   69.075243] 
[   69.075243] other info that might help us debug this:
[   69.075243] 
[   69.075321]  Possible unsafe locking scenario:
[   69.075321] 
[   69.075380]CPU0CPU1
[   69.075424]
[   69.075468]   lock(queued_arr_lock);
[   69.075534]lock((sqcp->cmnd_timerp));
[   69.075613]lock(queued_arr_lock);
[   69.075690]   lock((sqcp->cmnd_timerp));
[   69.075758] 
[   69.075758]  *** DEADLOCK ***
[   69.075758] 
[   69.075827] 1 lock held by rmmod/2890:
[   69.075867]  #0:  (queued_arr_lock){..-...}, at: [] 
stop_all_queued+0x17/0xc0 [scsi_debug]
[   69.076009] 
[   69.076009] stack backtrace:
[   69.076064] CPU: 1 PID: 2890 Comm: rmmod Not tainted 3.17.0-rc2+ #80
[   69.076117] Hardware name: VMware, Inc. VMware Virtual Platform/440BX 
Desktop Reference Platform, BIOS 6.00 07/31/2013
[   69.076200]  c1c93200  da25fe30 c146081f c1c93330 da25fe60 c145fbf6 
c158bbfc
[   69.076375]  c158bb99 c158bb7c c158bb91 c158bb7c da25fe9c df319070 df319098 
df319618
[   69.076550]  df3195f0 da25fecc c1071b94 df3195f0 c1c4a0b0 0001  
0001
[   69.076724] Call Trace:
[   69.076760]  [] dump_stack+0x4b/0x75
[   69.076805]  [] print_circula

Re: Kernel crypto API: cryptoperf performance measurement

[Milan Broz]

On 08/20/2014 03:25 PM, Jussi Kivilinna wrote:
>> One to four GB per second for XTS? 12 GB per second for AES CBC? Somehow 
>> that 
>> does not sound right.
> 
> Agreed, those do not look correct... I wonder what happened there. On
> new run, I got more sane results:

Which cryptsetup version are you using?

There was a bug in that test on fast machines (fixed in 1.6.3, I hope :)

But anyway, it is not intended as rigorous speed test,
it was intended for comparison of ciphers speed on particular machine.

Test basically tries to encrypt 1MB block (or multiple of this
if machine is too fast). All it runs through kernel userspace crypto API
interface.
(Real FDE is always slower because it runs over 512bytes blocks.)

Milan


> 
> #  Algorithm | Key |  Encryption |  Decryption
>  aes-cbc   128b   139,1 MiB/s  1713,6 MiB/s
>  serpent-cbc   128b62,2 MiB/s   232,9 MiB/s
>  twofish-cbc   128b   116,3 MiB/s   243,7 MiB/s
>  aes-cbc   256b   375,1 MiB/s  1159,4 MiB/s
>  serpent-cbc   256b62,1 MiB/s   214,9 MiB/s
>  twofish-cbc   256b   139,3 MiB/s   217,5 MiB/s
>  aes-xts   256b  1296,4 MiB/s  1272,5 MiB/s
>  serpent-xts   256b   283,3 MiB/s   275,6 MiB/s
>  twofish-xts   256b   294,8 MiB/s   299,3 MiB/s
>  aes-xts   512b   984,3 MiB/s   991,1 MiB/s
>  serpent-xts   512b   227,7 MiB/s   220,6 MiB/s
>  twofish-xts   512b   220,6 MiB/s   220,2 MiB/s
> 
> -Jussi
> 

--
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: [ANNOUNCE] util-linux 2.23

[Milan Broz]

On 25.4.2013 15:25, Andi Kleen wrote:
>> Util-linux 2.23 Release Notes
>> =
>>
>> The cryptoloop support in the commands mount(8) and losetup(8) has been
>> REMOVED. The encryption= mount option and -e,-E,--encryption losetup options
>> are no more supported.
> 
> That's crazy. You're taking away access to existing data.

Nope. You can use cryptsetup.

https://lkml.org/lkml/2012/11/2/162

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] algif_skcipher: Avoid crash if buffer is not multiple of cipher block size

[Milan Broz]

On 04/14/2013 06:12 PM, Milan Broz wrote:
> When user requests encryption (or decryption) of block which
> is not aligned to cipher block size through userspace crypto
> interface, an OOps like this can happen

And this is a reproducer for the problem above...

Milan

/* 
 * Check for unaligned buffer to block cipher size in kernel crypto API
 * fixed by patch: http://article.gmane.org/gmane.linux.kernel.cryptoapi/7980
 * 
 * Compile with gcc test.c -o tst
 */

#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

#ifndef AF_ALG
#define AF_ALG 38
#endif
#ifndef SOL_ALG
#define SOL_ALG 279
#endif

static int kernel_crypt(int opfd, const char *in, char *out, size_t length,
 const char *iv, size_t iv_length, uint32_t direction)
{
int r = 0;
ssize_t len;
struct af_alg_iv *alg_iv;
struct cmsghdr *header;
uint32_t *type;
struct iovec iov = {
.iov_base = (void*)(uintptr_t)in,
.iov_len = length,
};
int iv_msg_size = iv ? CMSG_SPACE(sizeof(*alg_iv) + iv_length) : 0;
char buffer[CMSG_SPACE(sizeof(type)) + iv_msg_size];
struct msghdr msg = {
.msg_control = buffer,
.msg_controllen = sizeof(buffer),
.msg_iov = &iov,
.msg_iovlen = 1,
};

if (!in || !out || !length)
return -EINVAL;

if ((!iv && iv_length) || (iv && !iv_length))
return -EINVAL;

memset(buffer, 0, sizeof(buffer));

/* Set encrypt/decrypt operation */
header = CMSG_FIRSTHDR(&msg);
header->cmsg_level = SOL_ALG;
header->cmsg_type = ALG_SET_OP;
header->cmsg_len = CMSG_LEN(sizeof(type));
type = (void*)CMSG_DATA(header);
*type = direction;

/* Set IV */
if (iv) {
header = CMSG_NXTHDR(&msg, header);
header->cmsg_level = SOL_ALG;
header->cmsg_type = ALG_SET_IV;
header->cmsg_len = iv_msg_size;
alg_iv = (void*)CMSG_DATA(header);
alg_iv->ivlen = iv_length;
memcpy(alg_iv->iv, iv, iv_length);
}

len = sendmsg(opfd, &msg, 0);
if (len != (ssize_t)length) {
r = -EIO;
goto bad;
}

len = read(opfd, out, length);
if (len != (ssize_t)length)
r = -EIO;
bad:
memset(buffer, 0, sizeof(buffer));
return r;
}

int main (int argc, char *argv[])
{
const char key[32] = "0123456789abcdef0123456789abcdef";
const char iv[16] =  "0001";
struct sockaddr_alg sa = {
.salg_family = AF_ALG,
.salg_type = "skcipher",
.salg_name = "cbc(aes)"
};
int tfmfd, opfd;
char *data;

if (posix_memalign((void*)&data, 4096, 32)) {
printf("Cannot alloc memory.\n");
return 1;
}

tfmfd = socket(AF_ALG, SOCK_SEQPACKET, 0);
if (tfmfd == -1)
goto bad;

if (bind(tfmfd, (struct sockaddr *)&sa, sizeof(sa)) == -1)
goto bad;

opfd = accept(tfmfd, NULL, 0);
if (opfd == -1)
goto bad;

if (setsockopt(tfmfd, SOL_ALG, ALG_SET_KEY, key, sizeof(key)) == -1)
goto bad;

if (kernel_crypt(opfd, data, data, 1, iv, sizeof(iv), ALG_OP_ENCRYPT) < 
0)
printf("Cannot encrypt data.\n");

close(tfmfd);
close(opfd);
free(data);
return 0;
bad:
printf("Cannot initialise cipher.\n");
return 1;
}
--
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/

[PATCH] algif_skcipher: Avoid crash if buffer is not multiple of cipher block size

[Milan Broz]

When user requests encryption (or decryption) of block which
is not aligned to cipher block size through userspace crypto
interface, an OOps like this can happen:

[  112.738285] BUG: unable to handle kernel paging request at e1c44840
[  112.738407] IP: [] scatterwalk_done+0x53/0x70
...
[  112.740515] Call Trace:
[  112.740588]  [] blkcipher_walk_done+0x160/0x1e0
[  112.740663]  [] blkcipher_walk_next+0x318/0x3c0
[  112.740737]  [] blkcipher_walk_first+0x70/0x160
[  112.740811]  [] blkcipher_walk_virt+0x17/0x20
[  112.740886]  [] cbc_encrypt+0x29/0x100 [aesni_intel]
[  112.740968]  [] ? get_user_pages_fast+0x123/0x150
[  112.741046]  [] ? trace_hardirqs_on+0xb/0x10
[  112.741119]  [] __ablk_encrypt+0x39/0x40 [ablk_helper]
[  112.741198]  [] ablk_encrypt+0x1a/0x70 [ablk_helper]
[  112.741275]  [] skcipher_recvmsg+0x20c/0x400 [algif_skcipher]
[  112.741359]  [] ? sched_clock_cpu+0x11d/0x1a0
[  112.741435]  [] ? find_get_page+0x79/0xc0
[  112.741509]  [] sock_aio_read+0x104/0x140
[  112.741580]  [] ? __do_fault+0x248/0x420
[  112.741650]  [] do_sync_read+0x97/0xd0
[  112.741719]  [] vfs_read+0x11d/0x140
[  112.741789]  [] ? sys_socketcall+0x2a3/0x320
[  112.741861]  [] sys_read+0x42/0x90
[  112.742578]  [] sysenter_do_call+0x12/0x32

Patch fixes it by simply rejecting buffer which is not multiple of cipher block.

(Bug is present in all stable kernels as well.)

Signed-off-by: Milan Broz 
---
 crypto/algif_skcipher.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 6a6dfc0..5f7713b 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -463,7 +463,7 @@ static int skcipher_recvmsg(struct kiocb *unused, struct 
socket *sock,
used -= used % bs;
 
err = -EINVAL;
-   if (!used)
+   if (!used || used % bs)
goto free;
 
ablkcipher_request_set_crypt(&ctx->req, sg,
-- 
1.7.10.4

--
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] make dm and dm-crypt forward cgroup context

[Milan Broz]

On 12.4.2013 2:22, Tejun Heo wrote:
> On Thu, Apr 11, 2013 at 08:06:10PM -0400, Mikulas Patocka wrote:
>> All that I can tell you is that adding an empty atomic operation 
>> "cmpxchg(&bio->bi_css->refcnt, bio->bi_css->refcnt, bio->bi_css->refcnt);" 
>> to bio_clone_context and bio_disassociate_task increases the time to run a 
>> benchmark from 23 to 40 seconds.
> 
> Right, linear target on ramdisk, very realistic, and you know what,
> hell with dm, let's just hand code everything into submit_bio().  I'm
> sure it will speed up your test case significantly.
> 
> If this actually matters, improve it in *sane* way.  Make the refcnts
> per-cpu and not use atomic ops.  In fact, we already have proposed
> implementation of percpu refcnt which is being used by aio restructure
> patches and likely to be included in some form.  It's not quite ready
> yet, so please work on something useful like that instead of
> continuing this non-sense.

Hey, what's going on here?

Seems dmcrypt problem transformed into block level refcount flame :)

Mikulas, please, can you talk to Tejun and find some better way how
to solve DM & block level context bio contexts here?
(Ideally on some realistic scenario, you have enough hw in Red Hat to try,
some raid0 ssds with linear on top should be good example)
and later (when agreed) implement it on dmcrypt?

I definitely do not want dmcrypt becomes guinea pig here, it should
remain simple as possible and should do transparent _encryption_
and not any inline device-mapper super optimizing games.


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: [dm-crypt] [dm-devel] dm-crypt performance

[Milan Broz]

On 9.4.2013 20:08, Mikulas Patocka wrote:
> 
> 
> On Tue, 26 Mar 2013, Milan Broz wrote:
> 
>> - Are we sure we are not inroducing some another side channel in disc
>> encryption? (Unprivileged user can measure timing here).
>> (Perhaps stupid reason but please do not prefer performance to security
>> in encryption. Enough we have timing attacks for AES implementations...)
> 
> So use serpent - it is implemented without any data-dependent lookup 
> tables, so it has no timing attacks.

I wish using something different than AES is just such simple technical issue
for many people. But e.g. just try it in FIPS mode where AES is the only 
option:-)

Anyway, using bio_associate_current() seems to be the right way to try now...

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: dm-crypt performance

[Milan Broz]

On 26.3.2013 21:28, Mike Snitzer wrote:

On Tue, Mar 26 2013 at  4:05pm -0400,
Milan Broz  wrote:


On Mon, Mar 25, 2013 at 11:47:22PM -0400, Mikulas Patocka wrote:


For best performance we could use the unbound workqueue implementation
with request sorting, if people don't object to the request sorting being
done in dm-crypt.


So again:

- why IO scheduler is not working properly here? Do it need some extensions?
If fixed, it can help even is some other non-dmcrypt IO patterns.
(I mean dmcrypt can set some special parameter for underlying device queue
automagically to fine-tune sorting parameters.)


Not sure, but IO scheduler changes are fairly slow to materialize given
the potential for adverse side-effects.  Are you so surprised that a
shotgun blast of IOs might make the IO schduler less optimal than if
some basic sorting were done at the layer above?


All I said is that I think the problems should be solved on proper layer where
are already all mechanisms to properly control it.
And only if it is not possible then use such workarounds.

CPU bounded io in dmcrypt is in kernel for >2 years and I know about just
few cases where it caused real problems. Maybe I am mistaken - then now is
ideal time for people to complain :)

Anyway, are we talking about the same Mikulas' patch I tested months ago
or you have something new?
I mean this part from series of dmcrypt patches:
http://mbroz.fedorapeople.org/dm-crypt/3.6-rc/dm-crypt-25-sort-writes.patch

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: [dm-devel] dm-crypt performance

[Milan Broz]

On 26.3.2013 13:27, Alasdair G Kergon wrote:

[Adding dm-crypt + linux-kernel]


Thanks.



On Mon, Mar 25, 2013 at 11:47:22PM -0400, Mikulas Patocka wrote:

I performed some dm-crypt performance tests as Mike suggested.

It turns out that unbound workqueue performance has improved somewhere
between kernel 3.2 (when I made the dm-crypt patches) and 3.8, so the
patches for hand-built dispatch are no longer needed.

For RAID-0 composed of two disks with total throughput 260MB/s, the
unbound workqueue performs as well as the hand-built dispatch (both
sustain the 260MB/s transfer rate).

For ramdisk, unbound workqueue performs better than hand-built dispatch
(620MB/s vs 400MB/s). Unbound workqueue with the patch that Mike suggested
(git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git) improves
performance slighlty on ramdisk compared to 3.8 (700MB/s vs. 620MB/s).


I found that ramdisk tests are usualy quite misleading for dmcrypt.
Better use some fast SSD, ideally in RAID0 (so you get >500MB or so).
Also be sure you compare recent machines which uses AES-NI.
For reference, null cipher (no crypt, data copy only) works as well,
but this is not real-world scenario.

After introducing Andi's patches, we created performance regression
for people which created "RAID over several dmcrypt devices".
(All IOs were processed by one core.) Rerely use case but several people
complained.
But most of people reported that current approach works much better
(even with stupid dd test - I think it is because page cache sumbits
requests from different CPUs so it in fact run in parallel).

But using dd with direct-io is trivial way how to simulate the "problem".
(I guess we all like using dd for performance testing... :-])


However, there is still the problem with request ordering. Milan found out
that under some circumstances parallel dm-crypt has worse performance than
the previous dm-crypt code. I found out that this is not caused by
deficiencies in the code that distributes work to individual processors.
Performance drop is caused by the fact that distributing write bios to
multiple processors causes the encryption to finish out of order and the
I/O scheduler is unable to merge these out-of-order bios.


If the IO scheduler is unable to merge these request because of out of order
bios, please try to FIX IO scheduler and do not invent workarounds in dmcrypt.
(With recent accelerated crypto this should not happen so often btw.)

I know it is not easy but I really do not like that in "little-walled
device-mapper garden" is something what should be done on different
layer (again).


The deadline and noop schedulers perform better (only 50% slowdown
compared to old dm-crypt), CFQ performs very badly (8 times slowdown).


If I sort the requests in dm-crypt to come out in the same order as they
were received, there is no longer any slowdown, the new crypt performs as
well as the old crypt, but the last time I submitted the patches, people
objected to sorting requests in dm-crypt, saying that the I/O scheduler
should sort them. But it doesn't. This problem still persists in the
current kernels.


I have probable no vote here anymore but for the record: I am strictly
against any sorting of requests in dmcrypt. My reasons are:

- dmcrypt should be simple transparent layer (doing one thing - encryption),
sorting of requests was always primarily IO scheduler domain
(which has well-known knobs to control it already)

- Are we sure we are not inroducing some another side channel in disc
encryption? (Unprivileged user can measure timing here).
(Perhaps stupid reason but please do not prefer performance to security
in encryption. Enough we have timing attacks for AES implementations...)

- In my testing (several months ago) output was very unstable - in some
situations it helps, in some it was worse. I have no longer hard
data but some test output was sent to Alasdair.


For best performance we could use the unbound workqueue implementation
with request sorting, if people don't object to the request sorting being
done in dm-crypt.


So again:

- why IO scheduler is not working properly here? Do it need some extensions?
If fixed, it can help even is some other non-dmcrypt IO patterns.
(I mean dmcrypt can set some special parameter for underlying device queue
automagically to fine-tune sorting parameters.)

- can we have some cpu-bound workqueue which automatically switch to unbound
(relocates work to another cpu) if it detects some saturation watermark etc?
(Again, this can be used in other code.
http://www.redhat.com/archives/dm-devel/2012-August/msg00288.html
(Yes, I see skepticism there :-)


On Tue, Mar 26, 2013 at 02:52:29AM -0400, Christoph Hellwig wrote:

FYI, XFS also does it's own request ordering for the metadata buffers,
because it knows the needed ordering and has a bigger view than than
than especially CFQ.  You at least have precedence in a widely used
subsystem for this code.


Nice. But XFS is much more complex system.
I

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

[Milan Broz]

On 12/19/2012 11:20 PM, Joseph Salisbury wrote:
> Great work, Milan.  Your patch fixes the bug, stops the panic and allows 
> dm-crypt to function properly.

Thanks.

> 
> Will you be requesting this in v3.8 ?

This should go into 3.7 stable as well, I am talking with Alasdair already
how to handle 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/

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

[Milan Broz]

Does this help?

dm-crypt: never use write same

Ciphertext device is not compatible with WRITE SAME,
disable it for all dmcrypt devices.

Signed-off-by: Milan Broz 

--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1844,6 +1844,12 @@ static int crypt_iterate_devices(struct dm_target *ti,
return fn(ti, cc->dev, cc->start, ti->len, data);
 }
 
+static void crypt_io_hints(struct dm_target *ti,
+   struct queue_limits *limits)
+{
+   limits->max_write_same_sectors = 0;
+}
+
 static struct target_type crypt_target = {
.name   = "crypt",
.version = {1, 11, 0},
@@ -1858,6 +1864,7 @@ static struct target_type crypt_target = {
.message = crypt_message,
.merge  = crypt_merge,
.iterate_devices = crypt_iterate_devices,
+   .io_hints = crypt_io_hints,
 };
 
 static int __init dm_crypt_init(void)

--
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: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[Milan Broz]

On 12/19/2012 08:58 PM, Mike Snitzer wrote:
> On Wed, Dec 19 2012 at 11:58am -0500,
> Martin K. Petersen  wrote:
> 
>>> "Joseph" == Joseph Salisbury  writes:
>>
>> Joseph> I captured the netconsole output from boot until I reproduced
>> Joseph> the bug. The RIP points to kcryptd_crypt_write_io_submit() in
>> Joseph> ~/drivers/md/dm-crypt.c.  The output can be seen at:
>>
>> I'm thinking that dm-crypt should probably set max_write_same_sectors to
>> 0. It doesn't really make much sense for a crypto driver to pass that
>> command through.

IMHO WRITE_SAME doesn't not make sense in encrypted storage. The same sectors
in plaintext are _not_ the same in ciphertext. NEVER.
(If so, you have a security problem.)

It must be disabled in dmcrypt explicitly then (max_write_same_sectors?).

Is it only dmcrypt, or encrypted fs are affected too?

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: Read O_DIRECT regression in 3.7-rc8 (bisected)

[Milan Broz]

On 12/08/2012 01:57 AM, Linus Torvalds wrote:
> 
> On Fri, 7 Dec 2012, Linus Torvalds wrote:
>>
>> This (TOTALLY UNTESTED) patch adds it the same iov_shorten() logic that 
>> the write side has. It does it differently (in fs/block_dev.c rather than 
>> in mm/filemap.c), but I actually suspect this is a nicer way to do it, and 
>> maybe we should do the write side truncation this way too.
> 
> Ok, rebooted and tested with your test-case, and it seems to work fine. 
> But I presume that you actually found the problem some other way, so you'd 
> want to verify that it fixes whatever original bigger issue you hit.

Yes, initially it crashed regression test for cryptsetup reencrypt tool
(direct-io is optional there) and that device reencryption case never finished.
(Which means in fact lost of access to the encrypted device for user
without some manual tweaking, so quite serious problem.)

Anyway, it works now with your patch, so

Reported-and-tested-by: Milan Broz 

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/

Read O_DIRECT regression in 3.7-rc8 (bisected)

[Milan Broz]

Hi Linus,

seems this commit in 3.7-rc8 caused regression for O_DIRECT
read near the end of the device.

bbec0270bdd887f96377065ee38b8848b5afa395 is the first bad commit
commit bbec0270bdd887f96377065ee38b8848b5afa395
Author: Linus Torvalds 
Date:   Thu Nov 29 12:31:52 2012 -0800

blkdev_max_block: make private to fs/buffer.c


With reproducer below (tested on i386), read should return
half of the buffer (8192 bytes), with patch above it fails
completely.

Milan

#define _GNU_SOURCE
#include 
#include 
#include 
#include 

#define BLOCK 8192
int main (int argc, char *argv[])
{
char *buf;
int fd, r;

if (posix_memalign((void*)&buf, 4096, 2 * BLOCK)) {
printf("alloc fail\n");
return 1;
}

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

if (lseek(fd, -BLOCK, SEEK_END) < 0) {
printf("seek fail\n");
close(fd);
return 2;
}

r = read(fd, buf, 2 * BLOCK);
printf("Read returned %d.\n", r);

close(fd);
return 0;
}
--
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: [RFC PATCH 0/4] Remove cryptoloop support (cryptsetup replacement)

[Milan Broz]

On 11/01/2012 09:11 PM, Milan Broz wrote:

> Dm-crypt/cryptsetup provides replacement for long time already.

Just one addition, here are notes how to replace cryptoloop
with cryptsetup.

Because there are several incompatible lostup implementations,
it cannot be automated. But you can easily find proper options
using few switches below.

Note, that some configurations are insecure and often
the insecure variant is mostly the default.

One such example is a passphrase used directly as key (not hashed)
(option -N which is sometimes default) an key is padded.
So with 8 char password and 256bit key 192bits of key is zeroed.

Another problem is with using predictable IV (initial vector, which
is basically just sector number in losetup) and CBC mode.
For CBC mode this is not secure, because it open system to
watermarking attacks.
(Cryptsetup uses ESSIV by default or XTS mode.)

Cryptsetup will not stop you to use such configurations though,
it is designed to allow everything possible if user wants.


LOSETUP -> CRYPTSETUP replacement
~

(N.B. for new images you should you use cryptsetup defaults and get rid
of most compat options.)

Short example for compatible mapping:

  "losetup -e twofish -N /dev/loop0 /image.img"

can be used by cryptsetup

  "cryptsetup create image_plain /image.img -c twofish-cbc-plain -H plain"

(Plaintext device will be /dev/mapper/image_plain instead of /dev/loop0.)


Details
~~~
In Debian and some other distros is hashing default, hash is "ripemd160",
default key size is mostly 256bits. In some distros hashing is not used.

For cryptsetup (plain mode, no LUKS) you will need these options:

 [-c|--cipher]   - cipher specification
 [-s|--key-size] - key size in bits
 [-h|--hash] - password hashing function ("plain" is no hashing)
 [-o|--offset]   - offset (in 512 sectors!)
 [-p|--skip] - IV offset (in 512 sectors!) (usually the same as -o)

Here are some common combinations and cryptsetup replacements:

  losetup parameters |  cryptsetup paramaters
~
  -e twofish [-N]| -c twofish-cbc-plain -H plain [-s 256]
  -e twofish | -c twofish-cbc-plain -H ripemd160 [-s 256]

Ditto for most other ciphers:

  -e aes [-N]| -c aes-cbc-plain -H plain [-s 256]
  -e aes | -c aes-cbc-plain -H ripemd160 [-s 256]
...

Key size (Debian option -k, not upstream)

  -k 128 | -s 128

Specifying offset, note losetup takes bytes, cryptsetup sectors.
Offset must be multiple of sectors (even in losetup).
SKip (-p) means offset for IV, you need specify both here.

  -o 2560| -o 5 -p 5   # 2560/512 = 5

Even some esoteric configurations possible
(This example was default for some very old SuSe distro.)

Loop using twofish with only 160 bit key (zero padded to 192 bits) with no IV.

   cryptsetup -c twofish-cbc-null -s 192 -h ripemd160:20

Cryptsetup doesn't provide direct replacement for --pass-fd option
(pass file descriptor to read key) but you can use directly keyfile
option and keyfile size (see cryptsetup man page).

Also note, that cryptsetup allows mapping of loop-aes (multi-key) devices
(see loopaesOpen command in man page).
But this was never part of mainline kernel, so it is not important
for mainline cryptoloop replacement.

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/

[RFC PATCH 1/4] Remove cryptoloop module.

[Milan Broz]

Mainline cryptoloop extension to block device loop driver is for years in limbo.

There are known problems (like predictable IV). This code is already
superseded by using dmcrypt/cryptsetup.
(Cryptsetup automatically allocates free loop device if run over file image.)

Other implementation, out of mainline tree loop-AES is already heavily
patching loop driver.

Current mainline userspace (util-linux) is going to remove encryption support
in next version (already removed in git tree), encryption support is already
deprecated in last release.

Let's stop pretend this code is supported and remove it completely.

Signed-off-by: Milan Broz 
---
 drivers/block/Makefile |1 -
 drivers/block/cryptoloop.c |  216 
 2 files changed, 217 deletions(-)
 delete mode 100644 drivers/block/cryptoloop.c

diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index 17e82df..d0bd32f 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -28,7 +28,6 @@ obj-$(CONFIG_BLK_DEV_OSD) += osdblk.o
 
 obj-$(CONFIG_BLK_DEV_UMEM) += umem.o
 obj-$(CONFIG_BLK_DEV_NBD)  += nbd.o
-obj-$(CONFIG_BLK_DEV_CRYPTOLOOP) += cryptoloop.o
 obj-$(CONFIG_VIRTIO_BLK)   += virtio_blk.o
 
 obj-$(CONFIG_VIODASD)  += viodasd.o
diff --git a/drivers/block/cryptoloop.c b/drivers/block/cryptoloop.c
deleted file mode 100644
index 8b6bb76..000
--- a/drivers/block/cryptoloop.c
+++ /dev/null
@@ -1,216 +0,0 @@
-/*
-   Linux loop encryption enabling module
-
-   Copyright (C)  2002 Herbert Valerio Riedel 
-   Copyright (C)  2003 Fruhwirth Clemens 
-
-   This module is free software; you can redistribute it and/or modify
-   it under the terms of the GNU General Public License as published by
-   the Free Software Foundation; either version 2 of the License, or
-   (at your option) any later version.
-
-   This module is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-   GNU General Public License for more details.
-
-   You should have received a copy of the GNU General Public License
-   along with this module; if not, write to the Free Software
-   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
- */
-
-#include 
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-MODULE_LICENSE("GPL");
-MODULE_DESCRIPTION("loop blockdevice transferfunction adaptor / CryptoAPI");
-MODULE_AUTHOR("Herbert Valerio Riedel ");
-
-#define LOOP_IV_SECTOR_BITS 9
-#define LOOP_IV_SECTOR_SIZE (1 << LOOP_IV_SECTOR_BITS)
-
-static int
-cryptoloop_init(struct loop_device *lo, const struct loop_info64 *info)
-{
-   int err = -EINVAL;
-   int cipher_len;
-   int mode_len;
-   char cms[LO_NAME_SIZE]; /* cipher-mode string */
-   char *cipher;
-   char *mode;
-   char *cmsp = cms;   /* c-m string pointer */
-   struct crypto_blkcipher *tfm;
-
-   /* encryption breaks for non sector aligned offsets */
-
-   if (info->lo_offset % LOOP_IV_SECTOR_SIZE)
-   goto out;
-
-   strncpy(cms, info->lo_crypt_name, LO_NAME_SIZE);
-   cms[LO_NAME_SIZE - 1] = 0;
-
-   cipher = cmsp;
-   cipher_len = strcspn(cmsp, "-");
-
-   mode = cmsp + cipher_len;
-   mode_len = 0;
-   if (*mode) {
-   mode++;
-   mode_len = strcspn(mode, "-");
-   }
-
-   if (!mode_len) {
-   mode = "cbc";
-   mode_len = 3;
-   }
-
-   if (cipher_len + mode_len + 3 > LO_NAME_SIZE)
-   return -EINVAL;
-
-   memmove(cms, mode, mode_len);
-   cmsp = cms + mode_len;
-   *cmsp++ = '(';
-   memcpy(cmsp, info->lo_crypt_name, cipher_len);
-   cmsp += cipher_len;
-   *cmsp++ = ')';
-   *cmsp = 0;
-
-   tfm = crypto_alloc_blkcipher(cms, 0, CRYPTO_ALG_ASYNC);
-   if (IS_ERR(tfm))
-   return PTR_ERR(tfm);
-
-   err = crypto_blkcipher_setkey(tfm, info->lo_encrypt_key,
- info->lo_encrypt_key_size);
-   
-   if (err != 0)
-   goto out_free_tfm;
-
-   lo->key_data = tfm;
-   return 0;
-
- out_free_tfm:
-   crypto_free_blkcipher(tfm);
-
- out:
-   return err;
-}
-
-
-typedef int (*encdec_cbc_t)(struct blkcipher_desc *desc,
-   struct scatterlist *sg_out,
-   struct scatterlist *sg_in,
-   unsigned int nsg);
-
-static int
-cryptoloop_transfer(struct loop_device *lo, int cmd,
-   struct page *raw_page, unsigned raw_off,
-   struct page *loop_page, unsigned loop_off,
-   int size, sector_t IV)
-{
-   struct crypto_blkcipher *tfm = lo->key_data;
-

[RFC PATCH 4/4] Remove transfer module support in loop.

[Milan Broz]

Remove transfer function support from loop driver.
The only user for it is for years XOR "encryption" and cryptoloop.

If userspace request encryption, EINVAL is returned.
The userspace visible struct is unchanged (just marked deprecated),
so old userspace will just see ioctl failure if encryption is requested.

Signed-off-by: Milan Broz 
---
 drivers/block/loop.c |  344 +-
 include/linux/loop.h |   31 -
 2 files changed, 35 insertions(+), 340 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 54046e5..4025227 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -85,81 +85,6 @@ static DEFINE_MUTEX(loop_index_mutex);
 static int max_part;
 static int part_shift;
 
-/*
- * Transfer functions
- */
-static int transfer_none(struct loop_device *lo, int cmd,
-struct page *raw_page, unsigned raw_off,
-struct page *loop_page, unsigned loop_off,
-int size, sector_t real_block)
-{
-   char *raw_buf = kmap_atomic(raw_page) + raw_off;
-   char *loop_buf = kmap_atomic(loop_page) + loop_off;
-
-   if (cmd == READ)
-   memcpy(loop_buf, raw_buf, size);
-   else
-   memcpy(raw_buf, loop_buf, size);
-
-   kunmap_atomic(loop_buf);
-   kunmap_atomic(raw_buf);
-   cond_resched();
-   return 0;
-}
-
-static int transfer_xor(struct loop_device *lo, int cmd,
-   struct page *raw_page, unsigned raw_off,
-   struct page *loop_page, unsigned loop_off,
-   int size, sector_t real_block)
-{
-   char *raw_buf = kmap_atomic(raw_page) + raw_off;
-   char *loop_buf = kmap_atomic(loop_page) + loop_off;
-   char *in, *out, *key;
-   int i, keysize;
-
-   if (cmd == READ) {
-   in = raw_buf;
-   out = loop_buf;
-   } else {
-   in = loop_buf;
-   out = raw_buf;
-   }
-
-   key = lo->lo_encrypt_key;
-   keysize = lo->lo_encrypt_key_size;
-   for (i = 0; i < size; i++)
-   *out++ = *in++ ^ key[(i & 511) % keysize];
-
-   kunmap_atomic(loop_buf);
-   kunmap_atomic(raw_buf);
-   cond_resched();
-   return 0;
-}
-
-static int xor_init(struct loop_device *lo, const struct loop_info64 *info)
-{
-   if (unlikely(info->lo_encrypt_key_size <= 0))
-   return -EINVAL;
-   return 0;
-}
-
-static struct loop_func_table none_funcs = {
-   .number = LO_CRYPT_NONE,
-   .transfer = transfer_none,
-}; 
-
-static struct loop_func_table xor_funcs = {
-   .number = LO_CRYPT_XOR,
-   .transfer = transfer_xor,
-   .init = xor_init
-}; 
-
-/* xfer_funcs[0] is special - its release function is never called */
-static struct loop_func_table *xfer_funcs[MAX_LO_CRYPT] = {
-   &none_funcs,
-   &xor_funcs
-};
-
 static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
 {
loff_t size, loopsize;
@@ -205,12 +130,20 @@ static inline int
 lo_do_transfer(struct loop_device *lo, int cmd,
   struct page *rpage, unsigned roffs,
   struct page *lpage, unsigned loffs,
-  int size, sector_t rblock)
+  int size)
 {
-   if (unlikely(!lo->transfer))
-   return 0;
+   char *raw_buf = kmap_atomic(rpage) + roffs;
+   char *loop_buf = kmap_atomic(lpage) + loffs;
 
-   return lo->transfer(lo, cmd, rpage, roffs, lpage, loffs, size, rblock);
+   if (cmd == READ)
+   memcpy(loop_buf, raw_buf, size);
+   else
+   memcpy(raw_buf, loop_buf, size);
+
+   kunmap_atomic(loop_buf);
+   kunmap_atomic(raw_buf);
+   cond_resched();
+   return 0;
 }
 
 /**
@@ -244,7 +177,7 @@ static int __do_lo_send_write(struct file *file,
  * buffering.
  */
 static int do_lo_send_direct_write(struct loop_device *lo,
-   struct bio_vec *bvec, loff_t pos, struct page *page)
+   struct bio_vec *bvec, loff_t pos)
 {
ssize_t bw = __do_lo_send_write(lo->lo_backing_file,
kmap(bvec->bv_page) + bvec->bv_offset,
@@ -254,63 +187,19 @@ static int do_lo_send_direct_write(struct loop_device *lo,
return bw;
 }
 
-/**
- * do_lo_send_write - helper for writing data to a loop device
- *
- * This is the slow, transforming version that needs to double buffer the
- * data as it cannot do the transformations in place without having direct
- * access to the destination pages of the backing file.
- */
-static int do_lo_send_write(struct loop_device *lo, struct bio_vec *bvec,
-   loff_t pos, struct page *page)
-{
-   int ret = lo_do_transfer(lo, WRITE, page, 0, bvec->bv_page,
-   bvec->bv_offset, bvec->bv_len, pos >> 9);
-   if (likely(!ret))
-   return __do_lo_send_write(lo-

[RFC PATCH 3/4] Deprecate loop crypto ioctl fields.

[Milan Broz]

Mark encryption loop fields in userspace API deprecated.

Signed-off-by: Milan Broz 
---
 include/uapi/linux/loop.h |   18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/uapi/linux/loop.h b/include/uapi/linux/loop.h
index e0cecd2..d0896c7 100644
--- a/include/uapi/linux/loop.h
+++ b/include/uapi/linux/loop.h
@@ -11,7 +11,7 @@
 
 
 #define LO_NAME_SIZE   64
-#define LO_KEY_SIZE32
+#define LO_KEY_SIZE32 /* deprecated */
 
 
 /*
@@ -33,11 +33,11 @@ struct loop_info {
unsigned long  lo_inode;/* ioctl r/o */
__kernel_old_dev_t lo_rdevice;  /* ioctl r/o */
intlo_offset;
-   intlo_encrypt_type;
-   intlo_encrypt_key_size; /* ioctl w/o */
+   intlo_encrypt_type; /* deprecated */
+   intlo_encrypt_key_size; /* deprecated */
intlo_flags;/* ioctl r/o */
char   lo_name[LO_NAME_SIZE];
-   unsigned char  lo_encrypt_key[LO_KEY_SIZE]; /* ioctl w/o */
+   unsigned char  lo_encrypt_key[LO_KEY_SIZE]; /* deprecated */
unsigned long  lo_init[2];
char   reserved[4];
 };
@@ -49,17 +49,17 @@ struct loop_info64 {
__u64  lo_offset;
__u64  lo_sizelimit;/* bytes, 0 == max available */
__u32  lo_number;   /* ioctl r/o */
-   __u32  lo_encrypt_type;
-   __u32  lo_encrypt_key_size; /* ioctl w/o */
+   __u32  lo_encrypt_type; /* deprecated */
+   __u32  lo_encrypt_key_size; /* deprecated */
__u32  lo_flags;/* ioctl r/o */
__u8   lo_file_name[LO_NAME_SIZE];
-   __u8   lo_crypt_name[LO_NAME_SIZE];
-   __u8   lo_encrypt_key[LO_KEY_SIZE]; /* ioctl w/o */
+   __u8   lo_crypt_name[LO_NAME_SIZE]; /* deprecated */
+   __u8   lo_encrypt_key[LO_KEY_SIZE]; /* deprecated */
__u64  lo_init[2];
 };
 
 /*
- * Loop filter types
+ * Loop filter types (deprecated)
  */
 
 #define LO_CRYPT_NONE  0
-- 
1.7.10.4

--
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/

[RFC PATCH 2/4] Remove cryptoloop config option.

[Milan Broz]

Remove use of CONFIG_BLK_DEV_CRYPTOLOOP across the tree.

Signed-off-by: Milan Broz 
---
 arch/arm/configs/colibri_pxa270_defconfig  |1 -
 arch/arm/configs/ezx_defconfig |1 -
 arch/arm/configs/imote2_defconfig  |1 -
 arch/arm/configs/lpc32xx_defconfig |1 -
 arch/arm/configs/netx_defconfig|1 -
 arch/arm/configs/nhk8815_defconfig |1 -
 arch/arm/configs/trizeps4_defconfig|1 -
 arch/ia64/configs/bigsur_defconfig |1 -
 arch/ia64/configs/generic_defconfig|1 -
 arch/ia64/configs/gensparse_defconfig  |1 -
 arch/ia64/configs/tiger_defconfig  |1 -
 arch/ia64/configs/xen_domu_defconfig   |1 -
 arch/m68k/configs/amiga_defconfig  |1 -
 arch/m68k/configs/apollo_defconfig |1 -
 arch/m68k/configs/atari_defconfig  |1 -
 arch/m68k/configs/bvme6000_defconfig   |1 -
 arch/m68k/configs/hp300_defconfig  |1 -
 arch/m68k/configs/mac_defconfig|1 -
 arch/m68k/configs/multi_defconfig  |1 -
 arch/m68k/configs/mvme147_defconfig|1 -
 arch/m68k/configs/mvme16x_defconfig|1 -
 arch/m68k/configs/q40_defconfig|1 -
 arch/m68k/configs/sun3_defconfig   |1 -
 arch/m68k/configs/sun3x_defconfig  |1 -
 arch/mips/configs/bcm47xx_defconfig|1 -
 arch/mips/configs/bigsur_defconfig |1 -
 arch/mips/configs/fuloong2e_defconfig  |1 -
 arch/mips/configs/ip27_defconfig   |1 -
 arch/mips/configs/ip32_defconfig   |1 -
 arch/mips/configs/jazz_defconfig   |1 -
 arch/mips/configs/lemote2f_defconfig   |1 -
 arch/mips/configs/malta_defconfig  |1 -
 arch/mips/configs/markeins_defconfig   |1 -
 arch/mips/configs/nlm_xlp_defconfig|1 -
 arch/mips/configs/nlm_xlr_defconfig|1 -
 arch/mips/configs/rm200_defconfig  |1 -
 arch/mips/configs/sead3_defconfig  |1 -
 arch/parisc/configs/712_defconfig  |1 -
 arch/parisc/configs/b180_defconfig |1 -
 arch/parisc/configs/c3000_defconfig|1 -
 arch/parisc/configs/default_defconfig  |1 -
 arch/powerpc/configs/85xx/ge_imp3a_defconfig   |1 -
 arch/powerpc/configs/86xx/gef_ppc9a_defconfig  |1 -
 arch/powerpc/configs/86xx/gef_sbc310_defconfig |1 -
 arch/powerpc/configs/86xx/gef_sbc610_defconfig |1 -
 arch/powerpc/configs/86xx/sbc8641d_defconfig   |1 -
 arch/powerpc/configs/c2k_defconfig |1 -
 arch/powerpc/configs/chroma_defconfig  |1 -
 arch/powerpc/configs/ppc6xx_defconfig  |1 -
 arch/score/configs/spct6600_defconfig  |1 -
 arch/sh/configs/sdk7786_defconfig  |1 -
 arch/sh/configs/sh7785lcr_32bit_defconfig  |1 -
 arch/sh/configs/titan_defconfig|1 -
 arch/sparc/configs/sparc32_defconfig   |1 -
 arch/sparc/configs/sparc64_defconfig   |1 -
 arch/tile/configs/tilegx_defconfig |1 -
 arch/tile/configs/tilepro_defconfig|1 -
 arch/um/defconfig  |1 -
 drivers/block/Kconfig  |   23 ---
 59 files changed, 81 deletions(-)

diff --git a/arch/arm/configs/colibri_pxa270_defconfig 
b/arch/arm/configs/colibri_pxa270_defconfig
index 2ef2c5e..5d8530a 100644
--- a/arch/arm/configs/colibri_pxa270_defconfig
+++ b/arch/arm/configs/colibri_pxa270_defconfig
@@ -79,7 +79,6 @@ CONFIG_MTD_NAND_DISKONCHIP_PROBE_HIGH=y
 CONFIG_MTD_NAND_DISKONCHIP_BBTWRITE=y
 CONFIG_MTD_ONENAND=y
 CONFIG_BLK_DEV_LOOP=y
-CONFIG_BLK_DEV_CRYPTOLOOP=m
 CONFIG_BLK_DEV_NBD=y
 CONFIG_BLK_DEV_RAM=y
 CONFIG_BLK_DEV_RAM_COUNT=8
diff --git a/arch/arm/configs/ezx_defconfig b/arch/arm/configs/ezx_defconfig
index d95763d..8c124c0 100644
--- a/arch/arm/configs/ezx_defconfig
+++ b/arch/arm/configs/ezx_defconfig
@@ -187,7 +187,6 @@ CONFIG_MTD_OTP=y
 CONFIG_MTD_CFI_INTELEXT=y
 CONFIG_MTD_PXA2XX=y
 CONFIG_BLK_DEV_LOOP=m
-CONFIG_BLK_DEV_CRYPTOLOOP=m
 CONFIG_BLK_DEV_NBD=m
 CONFIG_BLK_DEV_RAM=y
 # CONFIG_MISC_DEVICES is not set
diff --git a/arch/arm/configs/imote2_defconfig 
b/arch/arm/configs/imote2_defconfig
index fd996bb..2866a41 100644
--- a/arch/arm/configs/imote2_defconfig
+++ b/arch/arm/configs/imote2_defconfig
@@ -167,7 +167,6 @@ CONFIG_MTD_OTP=y
 CONFIG_MTD_CFI_INTELEXT=y
 CONFIG_MTD_PXA2XX=y
 CONFIG_BLK_DEV_LOOP=m
-CONFIG_BLK_DEV_CRYPTOLOOP=m
 CONFIG_BLK_DEV_NBD=m
 CONFIG_BLK_DEV_RAM=y
 # CONFIG_MISC_DEVICES is not set
diff --git a/arch/arm/configs/lpc32xx_defconfig 
b/arch/arm/configs/lpc32xx_defconfig
index 92386b2..b3f923d 100644
--- a/arch/arm/configs/lpc32xx_defconfig
+++ b/arch/arm/configs/lpc32xx_defconfig
@@ -59,7 +59,6

[RFC PATCH 0/4] Remove cryptoloop support

[Milan Broz]

Hi,

after several "is cryptoloop supported/secure/maintained" discussions
(which regularly repeats for years on various occasions) we should do something.

So let's remove cryptoloop support from kernel :-)

Current mainline userspace (util-linux) is going to remove encryption support
in next losetup version (already removed in git tree), encryption support is
already deprecated in the last release.

There are known problems with cryptoloop (like predictable IV or hardcoded 
sizes),
and if you read even some very old notes, nothing changed for years.
(e.g. this page http://mareichelt.com/pub/texts.cryptoloop.html)

Dm-crypt/cryptsetup provides replacement for long time already.
It allocates loop device automatically for file images and with proper 
parameters
it can map existing images - even some old which are not supported by current
cryptoloop. On the other side it requires device-mapper modules (dm_mod, 
dm_crypt).

Alternative (out of tree) loop-AES already replaces most of the kernel
and userpsace code by own patches anyway.

I am not fan of removing old code this way but I do not see alternative here.
Please comment if you see better solution...

Thanks,
Milan

Milan Broz (4):
  Remove cryptoloop module.
  Remove cryptoloop config option.
  Deprecate loop crypto ioctl fields.
  Remove transfer module support in loop.

 arch/arm/configs/colibri_pxa270_defconfig  |1 -
 arch/arm/configs/ezx_defconfig |1 -
 arch/arm/configs/imote2_defconfig  |1 -
 arch/arm/configs/lpc32xx_defconfig |1 -
 arch/arm/configs/netx_defconfig|1 -
 arch/arm/configs/nhk8815_defconfig |1 -
 arch/arm/configs/trizeps4_defconfig|1 -
 arch/ia64/configs/bigsur_defconfig |1 -
 arch/ia64/configs/generic_defconfig|1 -
 arch/ia64/configs/gensparse_defconfig  |1 -
 arch/ia64/configs/tiger_defconfig  |1 -
 arch/ia64/configs/xen_domu_defconfig   |1 -
 arch/m68k/configs/amiga_defconfig  |1 -
 arch/m68k/configs/apollo_defconfig |1 -
 arch/m68k/configs/atari_defconfig  |1 -
 arch/m68k/configs/bvme6000_defconfig   |1 -
 arch/m68k/configs/hp300_defconfig  |1 -
 arch/m68k/configs/mac_defconfig|1 -
 arch/m68k/configs/multi_defconfig  |1 -
 arch/m68k/configs/mvme147_defconfig|1 -
 arch/m68k/configs/mvme16x_defconfig|1 -
 arch/m68k/configs/q40_defconfig|1 -
 arch/m68k/configs/sun3_defconfig   |1 -
 arch/m68k/configs/sun3x_defconfig  |1 -
 arch/mips/configs/bcm47xx_defconfig|1 -
 arch/mips/configs/bigsur_defconfig |1 -
 arch/mips/configs/fuloong2e_defconfig  |1 -
 arch/mips/configs/ip27_defconfig   |1 -
 arch/mips/configs/ip32_defconfig   |1 -
 arch/mips/configs/jazz_defconfig   |1 -
 arch/mips/configs/lemote2f_defconfig   |1 -
 arch/mips/configs/malta_defconfig  |1 -
 arch/mips/configs/markeins_defconfig   |1 -
 arch/mips/configs/nlm_xlp_defconfig|1 -
 arch/mips/configs/nlm_xlr_defconfig|1 -
 arch/mips/configs/rm200_defconfig  |1 -
 arch/mips/configs/sead3_defconfig  |1 -
 arch/parisc/configs/712_defconfig  |1 -
 arch/parisc/configs/b180_defconfig |1 -
 arch/parisc/configs/c3000_defconfig|1 -
 arch/parisc/configs/default_defconfig  |1 -
 arch/powerpc/configs/85xx/ge_imp3a_defconfig   |1 -
 arch/powerpc/configs/86xx/gef_ppc9a_defconfig  |1 -
 arch/powerpc/configs/86xx/gef_sbc310_defconfig |1 -
 arch/powerpc/configs/86xx/gef_sbc610_defconfig |1 -
 arch/powerpc/configs/86xx/sbc8641d_defconfig   |1 -
 arch/powerpc/configs/c2k_defconfig |1 -
 arch/powerpc/configs/chroma_defconfig  |1 -
 arch/powerpc/configs/ppc6xx_defconfig  |1 -
 arch/score/configs/spct6600_defconfig  |1 -
 arch/sh/configs/sdk7786_defconfig  |1 -
 arch/sh/configs/sh7785lcr_32bit_defconfig  |1 -
 arch/sh/configs/titan_defconfig|1 -
 arch/sparc/configs/sparc32_defconfig   |1 -
 arch/sparc/configs/sparc64_defconfig   |1 -
 arch/tile/configs/tilegx_defconfig |1 -
 arch/tile/configs/tilepro_defconfig|1 -
 arch/um/defconfig  |1 -
 drivers/block/Kconfig  |   23 --
 drivers/block/Makefile |1 -
 drivers/block/cryptoloop.c |  216 ---
 drivers/block/loop.c   |  344 +++-
 include/linux/loop.h   |   31 ---
 inc

[PATCH] Fix crypto api init for 3.6.4-rt10

[Milan Broz]

Fix crypto api for 3.6.4-rt10 (broken only in realtime patchset)

In peterz-srcu-crypto-chain.patch the blocking notifier is changed
to srcu notifier and added initialization to module init fucntion.

Later, in crypto-make-core-static-and-init-scru-early.patch, is that
initialization added also to core_initcall().

This patch removes crypto_chain init from algapi initialization,
because this function is called later and already initialized
cryptomgr notifier is lost.

This cause a failure in initialization of larval algorithms,
like e.g. cbc(aes).

Signed-off-by: Milan Broz 

--- crypto/algapi.c.old 2012-10-30 16:11:23.0 +0100
+++ crypto/algapi.c 2012-10-30 16:12:14.988847944 +0100
@@ -956,7 +956,6 @@ EXPORT_SYMBOL_GPL(crypto_xor);
 
 static int __init crypto_algapi_init(void)
 {
-   srcu_init_notifier_head(&crypto_chain);
crypto_init_proc();
return 0;
 }


--
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: [dm-crypt] cryptsetup not working under 3.6 - RT patch set seem to break it

[Milan Broz]

On 10/30/2012 10:40 AM, Uwe Kleine-König wrote:
>> Ha, this is exciting, vanilla 3.6.4 works, with -rt10 patch it does not.
> I experienced that problem, too. This is on a laptop with encrypted
> rootfs, so debugging is a bit harder here. I am just setting up another
> machine to debug that.

FYI the problematic patch is peterz-srcu-crypto-chain.patch in rt10
patchset, after that, crypto_alloc_ablkcipher() now returns -ENOENT (-2)
in dmcrypt.

(I will check it later but maybe someone already playing with 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: [dm-crypt] cryptsetup not working under 3.6 - regression from 3.4?

[Milan Broz]

On 10/29/2012 09:14 PM, Tvrtko Ursulin wrote:
> Unless RT patchset is the culprit. Hm.. that would be unexpected, but I 
> guess it is worth a shot. I'll let you know what happens without -rt.

Yes, try not patched mainline first.
Focus on the crypto api - if the crypto modules are not loading automatically, 
as you
said in previous mails, something is misconfigured there - it can be even
related to userspace tools (modprobe, udev, ...) check that you have recent 
versions.

It works for me here after "make oldconfig" with the .config you sent...

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: [dm-crypt] cryptsetup not working under 3.6 - regression from 3.4?

[Milan Broz]

On 10/29/2012 08:31 PM, Tvrtko Ursulin wrote:
> Just tried 3.6.4 and it is still broken. Is there anything else I could 
> try to debug this?

See response from response from Ondra on dmcrypt list - your kernel config
is perhaps broken.

Please use fresh checkout, run make oldconfig and try again.
(Many people use 3.6 already and there is no other report.)

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: cryptsetup not working under 3.6 - regression from 3.4?

[Milan Broz]

On 10/21/2012 02:36 PM, Tvrtko Ursulin wrote:
> On 21/10/12 13:20, Zdenek Kaspar wrote:
 I would say you are still missing some modules.

> Kernel says this:
> device-mapper: table: 252:1: crypt: Error allocating crypto tfm
> device-mapper: ioctl: error adding target to table

 It complains about aes-cbc-essiv:sha256.

 It can be missing CBC od SHA256, but according the message I bet
 you have no "cbc" block cipher mode module compiled.

 Can you grep your final .config for CONFIG_CRYPTO_CBC and
 CONFIG_CRYPTO_SHA256 a paste it here?
>>>
>>> I both working 3.4 and non-working 3.6 situation is the same:
>>>
>>> CONFIG_CRYPTO_CBC=y
>>> CONFIG_CRYPTO_SHA256=m
>>
>> Compare please:
>> grep CONFIG_CRYPTO /boot/config-3.4
>> grep CONFIG_CRYPTO /boot/config-3.6
>>
>> One of the problem could be that your configuration misses something
>> like: CONFIG_CRYPTO_BLKCIPHER, CONFIG_CRYPTO_MANAGER, etc.. or some of
>> those could changed into modules and are not getting loaded..
> 
> Here it is:

Hm, so it should work without problem. Can you paste full output of failing
cryptsetup command (with added --debug switch) and your full kernel .config?

Cryptsetup itself has regression tests which test almost all common
combinations of ciphers so the problem is almost surely in some kernel
part misconfiguration. 
Any other related messages in syslog beside two lines you posted?

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: [dm-crypt] cryptsetup not working under 3.6 - regression from 3.4?

[Milan Broz]

On 10/20/2012 10:44 PM, Tvrtko Ursulin wrote:
> But I repeat, even if I load the required modules before hand things do 
> not work.

I would say you are still missing some modules.

> Kernel says this:
>   device-mapper: table: 252:1: crypt: Error allocating crypto tfm
>   device-mapper: ioctl: error adding target to table

It complains about aes-cbc-essiv:sha256.

It can be missing CBC od SHA256, but according the message I bet
you have no "cbc" block cipher mode module compiled.

Can you grep your final .config for CONFIG_CRYPTO_CBC and 
CONFIG_CRYPTO_SHA256 a paste it 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: PROBLEM: [dm-crypt] read starvation during writeback

[Milan Broz]

On 10/12/2012 09:37 PM, Michael Zugelder wrote:
> I'm having an issue reading data via dm-crypt when there is heavy
> writeback going on (i.e. copying large files or using dd). A single
> read takes anywhere from a few seconds to multiple minutes.
> 
> Testing setup:
>  * Fedora 17, stock 3.5.4-2.fc17 kernel and a self-compiled 3.6.1 kernel
>  * 320 GiB USB hard drive (sdb)

I guess that USB is the key factor here... I remember to have similar
problem some time ago even without dmcrypt.

Is it reproducible with the same kernel cfg but with internal disk?

You can also test completely fake underlying device,
use device-mapper- zero target:
dmsetup create dev_zero --table "0  zero"
(All writes are dropped and all reads returns zero in this case.)

Is there any starvation with this setup? (It shouldn't.)

>  * dmsetup library version 1.02.74 (2012-03-06), driver version 4.23.0
>  * dm-crypt set up using cipher_null:
>echo "0 $(blockdev --getsize /dev/sdb) crypt cipher_null - 0 /dev/sdb 
> 0"|dmsetup create test

Btw you can use cryptsetup with cipher "null" to simplify
(I added to cryptsetup to test exactly such scenarios).

So, it means that encryption is not the problem.
The crypto_null will cause that there is no crypto operation
(it is just plain data copy) but the remaining dmcrypt overhead remains
exactly the same.

> * writeback induced by running 'dd if=/dev/zero of=$target bs=1M'

Any change if you use oflag=direct ? (iow using direct io)

> I experimented a bit with the other device mapper targets, namely linear
> and stripe, but both worked completely fine. I also tried putting a
> linear mapping above dm-crypt, with no impact on performance. Comparing
> the content of the /sys/block/$DEV files of the linear mapping and
> dm-crypt, there are no differences beside the name, dev no, stats,
> uevent and inflight files.

There is crucial difference between linear/stripe and dmcrypt:
linear just remaps IO target device, dmcrypt queues operations
(using kernel workqueue) and creates full bio clones.
So comparison here is IMHO not much helpful.

There are two internal dmcrypt queues, but I think that the problem
is triggered by some combination with USB storage backend.

> Any pointers would be appreciated, I haven't found much on the web about
> this issue.

Btw there was a proposed rewrite of internal dmcrypt queues, if you have time,
you can try if it changes anything for your use case.
Patches in dm-devel archive
http://www.redhat.com/archives/dm-devel/2012-August/msg00210.html

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/1] dm-integrity: integrity protection device-mapper target

[Milan Broz]

On 09/24/2012 06:20 PM, Kasatkin, Dmitry wrote:

>> So it can provide confidentiality but it CANNOT provide integrity protection.
>>
> Yes, it provides confidentiality and via encryption it provides
> certain level of integrity protection.
> Data cannot be modified without being detected.
> Decryption will result in garbage...

It should, but it is not cryptographic integrity protection.
Moreover, you can revert ciphertext sector content.

Btw I think dm-integrity doesn't solve the reply problem as well.

(IOW can you replace/revert both data and hash, still using the same key
in keyring, without dm-integrity able to detect it?)

In dm-verity, root hash will change after such data tampering.

>> Obvious question: can be dm-verity extended to provide read-write integrity?
>>
> 
> I think re-calculating hash trees all the time and syncing block
> hashes and tree itself is heavier operation...

Then why not extend dm-verity to use you hash format? There are options
for that and we can redefine table line if necessary.

(You are duplicating a lot of code here otherwise, not counting userspace yet.)

Whatever, I shortly read the code and have some notes.

First, device-mapper is variable system, you can stack devices in arbitrary 
order.

Unfortunately, this is something what can be dangerous in crypto, here you can 
e.g.
 - do mac (integrity) then encrypt
 - do encrypt, then check integrity

In many implementations mac-then-encrypt system was not secure.
Are we sure that it cannot provide side channels here?

Note read-only integrity target (like dm-verity) is specific case, you should
not be able to run any chosen ciphertext attacks here, it is read-only device.

In fact, I am not sure if we should provide separate read-write block integrity
(wihtout encryption) target at all.

Either it should use standard mode of authenticated encryption (like GCM)
or data integrity should be upper layer business (which knows better which data
really need integrity checking and which areas are just unused garbage.
If you consider hashing and encryption as "heavy operation" you should minimize
it to only really used area - and only upper layer know about real used data 
content.)

(Again, this is different for read-only target, where it usually uses compressed
RO fs image which uses all available space.)


1) discards
It seems the dm-integrity target doesn't solve problem with discards.

If you send discard request to underlying device, data content of discarded area
is undefined (or zeroed if discards zeroes data) but is is definitely "invalid"
form the dm-integrity point of view. And you are allowing discard IOs there.

I am not sure what should happen, but the behaviour should be documented (at 
least)
or disabled.

2)
All used hash algorithms must be configurable.

>From your doc:

+While NIST does recommend to use SHA256 hash algorithm instead of SHA1,
+this does not apply to HMAC(SHA1), because of keying.
+This target uses HMAC(SHA1), because it takes much less space and it is
+faster to calculate. There is no parameter to change hmac hash algorithm.

While this is technically true, http://csrc.nist.gov/groups/ST/hash/policy.html
disallowing use of another hash algorithm is wrong, and in fact it is regression
in comparison with dm-verity (which already implements all needed calculations
and uses hash name as table line option).

+HMAC(SHA1) size is 20 bytes. So every 4k block on the integrity device can
+store 204 hmacs. In order to get the required size of the integrity device,
+it is necessary to divide data device size by 204.

Why are you hardcoding block and hash sizes?
Again, dm-verity have this configurable with some good default.

ditto for kernel keyring:
+   const char *hmac_algo = "hmac(sha1)"; /* uncompromized yet */

3)
I like that this target can use kernel keyring, in fact, we should implement
similar option to dm crypt/verity. But the target should not be limited
to use TPM keys only.
(And key is stored directly in kernel memory later for hash calculation anyway
allowing hw attacks reading it from RAM.)

4)
reboot notifier cannot be used this way in generic storage stacking IMHO,
but I think there was discussion with Mikulas already
http://www.redhat.com/archives/dm-devel/2012-March/msg00164.html

5)
output target table should not translate device to device symbolic names

Anyway, code can be changed. But the high level questions remains...

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/1] dm-integrity: integrity protection device-mapper target

[Milan Broz]

On 09/24/2012 11:55 AM, Dmitry Kasatkin wrote:
> Both dm-verity and dm-crypt provide block level integrity protection.

This is not correct. dm-crypt is transparent block encryption target,
where always size of plaintext == size of ciphertext.

So it can provide confidentiality but it CANNOT provide integrity protection.

We need extra space to store auth tag which dmcrypt cannot provide currently.

> dm-integrity provides a lighter weight read-write block level integrity
> protection for file systems not requiring full disk encryption, but
> which do require writability.

Obvious question: can be dm-verity extended to provide read-write integrity?

I would prefer to use standard mode like GCM to provide both encryption and
integrity protection than inventing something new.

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: [dm-devel] [PATCH v2 2/2] dm: verity support data device offset (Linux 3.4.7)

[Milan Broz]

On 08/09/2012 02:40 AM, Wesley Miaw wrote:
> On Aug 8, 2012, at 1:56 PM, Milan Broz wrote:
> 
>> On 08/08/2012 10:46 PM, Wesley Miaw wrote:
>> 
>>> I did modify veritysetup on my own so the format and verify
>>> commands will work with regular files on disk instead of having
>>> to mount through loop devices.
>> 
>> Which veritysetup? In upstream (cryptsetup repository) it allocates
>> loop automatically. (And for userspace verification it doesn't need
>> loop at all.)
>> 
>> Anyway, please send a patch for userspace as well then ;-)
> 
> This isn't as polished because I pretty much just added support to do
> what I needed. I'm not sure if the LKML is the right place to post,
> so let me know if I should send this somewhere else.

This is libcryptsetup userspace so better list for this is dmcrypt
mailing list (and/or cc me, I will handle these changes anyway).

The allocated crypto "file" context cannot be later used for some kind
of operations. I do not like this approach musch... 
You cannot use file argument for dm-target directly, so your patch
is useful only for your use case but not for anything else.

Anyway, I am sure there is better way how to solve it I just need
to understand what the problem is. What's wrong if code allocates
loop devices (if argument is file)?
Performance? Loop not available? Need root access?

Please explain what's the problem first.
(btw that patch is mangled by a mailer but not a problem now).

> Your previous email implied that veritysetup would need a way to
> determine if the data offset option is supported; I did not modify
> veritysetup to support this idea as I didn't need it.

Once kernel get this option (if you convince upstream:) then I will
the option to userspace. We have to handle many dm-crypt extensions
so not a big problem for dm-verity as well.

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: [dm-devel] [PATCH] dm: verity support data device offset (Linux 3.4.7)

[Milan Broz]

On 08/08/2012 10:46 PM, Wesley Miaw wrote:
> On Aug 8, 2012, at 1:31 PM, Milan Broz wrote:

> I did modify veritysetup on my own so the format and verify commands will 
> work with regular files on disk instead of having to mount through loop 
> devices.

Which veritysetup? In upstream (cryptsetup repository) it allocates loop 
automatically.
(And for userspace verification it doesn't need loop at all.)

Anyway, please send a patch for userspace as well then ;-)

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: [dm-devel] [PATCH] dm: verity support data device offset (Linux 3.4.7)

[Milan Broz]

On 08/08/2012 08:46 PM, Mikulas Patocka wrote:

> The problem with the patch is that it changes interface to the userspace 
> tool. The userspace tool veritysetup already exists in recent cryptsetup 
> package, so we can't change the interface - you should change the patch so 
> that the starting data block is the last argument and the argument is 
> optional - so that it is compatible with the existing userspace too.

yes. Please never change interface without at least increasing target version.

I have to add userspace support as well to veritysetup and we need a way
how to detect that option is supported by running kernel.

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: 2.6.25-rc[1,2]: failed to setup dm-crypt key mapping

[Milan Broz]

Herbert Xu wrote:
> On Wed, Feb 20, 2008 at 12:23:21PM +0100, Milan Broz wrote:
>> It seems that some module dependency was lost, 
>> dm-crypt with async crypto depends now on crypto_blkcipher module
>> for this configuration.
>>
>> Herbert, any following change required for dm-crypt or it is only
>> crypto subsystem issue?
>> (With old kernel/dm-crypt it loads "blkcipher" not "crypto_blkcipher",
>> maybe some initramdisk change required too...)
> 
> Right blkcipher was renamed to crypto_blkcipher due to the merge
> of blkcipher/ablkcipher.  If this is causing a problem with the
> initramfs then we need to fix the initramfs tools to look at the
> actual dependencies rather than hard-coding it.

I just tested one affected configuration and problem was in missing
"chainiv.ko" module on ramdisk.

Milan
--
[EMAIL PROTECTED]

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

Re: 2.6.25-rc[1,2]: failed to setup dm-crypt key mapping

[Milan Broz]

Michael S. Tsirkin wrote:
> I am booting a thinkpad T60p off of an encrypted disk (with crypto
> modules in initramfs).
> This works fine in 2.6.24, but in both 2.6.25-rc1 and 2.6.25-rc2
> won't boot with the following messages on the console
> (copied by hand, sorry about typos):
> device-mapper: table: 254:0:crypt
> Error allocating crypto tfm
> Failed to setup dm_crypt key mapping.
> check kernel for support for the aes-cbc-essiv:sha256
> cipher spec and verify that /dev/sda6 contains at least 133 sectors.
> Failed to read from storage.
> 
> git bisec points to the following commit:
> 
> commit 3a7f6c990ad04e6f576a159876c602d14d6f7fef
> dm crypt: use async crypto
> 
> Reverting commit 3a7f6c990ad04e6f576a159876c602d14d6f7fef
> on top of 2.6.25-rc2 results in a bootable configuration for me.
> 
> .config attached.

It seems that some module dependency was lost, 
dm-crypt with async crypto depends now on crypto_blkcipher module
for this configuration.

Herbert, any following change required for dm-crypt or it is only
crypto subsystem issue?
(With old kernel/dm-crypt it loads "blkcipher" not "crypto_blkcipher",
maybe some initramdisk change required too...)

Milan
--
[EMAIL PROTECTED]

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

Re: data corruption with dmcrypt/LUKS

[Milan Broz]

Christoph Anton Mitterer wrote:
> On Sun, 2008-02-03 at 23:06 +0100, Milan Broz wrote:
...
>>> 2) The second bug happens only rarely and leads to a panic.
>>> Unfortunately it's difficult to reproduce, but it always happened when I
>>> mkfs.ext3 on the /dev/mapper/sda2.
>>> There's a stack-trace printed which clearly involves some dmcrypt
>>> lines...
>> But no stack trace attached here... please attach it.
> Unfortunately I don't have one,... nothing was written to the logs and I
> forgot to write it up :-/
> 
> 
> 
>> It can be known bug which was fixed in stable version some time ago
>> see http://lkml.org/lkml/2007/7/20/211
> Uhm but that patch should be part of 2.6.24, shouldn't it?

Yes, so if you hit this with 2.6.24 too is very important to sent OOps
log to identify problem (or link to screen snapshot, digital camera
snapshot or so).

...
> btw: What's about the dmcrypt mailing list,.. I've tried to subscribe
> but no answers, and I get not posts (not even my owns).

This is another story...
I hope that someone with admin rights will fix it (Christophe ?)

Milan
--
[EMAIL PROTECTED]

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

Re: data corruption with dmcrypt/LUKS

[Milan Broz]

Christoph Anton Mitterer wrote:
>  ok but this is just, because those files are still cached in RAM>
> 
> 
> 
> Here's the first problem:
> 1) When I now diff the two versions again (the unencrypted and the one
> from the encrypted partition) I get differences...
> I'm quite sure that this is not due to damaged RAM or harddisk (checked
> several times with memtest and badblocks) and the corruption is always
> the same, although not fully reproducibly.
> The filesystem tree itself seems to be the same on both discs (but I'm
> not sure if the permissions and owners are copied correctly), but there
> are differences in some (though not all) files.
> The difference is always the same, that for one or more bytes of the
> affected files, the hexcode is reduced by 0x10
> That is:
> If the file contains a byte "T" (0x74) on the unencrypted partition it
> will have a "D" (0x64) on the encrypted.

Hi,
Are you sure, that your USB-stick is not faulty ?
Could you reproduce it with different piece of hw ?
(Several strange reports for dm-crypt over USB were identified to be 
USB hw faults.)

> 2) The second bug happens only rarely and leads to a panic.
> Unfortunately it's difficult to reproduce, but it always happened when I
> mkfs.ext3 on the /dev/mapper/sda2.
> There's a stack-trace printed which clearly involves some dmcrypt
> lines...

But no stack trace attached here... please attach it.

It can be known bug which was fixed in stable version some time ago
see http://lkml.org/lkml/2007/7/20/211

> btw: are there any other currently known bugs in dmcrypt? Or is it
> considered as "production stable"?

No known bugs causing data corruption, no such reports so far
for stable kernel.

Milan
--
[EMAIL PROTECTED]

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

Re: 2.6.24-rc2-mm1: kcryptd vs lockdep

[Milan Broz]

Torsten Kaiser wrote:
> On Nov 19, 2007 10:00 PM, Milan Broz <[EMAIL PROTECTED]> wrote:
>> Torsten Kaiser wrote:
>>> Anything I could try, apart from more boots with slub_debug=F?
> 
> One time it triggered with slub_debug=F, but no additional output.
> With slub_debug=FP I have not seen it again, so I can't say if that
> would yield more info.
> 
>> Please could you try which patch from the dm-crypt series cause this ?
>> (agk-dm-dm-crypt* names.)
>>
>> I suspect agk-dm-dm-crypt-move-bio-submission-to-thread.patch because
>> there is one work struct used subsequently in two threads...
>> (io thread already started while crypt thread is processing lockdep_map
>> after calling f(work)...)
> 
> After reverting only
> agk-dm-dm-crypt-move-bio-submission-to-thread.patch I also have not
> seen the 'held lock freed' message again.

Ok, then I have question: Is the following pseudocode correct
(and problem is in lock validation which checks something
already initialized for another queue) or reusing work_struct
is not permitted from inside called work function ?

(Note comment in code "It is permissible to free the struct
work_struct from inside the function that is called from it".)

struct work_struct work;
struct workqueue_struct *a, *b;

do_b(*work) 
{
/* do something else */
}

do_a(*work)
{
/* do something */
INIT_WORK(&work, do_b);
queue_work(b, &work);
}


INIT_WORK(&work, do_a);
queue_work(a, &work);

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

Re: 2.6.24-rc2-mm1: kcryptd vs lockdep

[Milan Broz]

Torsten Kaiser wrote:
> On Nov 19, 2007 8:56 AM, Ingo Molnar <[EMAIL PROTECTED]> wrote:
>> * Torsten Kaiser <[EMAIL PROTECTED]> wrote:
...
> Above this acquire/release sequence is the following comment:
> #ifdef CONFIG_LOCKDEP
> /*
>  * It is permissible to free the struct work_struct
>  * from inside the function that is called from it,
>  * this we need to take into account for lockdep too.
>  * To avoid bogus "held lock freed" warnings as well
>  * as problems when looking into work->lockdep_map,
>  * make a copy and use that here.
>  */
> struct lockdep_map lockdep_map = work->lockdep_map;
> #endif
> 
> Did something trigger this anyway?
> 
> Anything I could try, apart from more boots with slub_debug=F?

Please could you try which patch from the dm-crypt series cause this ?
(agk-dm-dm-crypt* names.)

I suspect agk-dm-dm-crypt-move-bio-submission-to-thread.patch because
there is one work struct used subsequently in two threads...
(io thread already started while crypt thread is processing lockdep_map
after calling f(work)...)

(btw these patches prepare dm-crypt for next patchset introducing 
async cryptoapi, so there should be no functional changes yet.)

Milan
--
[EMAIL PROTECTED]


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