Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys

[David Gstir]

Hi!

> On 25.03.2021, at 06:26, Sumit Garg  wrote:
> 
> On Wed, 24 Mar 2021 at 19:37, Ahmad Fatoum  wrote:
>> 
>> Hello Sumit,
>> 
>> On 24.03.21 11:47, Sumit Garg wrote:
>>> On Wed, 24 Mar 2021 at 14:56, Ahmad Fatoum  wrote:
 
 Hello Mimi,
 
 On 23.03.21 19:07, Mimi Zohar wrote:
> On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote:
>> On 21.03.21 21:48, Horia Geantă wrote:
>>> caam has random number generation capabilities, so it's worth using that
>>> by implementing .get_random.
>> 
>> If the CAAM HWRNG is already seeding the kernel RNG, why not use the 
>> kernel's?
>> 
>> Makes for less code duplication IMO.
> 
> Using kernel RNG, in general, for trusted keys has been discussed
> before.   Please refer to Dave Safford's detailed explanation for not
> using it [1].
 
 The argument seems to boil down to:
 
 - TPM RNG are known to be of good quality
 - Trusted keys always used it so far
 
 Both are fine by me for TPMs, but the CAAM backend is new code and neither 
 point
 really applies.
 
 get_random_bytes_wait is already used for generating key material 
 elsewhere.
 Why shouldn't new trusted key backends be able to do the same thing?
 
>>> 
>>> Please refer to documented trusted keys behaviour here [1]. New
>>> trusted key backends should align to this behaviour and in your case
>>> CAAM offers HWRNG so we should be better using that.
>> 
>> Why is it better?
>> 
>> Can you explain what benefit a CAAM user would have if the trusted key
>> randomness comes directly out of the CAAM instead of indirectly from
>> the kernel entropy pool that is seeded by it?
> 
> IMO, user trust in case of trusted keys comes from trusted keys
> backend which is CAAM here. If a user doesn't trust that CAAM would
> act as a reliable source for RNG then CAAM shouldn't be used as a
> trust source in the first place.
> 
> And I think building user's trust for kernel RNG implementation with
> multiple entropy contributions is pretty difficult when compared with
> CAAM HWRNG implementation.

Generally speaking, I’d say trusting the CAAM RNG and trusting in it’s
other features are two separate things. However, reading through the CAAM
key blob spec I’ve got here, CAAM key blob keys (the keys that secure a blob’s
content) are generated using its internal RNG. So I’d save if the CAAM RNG
is insecure, so are generated key blobs. Maybe somebody with more insight
into the CAAM internals can verify that, but I don’t see any point in using
the kernel’s RNG as long as we let CAAM generate the key blob keys for us.

Cheers,
dave

Re: [PATCH v2 1/2] security/keys/secure_key: Adds the secure key support based on CAAM.

[David Gstir]

Hi!

> On 03.08.2018, at 20:28, Mimi Zohar  wrote:
> 
> If they have symmetric key support, there would be no need for
> the
> symmetric key ever to leave the device in the clear. The device
> would unseal/decrypt data, such as an encrypted key.
> 
> The "symmetric" key type would be a generic interface for
> different
> devices.
 
 It's possible, but it would only work for a non-bulk use case; do
 we
 have one of those?
>>> 
>>> "trusted" keys are currently being used to decrypt other keys (eg.
>>> encrypted, ecryptfs, ...).
>> 
>> Yes, but that's just double encryption: it serves no real security
>> purpose because at the end of the chain, the symmetric key is released
>> into kernel memory to use in software crypto.  Unless you're using a
>> high speed (and high cost) crypto accelerator with HSA, this will
>> always be the case for bulk crypto.
>> 
>> The other slight problem with this chain of crypto is that I can impose
>> conditions on the key release from the TPM (well TPM2, since TPM1.2 has
>> a very weak policy engine) but if we use intermediate steps, those
>> conditions might not be preserved, so I think the ideal would be a
>> trusted key being released directly to LUKS or ecryptfs because the TPM
>> can then verify the policy at actual unseal time.  I get that for the
>> ecryptfs case you might want one key per file for sharding and sharing,
>> and that's more like a bulk case because the TPM isn't going to keep up
>> with the number of unseal operations required.
> 
> Agreed.  Yet there are a couple of reasons for having this sort of
> indirection. By having the "encrypted" key encrypted/decrypted by a
> "trusted" key, the "trusted" key could be updated without impacting
> the "encrypted" key.  This could be used, for example, for key
> migration.  Another reason would be to define a single "trusted" key
> sealed to a set of PCRs, which could be used to encrypt/decrypt
> multiple symmetric keys.
> 
> Nothing is preventing these subsystems from directly using a "trusted"
> key.
> 
> This discussion is the result of Udit Agarwal's posting a "secure" key
> patch. Before defining a new key type, whether it is called "secure"
> or "symmetric", we need to understand how the this new key type is
> going to be used.  Will it have a similar ability to impose conditions
> on the key release as the TPM?  Will it have symmetric key support, so
> that the symmetric key never needs to be released from the HW?


I'm looking into mainline support for CAAM-protected keys. I currently have
hacky patches that duplicate trusted keys, and use CAAM instead of TPM to
seal/unseal keys and make them available to other kernel features like fscrypt.
This patch by Udit Agarwal looks like an interesting base for my code.
However, AFAICT there has been no progress for some time now. 

Is anybody still working on this? If not, I'm happy to help out! :)

Regarding the new key type:
In addition to unsealing a key/data into kernel memory, CAAM also supports
unsealing a symmetric key directly into a key register of CAAM's crypto
acceleration engine. This is something I'd like to have, but it will surely
require changes the the CAAM crypto driver as well. Jan Lübbe already mentioned
he is interested in this too: 
https://www.spinics.net/lists/keyrings/msg03919.html

AFAIK, CAAM does not have fine-grained key release restrictions like TPM.
IMHO such a new key type should provide a generic interface with backends for
CAAM, TPM and possibly others to seal/unseal arbitrary data. As for supporting
keys that only reside in the HW, I'm not yet sure what the best approach would 
be.
It should probably tie into the crypto API to be usable throughout the kernel...

Thanks!
David

Re: [PATCH v2 1/2] security/keys/secure_key: Adds the secure key support based on CAAM.

[David Gstir]

Hi!

> On 03.08.2018, at 20:28, Mimi Zohar  wrote:
> 
> If they have symmetric key support, there would be no need for
> the
> symmetric key ever to leave the device in the clear. The device
> would unseal/decrypt data, such as an encrypted key.
> 
> The "symmetric" key type would be a generic interface for
> different
> devices.
 
 It's possible, but it would only work for a non-bulk use case; do
 we
 have one of those?
>>> 
>>> "trusted" keys are currently being used to decrypt other keys (eg.
>>> encrypted, ecryptfs, ...).
>> 
>> Yes, but that's just double encryption: it serves no real security
>> purpose because at the end of the chain, the symmetric key is released
>> into kernel memory to use in software crypto.  Unless you're using a
>> high speed (and high cost) crypto accelerator with HSA, this will
>> always be the case for bulk crypto.
>> 
>> The other slight problem with this chain of crypto is that I can impose
>> conditions on the key release from the TPM (well TPM2, since TPM1.2 has
>> a very weak policy engine) but if we use intermediate steps, those
>> conditions might not be preserved, so I think the ideal would be a
>> trusted key being released directly to LUKS or ecryptfs because the TPM
>> can then verify the policy at actual unseal time.  I get that for the
>> ecryptfs case you might want one key per file for sharding and sharing,
>> and that's more like a bulk case because the TPM isn't going to keep up
>> with the number of unseal operations required.
> 
> Agreed.  Yet there are a couple of reasons for having this sort of
> indirection. By having the "encrypted" key encrypted/decrypted by a
> "trusted" key, the "trusted" key could be updated without impacting
> the "encrypted" key.  This could be used, for example, for key
> migration.  Another reason would be to define a single "trusted" key
> sealed to a set of PCRs, which could be used to encrypt/decrypt
> multiple symmetric keys.
> 
> Nothing is preventing these subsystems from directly using a "trusted"
> key.
> 
> This discussion is the result of Udit Agarwal's posting a "secure" key
> patch. Before defining a new key type, whether it is called "secure"
> or "symmetric", we need to understand how the this new key type is
> going to be used.  Will it have a similar ability to impose conditions
> on the key release as the TPM?  Will it have symmetric key support, so
> that the symmetric key never needs to be released from the HW?


I'm looking into mainline support for CAAM-protected keys. I currently have
hacky patches that duplicate trusted keys, and use CAAM instead of TPM to
seal/unseal keys and make them available to other kernel features like fscrypt.
This patch by Udit Agarwal looks like an interesting base for my code.
However, AFAICT there has been no progress for some time now. 

Is anybody still working on this? If not, I'm happy to help out! :)

Regarding the new key type:
In addition to unsealing a key/data into kernel memory, CAAM also supports
unsealing a symmetric key directly into a key register of CAAM's crypto
acceleration engine. This is something I'd like to have, but it will surely
require changes the the CAAM crypto driver as well. Jan Lübbe already mentioned
he is interested in this too: 
https://www.spinics.net/lists/keyrings/msg03919.html

AFAIK, CAAM does not have fine-grained key release restrictions like TPM.
IMHO such a new key type should provide a generic interface with backends for
CAAM, TPM and possibly others to seal/unseal arbitrary data. As for supporting
keys that only reside in the HW, I'm not yet sure what the best approach would 
be.
It should probably tie into the crypto API to be usable throughout the kernel...

Thanks!
David

Re: [RFC] UBIFS authentication

[David Gstir]

Hi Sascha,

> On 10.04.2018, at 09:02, Sascha Hauer <s.ha...@pengutronix.de> wrote:
> 
> On Mon, Apr 09, 2018 at 05:23:05PM +0200, David Gstir wrote:
>> Hi Sascha,
>> 
>>> On 09.04.2018, at 11:59, Sascha Hauer <s.ha...@pengutronix.de> wrote:
>>> 
>>> Hi David,
>>> 
>>> On Wed, Jan 17, 2018 at 04:19:14PM +0100, David Gstir wrote:
>>>> Hi everybody!
>>>> 
>>>> ### Index Authentication
>>>> 
>>>> Through UBIFS' concept of a wandering tree, it already takes care of only
>>>> updating and persisting changed parts from leaf node up to the root node
>>>> of the full B+ tree. This enables us to augment the index nodes of the tree
>>>> with a hash over each node's child nodes. As a result, the index basically 
>>>> also
>>>> a Merkle tree. Since the leaf nodes of the index contain the actual 
>>>> filesystem
>>>> data, the hashes of their parent index nodes thus cover all the file 
>>>> contents
>>>> and file metadata. When a file changes, the UBIFS index is updated 
>>>> accordingly
>>>> from the leaf nodes up to the root node including the master node. This 
>>>> process
>>>> can be hooked to recompute the hash only for each changed node at the same 
>>>> time.
>>>> Whenever a file is read, UBIFS can verify the hashes from each leaf node 
>>>> up to
>>>> the root node to ensure the node's integrity.
>>>> 
>>>> To ensure the authenticity of the whole index, the UBIFS master node 
>>>> stores a
>>>> keyed hash (HMAC) over its own contents (which includes the location of 
>>>> the root
>>>> node) and the root node of the index tree. As mentioned above, the master 
>>>> node
>>>> is always written to the flash whenever the index is persisted (ie. on 
>>>> index
>>>> commit).
>>>> 
>>>> Using this approach only UBIFS index nodes and the master node are changed 
>>>> to
>>>> include a hash. All other types of nodes will remain unchanged. This 
>>>> reduces
>>>> the storage overhead which is precious for users of UBIFS (ie. embedded
>>>> devices).
>>>> 
>>>> 
>>>>   HMAC Master Node
>>>>|
>>>> ' ' ' ' ' ' ' ' ' ' ' ' ' '|' '
>>>> ' +---+o  '
>>>> ' |  Master Node  |   '
>>>> ' +---+   '   Hash Index Node 
>>>> #1
>>>> ' |   '|
>>>>  . . . . . . . .'. . . . . .  v . . . . . . . . . . . . . . . .|. . . 
>>>> . .
>>>>  .  '  +---+  'o   
>>>>  .
>>>>  .  '  | Index Node #1 |  '
>>>>  .
>>>>  .  '  +---+  '
>>>>  .
>>>>  .  '|...   |  (fanout: 8) 
>>>>  .
>>>>  .  ' ' ' '  | ' ' ' '  | ' ' ' ' '
>>>>  .
>>>>  .   +---+  +--+   
>>>>  .
>>>>  .   | |   
>>>>  .
>>>>  . ' ' ' ' ' v ' ' ' ' '  ' ' ' ' ' '  v  ' ' ' ' ' ' ' ' ' ' '
>>>>  .
>>>>  . ' +---+ '  ' +---+ '
>>>>  .
>>>>  . ' | Index Node #2 | '  ' | Index Node #3 | '
>>>>  .
>>>>  . ' +---+ '  ' +---+ '
>>>>  .
>>>>  . '   |   ... '  '|   ...   |'
>>>>  .
>>>>  . . . ' . v . . . . . . . '. ' . . .  v . . . . v . . . . . . . .' . 
>>>> . .
>>>>' +---+ '  '+--+  +---+'
>>>>' | Data Node | '  '| INO Node |  | DENT Node |'
>>>>' +---+ '  '+--+  +---+'
>>>>'  o   

Re: [RFC] UBIFS authentication

[David Gstir]

Hi Sascha,

> On 10.04.2018, at 09:02, Sascha Hauer  wrote:
> 
> On Mon, Apr 09, 2018 at 05:23:05PM +0200, David Gstir wrote:
>> Hi Sascha,
>> 
>>> On 09.04.2018, at 11:59, Sascha Hauer  wrote:
>>> 
>>> Hi David,
>>> 
>>> On Wed, Jan 17, 2018 at 04:19:14PM +0100, David Gstir wrote:
>>>> Hi everybody!
>>>> 
>>>> ### Index Authentication
>>>> 
>>>> Through UBIFS' concept of a wandering tree, it already takes care of only
>>>> updating and persisting changed parts from leaf node up to the root node
>>>> of the full B+ tree. This enables us to augment the index nodes of the tree
>>>> with a hash over each node's child nodes. As a result, the index basically 
>>>> also
>>>> a Merkle tree. Since the leaf nodes of the index contain the actual 
>>>> filesystem
>>>> data, the hashes of their parent index nodes thus cover all the file 
>>>> contents
>>>> and file metadata. When a file changes, the UBIFS index is updated 
>>>> accordingly
>>>> from the leaf nodes up to the root node including the master node. This 
>>>> process
>>>> can be hooked to recompute the hash only for each changed node at the same 
>>>> time.
>>>> Whenever a file is read, UBIFS can verify the hashes from each leaf node 
>>>> up to
>>>> the root node to ensure the node's integrity.
>>>> 
>>>> To ensure the authenticity of the whole index, the UBIFS master node 
>>>> stores a
>>>> keyed hash (HMAC) over its own contents (which includes the location of 
>>>> the root
>>>> node) and the root node of the index tree. As mentioned above, the master 
>>>> node
>>>> is always written to the flash whenever the index is persisted (ie. on 
>>>> index
>>>> commit).
>>>> 
>>>> Using this approach only UBIFS index nodes and the master node are changed 
>>>> to
>>>> include a hash. All other types of nodes will remain unchanged. This 
>>>> reduces
>>>> the storage overhead which is precious for users of UBIFS (ie. embedded
>>>> devices).
>>>> 
>>>> 
>>>>   HMAC Master Node
>>>>|
>>>> ' ' ' ' ' ' ' ' ' ' ' ' ' '|' '
>>>> ' +---+o  '
>>>> ' |  Master Node  |   '
>>>> ' +---+   '   Hash Index Node 
>>>> #1
>>>> ' |   '|
>>>>  . . . . . . . .'. . . . . .  v . . . . . . . . . . . . . . . .|. . . 
>>>> . .
>>>>  .  '  +---+  'o   
>>>>  .
>>>>  .  '  | Index Node #1 |  '
>>>>  .
>>>>  .  '  +---+  '
>>>>  .
>>>>  .  '|...   |  (fanout: 8) 
>>>>  .
>>>>  .  ' ' ' '  | ' ' ' '  | ' ' ' ' '
>>>>  .
>>>>  .   +---+  +--+   
>>>>  .
>>>>  .   | |   
>>>>  .
>>>>  . ' ' ' ' ' v ' ' ' ' '  ' ' ' ' ' '  v  ' ' ' ' ' ' ' ' ' ' '
>>>>  .
>>>>  . ' +---+ '  ' +---+ '
>>>>  .
>>>>  . ' | Index Node #2 | '  ' | Index Node #3 | '
>>>>  .
>>>>  . ' +---+ '  ' +---+ '
>>>>  .
>>>>  . '   |   ... '  '|   ...   |'
>>>>  .
>>>>  . . . ' . v . . . . . . . '. ' . . .  v . . . . v . . . . . . . .' . 
>>>> . .
>>>>' +---+ '  '+--+  +---+'
>>>>' | Data Node | '  '| INO Node |  | DENT Node |'
>>>>' +---+ '  '+--+  +---+'
>>>>'  o'  'o  '
>>>> 

Re: [RFC] UBIFS authentication

[David Gstir]

Hi Sascha,

> On 09.04.2018, at 11:59, Sascha Hauer <s.ha...@pengutronix.de> wrote:
> 
> Hi David,
> 
> On Wed, Jan 17, 2018 at 04:19:14PM +0100, David Gstir wrote:
>> Hi everybody!
>> 
>> ### Index Authentication
>> 
>> Through UBIFS' concept of a wandering tree, it already takes care of only
>> updating and persisting changed parts from leaf node up to the root node
>> of the full B+ tree. This enables us to augment the index nodes of the tree
>> with a hash over each node's child nodes. As a result, the index basically 
>> also
>> a Merkle tree. Since the leaf nodes of the index contain the actual 
>> filesystem
>> data, the hashes of their parent index nodes thus cover all the file contents
>> and file metadata. When a file changes, the UBIFS index is updated 
>> accordingly
>> from the leaf nodes up to the root node including the master node. This 
>> process
>> can be hooked to recompute the hash only for each changed node at the same 
>> time.
>> Whenever a file is read, UBIFS can verify the hashes from each leaf node up 
>> to
>> the root node to ensure the node's integrity.
>> 
>> To ensure the authenticity of the whole index, the UBIFS master node stores a
>> keyed hash (HMAC) over its own contents (which includes the location of the 
>> root
>> node) and the root node of the index tree. As mentioned above, the master 
>> node
>> is always written to the flash whenever the index is persisted (ie. on index
>> commit).
>> 
>> Using this approach only UBIFS index nodes and the master node are changed to
>> include a hash. All other types of nodes will remain unchanged. This reduces
>> the storage overhead which is precious for users of UBIFS (ie. embedded
>> devices).
>> 
>> 
>>HMAC Master Node
>> |
>>  ' ' ' ' ' ' ' ' ' ' ' ' ' '|' '
>>  ' +---+o  '
>>  ' |  Master Node  |   '
>>  ' +---+   '   Hash Index Node #1
>>  ' |   '|
>>   . . . . . . . .'. . . . . .  v . . . . . . . . . . . . . . . .|. . . . 
>> .
>>   .  '  +---+  'o
>> .
>>   .  '  | Index Node #1 |  ' 
>> .
>>   .  '  +---+  ' 
>> .
>>   .  '|...   |  (fanout: 8)  
>> .
>>   .  ' ' ' '  | ' ' ' '  | ' ' ' ' ' 
>> .
>>   .   +---+  +--+
>> .
>>   .   | |
>> .
>>   . ' ' ' ' ' v ' ' ' ' '  ' ' ' ' ' '  v  ' ' ' ' ' ' ' ' ' ' ' 
>> .
>>   . ' +---+ '  ' +---+ ' 
>> .
>>   . ' | Index Node #2 | '  ' | Index Node #3 | ' 
>> .
>>   . ' +---+ '  ' +---+ ' 
>> .
>>   . '   |   ... '  '|   ...   |' 
>> .
>>   . . . ' . v . . . . . . . '. ' . . .  v . . . . v . . . . . . . .' . . 
>> .
>> ' +---+ '  '+--+  +---+'
>> ' | Data Node | '  '| INO Node |  | DENT Node |'
>> ' +---+ '  '+--+  +---+'
>> '  o'  'o  '
>> ' '|' ' ' ' ' ' ' ' '  ' ' ' ' ' ' ' ' ' ' ' ' ' ' ' ' '|' '
>>||
>>  Hash Index Node #2Hash Index Node #3
> 
> When a hash covers an index node and also its children then of course
> this is really space efficient, but this also means that in order to
> read a node we always have to read all of its children. Also adding a
> new leaf node means rehashing all siblings. Is it really worth paying
> this price to save a few bytes for more hashes?
> I would rather suggest to add a hash per child to each index node.

What you propose is a simple tradeoff between the required on-flash storage
space and the amount of read operations needed to verify the index node 
integrity.

To get some numbers here

Re: [RFC] UBIFS authentication

[David Gstir]

Hi Sascha,

> On 09.04.2018, at 11:59, Sascha Hauer  wrote:
> 
> Hi David,
> 
> On Wed, Jan 17, 2018 at 04:19:14PM +0100, David Gstir wrote:
>> Hi everybody!
>> 
>> ### Index Authentication
>> 
>> Through UBIFS' concept of a wandering tree, it already takes care of only
>> updating and persisting changed parts from leaf node up to the root node
>> of the full B+ tree. This enables us to augment the index nodes of the tree
>> with a hash over each node's child nodes. As a result, the index basically 
>> also
>> a Merkle tree. Since the leaf nodes of the index contain the actual 
>> filesystem
>> data, the hashes of their parent index nodes thus cover all the file contents
>> and file metadata. When a file changes, the UBIFS index is updated 
>> accordingly
>> from the leaf nodes up to the root node including the master node. This 
>> process
>> can be hooked to recompute the hash only for each changed node at the same 
>> time.
>> Whenever a file is read, UBIFS can verify the hashes from each leaf node up 
>> to
>> the root node to ensure the node's integrity.
>> 
>> To ensure the authenticity of the whole index, the UBIFS master node stores a
>> keyed hash (HMAC) over its own contents (which includes the location of the 
>> root
>> node) and the root node of the index tree. As mentioned above, the master 
>> node
>> is always written to the flash whenever the index is persisted (ie. on index
>> commit).
>> 
>> Using this approach only UBIFS index nodes and the master node are changed to
>> include a hash. All other types of nodes will remain unchanged. This reduces
>> the storage overhead which is precious for users of UBIFS (ie. embedded
>> devices).
>> 
>> 
>>HMAC Master Node
>> |
>>  ' ' ' ' ' ' ' ' ' ' ' ' ' '|' '
>>  ' +---+o  '
>>  ' |  Master Node  |   '
>>  ' +---+   '   Hash Index Node #1
>>  ' |   '|
>>   . . . . . . . .'. . . . . .  v . . . . . . . . . . . . . . . .|. . . . 
>> .
>>   .  '  +---+  'o
>> .
>>   .  '  | Index Node #1 |  ' 
>> .
>>   .  '  +---+  ' 
>> .
>>   .  '|...   |  (fanout: 8)  
>> .
>>   .  ' ' ' '  | ' ' ' '  | ' ' ' ' ' 
>> .
>>   .   +---+  +--+
>> .
>>   .   | |
>> .
>>   . ' ' ' ' ' v ' ' ' ' '  ' ' ' ' ' '  v  ' ' ' ' ' ' ' ' ' ' ' 
>> .
>>   . ' +---+ '  ' +---+ ' 
>> .
>>   . ' | Index Node #2 | '  ' | Index Node #3 | ' 
>> .
>>   . ' +---+ '  ' +---+ ' 
>> .
>>   . '   |   ... '  '|   ...   |' 
>> .
>>   . . . ' . v . . . . . . . '. ' . . .  v . . . . v . . . . . . . .' . . 
>> .
>> ' +---+ '  '+--+  +---+'
>> ' | Data Node | '  '| INO Node |  | DENT Node |'
>> ' +---+ '  '+--+  +---+'
>> '  o'  'o  '
>> ' '|' ' ' ' ' ' ' ' '  ' ' ' ' ' ' ' ' ' ' ' ' ' ' ' ' '|' '
>>||
>>  Hash Index Node #2Hash Index Node #3
> 
> When a hash covers an index node and also its children then of course
> this is really space efficient, but this also means that in order to
> read a node we always have to read all of its children. Also adding a
> new leaf node means rehashing all siblings. Is it really worth paying
> this price to save a few bytes for more hashes?
> I would rather suggest to add a hash per child to each index node.

What you propose is a simple tradeoff between the required on-flash storage
space and the amount of read operations needed to verify the index node 
integrity.

To get some numbers here: In case of SHA256 and a fan

Re: [RFC] UBIFS authentication

[David Gstir]

Hi!

> On 17.01.2018, at 16:19, David Gstir <da...@sigma-star.at> wrote:
> 
> Hi everybody!
> 
> Richard and I have been working on extending UBIFS' security features and came
> up with the following concept to add full file contents and metadata 
> authentication.
> 
> For block devices like eMMCs dm-crypt and dm-verity/dm-integrity can be used 
> to
> get full data confidentiality and authenticity, but for raw flash or more
> specifically UBIFS, existing options are not ideal:
> 
> One option is to use eCryptfs with some out-of-tree patches that add AEAD 
> cipher
> (AES-GCM) support, but does not look like there was much progress recently 
> [1].
> 
> Another option is to use IMA/EVM as described by Marc Kleine-Budde in his
> ELCE 2016 talk [2], but this just protects the file payload and some 
> attributes
> and not the full filesystem data structures.
> 
> A short overview of existing options is also given here [3].
> 
> Due to the design of UBIFS it is actually a bit easier than on other 
> filesystems
> to authenticate its data structures and ensure consistency of on-flash data.
> 
> I've attached the whitepaper below and also published it here [4].
> 
> Comments are welcome. :)

*ping*

Did anybody get a chance to look at this yet, or is everybody still busy with 
Meltdown and Spectre? ;D

Thanks,
David

Re: [RFC] UBIFS authentication

[David Gstir]

Hi!

> On 17.01.2018, at 16:19, David Gstir  wrote:
> 
> Hi everybody!
> 
> Richard and I have been working on extending UBIFS' security features and came
> up with the following concept to add full file contents and metadata 
> authentication.
> 
> For block devices like eMMCs dm-crypt and dm-verity/dm-integrity can be used 
> to
> get full data confidentiality and authenticity, but for raw flash or more
> specifically UBIFS, existing options are not ideal:
> 
> One option is to use eCryptfs with some out-of-tree patches that add AEAD 
> cipher
> (AES-GCM) support, but does not look like there was much progress recently 
> [1].
> 
> Another option is to use IMA/EVM as described by Marc Kleine-Budde in his
> ELCE 2016 talk [2], but this just protects the file payload and some 
> attributes
> and not the full filesystem data structures.
> 
> A short overview of existing options is also given here [3].
> 
> Due to the design of UBIFS it is actually a bit easier than on other 
> filesystems
> to authenticate its data structures and ensure consistency of on-flash data.
> 
> I've attached the whitepaper below and also published it here [4].
> 
> Comments are welcome. :)

*ping*

Did anybody get a chance to look at this yet, or is everybody still busy with 
Meltdown and Spectre? ;D

Thanks,
David

[RFC] UBIFS authentication

[David Gstir]

Hi everybody!

Richard and I have been working on extending UBIFS' security features and came
up with the following concept to add full file contents and metadata 
authentication.

For block devices like eMMCs dm-crypt and dm-verity/dm-integrity can be used to
get full data confidentiality and authenticity, but for raw flash or more
specifically UBIFS, existing options are not ideal:

One option is to use eCryptfs with some out-of-tree patches that add AEAD cipher
(AES-GCM) support, but does not look like there was much progress recently [1].

Another option is to use IMA/EVM as described by Marc Kleine-Budde in his
ELCE 2016 talk [2], but this just protects the file payload and some attributes
and not the full filesystem data structures.

A short overview of existing options is also given here [3].

Due to the design of UBIFS it is actually a bit easier than on other filesystems
to authenticate its data structures and ensure consistency of on-flash data.

I've attached the whitepaper below and also published it here [4].

Comments are welcome. :)

Thanks,
david

[1] https://bugs.launchpad.net/ecryptfs/+bug/1176448
[2] https://elinux.org/images/f/f8/Verified_Boot.pdf
[3] https://www.timesys.com/security/secure-boot-encrypted-data-storage/
[4] https://github.com/sigma-star/ubifs-authentication



---

% UBIFS Authentication
% sigma star gmbh
% 2018

# Introduction

UBIFS utilizes the fscrypt framework to provide confidentiality for file
contents and file names. This prevents attacks where an attacker is able to
read contents of the filesystem on a single point in time. A classic example
is a lost smartphone where the attacker is unable to read personal data stored
on the device without the filesystem decryption key.

At the current state, UBIFS encryption however does not prevent attacks where
the attacker is able to modify the filesystem contents and the user uses the
device afterwards. In such a scenario an attacker can modify filesystem
contents arbitrarily without the user noticing. One example is to modify a
binary to perform a malicious action when executed [DMC-CBC-ATTACK]. Since
most of the filesystem metadata of UBIFS is stored in plain, this makes it
fairly easy to swap files and replace their contents.

Other full disk encryption systems like dm-crypt cover all filesystem metadata,
which makes such kinds of attacks more complicated, but not impossible.
Especially, if the attacker is given access to the device multiple points in
time. For dm-crypt and other filesystems that build upon the Linux block IO
layer, the dm-integrity or dm-verity subsystems [DM-INTEGRITY, DM-VERITY] 
can be used to get full data authentication at the block layer.
These can also be combined with dm-crypt [CRYPTSETUP2].

This document describes an approach to get file contents _and_ full metadata
authentication for UBIFS. Since UBIFS uses fscrypt for file contents and file
name encryption, the authentication system could be tied into fscrypt such that
existing features like key derivation can be utilized. It should however also
be possible to use UBIFS authentication without using encryption.


## MTD, UBI & UBIFS

On Linux, the MTD (Memory Technology Devices) subsystem provides a uniform
interface to access raw flash devices. One of the more prominent subsystems that
work on top of MTD is UBI (Unsorted Block Images). It provides volume management
for flash devices and is thus somewhat similar to LVM for block devices. In
addition, it deals with flash-specific wear-leveling and transparent I/O error
handling. UBI offers logical erase blocks (LEBs) to the layers on top of it
and maps them transparently to physical erase blocks (PEBs) on the flash.

UBIFS is a filesystem for raw flash which operates on top of UBI. Thus, wear
leveling and some flash specifics are left to UBI, while UBIFS focuses on
scalability, performance and recoverability.



++ +***+ +---+ +-+
|| * UBIFS * | UBI-BLOCK | | ... |
| JFFS/JFFS2 | +***+ +---+ +-+
|| +-+ +---+ +-+
|| |  UBI| | MTD-BLOCK | | ... |
++ +-+ +---+ +-+
+--+
|  MEMORY TECHNOLOGY DEVICES (MTD) |
+--+
+-+ +--+ +-+
| NAND DRIVERS| |NOR DRIVERS   | | ... |
+-+ +--+ +-+

Figure 1: Linux kernel subsystems for dealing with raw flash



Internally, UBIFS maintains multiple data structures which are persisted on
the flash:

- *Index*: an on-flash B+ tree where the leaf nodes contain 

[RFC] UBIFS authentication

[David Gstir]

Hi everybody!

Richard and I have been working on extending UBIFS' security features and came
up with the following concept to add full file contents and metadata 
authentication.

For block devices like eMMCs dm-crypt and dm-verity/dm-integrity can be used to
get full data confidentiality and authenticity, but for raw flash or more
specifically UBIFS, existing options are not ideal:

One option is to use eCryptfs with some out-of-tree patches that add AEAD cipher
(AES-GCM) support, but does not look like there was much progress recently [1].

Another option is to use IMA/EVM as described by Marc Kleine-Budde in his
ELCE 2016 talk [2], but this just protects the file payload and some attributes
and not the full filesystem data structures.

A short overview of existing options is also given here [3].

Due to the design of UBIFS it is actually a bit easier than on other filesystems
to authenticate its data structures and ensure consistency of on-flash data.

I've attached the whitepaper below and also published it here [4].

Comments are welcome. :)

Thanks,
david

[1] https://bugs.launchpad.net/ecryptfs/+bug/1176448
[2] https://elinux.org/images/f/f8/Verified_Boot.pdf
[3] https://www.timesys.com/security/secure-boot-encrypted-data-storage/
[4] https://github.com/sigma-star/ubifs-authentication



---

% UBIFS Authentication
% sigma star gmbh
% 2018

# Introduction

UBIFS utilizes the fscrypt framework to provide confidentiality for file
contents and file names. This prevents attacks where an attacker is able to
read contents of the filesystem on a single point in time. A classic example
is a lost smartphone where the attacker is unable to read personal data stored
on the device without the filesystem decryption key.

At the current state, UBIFS encryption however does not prevent attacks where
the attacker is able to modify the filesystem contents and the user uses the
device afterwards. In such a scenario an attacker can modify filesystem
contents arbitrarily without the user noticing. One example is to modify a
binary to perform a malicious action when executed [DMC-CBC-ATTACK]. Since
most of the filesystem metadata of UBIFS is stored in plain, this makes it
fairly easy to swap files and replace their contents.

Other full disk encryption systems like dm-crypt cover all filesystem metadata,
which makes such kinds of attacks more complicated, but not impossible.
Especially, if the attacker is given access to the device multiple points in
time. For dm-crypt and other filesystems that build upon the Linux block IO
layer, the dm-integrity or dm-verity subsystems [DM-INTEGRITY, DM-VERITY] 
can be used to get full data authentication at the block layer.
These can also be combined with dm-crypt [CRYPTSETUP2].

This document describes an approach to get file contents _and_ full metadata
authentication for UBIFS. Since UBIFS uses fscrypt for file contents and file
name encryption, the authentication system could be tied into fscrypt such that
existing features like key derivation can be utilized. It should however also
be possible to use UBIFS authentication without using encryption.


## MTD, UBI & UBIFS

On Linux, the MTD (Memory Technology Devices) subsystem provides a uniform
interface to access raw flash devices. One of the more prominent subsystems that
work on top of MTD is UBI (Unsorted Block Images). It provides volume management
for flash devices and is thus somewhat similar to LVM for block devices. In
addition, it deals with flash-specific wear-leveling and transparent I/O error
handling. UBI offers logical erase blocks (LEBs) to the layers on top of it
and maps them transparently to physical erase blocks (PEBs) on the flash.

UBIFS is a filesystem for raw flash which operates on top of UBI. Thus, wear
leveling and some flash specifics are left to UBI, while UBIFS focuses on
scalability, performance and recoverability.



++ +***+ +---+ +-+
|| * UBIFS * | UBI-BLOCK | | ... |
| JFFS/JFFS2 | +***+ +---+ +-+
|| +-+ +---+ +-+
|| |  UBI| | MTD-BLOCK | | ... |
++ +-+ +---+ +-+
+--+
|  MEMORY TECHNOLOGY DEVICES (MTD) |
+--+
+-+ +--+ +-+
| NAND DRIVERS| |NOR DRIVERS   | | ... |
+-+ +--+ +-+

Figure 1: Linux kernel subsystems for dealing with raw flash



Internally, UBIFS maintains multiple data structures which are persisted on
the flash:

- *Index*: an on-flash B+ tree where the leaf nodes contain 

[PATCH] crypto: caam - properly set IV after {en,de}crypt

[David Gstir]

Certain cipher modes like CTS expect the IV (req->info) of
ablkcipher_request (or equivalently req->iv of skcipher_request) to
contain the last ciphertext block when the {en,de}crypt operation is done.
This is currently not the case for the CAAM driver which in turn breaks
e.g. cts(cbc(aes)) when the CAAM driver is enabled.

This patch fixes the CAAM driver to properly set the IV after the
{en,de}crypt operation of ablkcipher finishes.

This issue was revealed by the changes in the SW CTS mode in commit
0605c41cc53ca ("crypto: cts - Convert to skcipher")

Cc: <sta...@vger.kernel.org> # 4.8+
Signed-off-by: David Gstir <da...@sigma-star.at>
---
 drivers/crypto/caam/caamalg.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 398807d1b77e..c45b5bf65254 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -882,10 +882,10 @@ static void ablkcipher_encrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 {
struct ablkcipher_request *req = context;
struct ablkcipher_edesc *edesc;
-#ifdef DEBUG
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
 
+#ifdef DEBUG
dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
 #endif
 
@@ -904,6 +904,14 @@ static void ablkcipher_encrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 #endif
 
ablkcipher_unmap(jrdev, edesc, req);
+
+   /*
+* The crypto API expects us to set the IV (req->info) to the last
+* ciphertext block. This is used e.g. by the CTS mode.
+*/
+   scatterwalk_map_and_copy(req->info, req->dst, req->nbytes - ivsize,
+ivsize, 0);
+
kfree(edesc);
 
ablkcipher_request_complete(req, err);
@@ -914,10 +922,10 @@ static void ablkcipher_decrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 {
struct ablkcipher_request *req = context;
struct ablkcipher_edesc *edesc;
-#ifdef DEBUG
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
 
+#ifdef DEBUG
dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
 #endif
 
@@ -935,6 +943,14 @@ static void ablkcipher_decrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 #endif
 
ablkcipher_unmap(jrdev, edesc, req);
+
+   /*
+* The crypto API expects us to set the IV (req->info) to the last
+* ciphertext block.
+*/
+   scatterwalk_map_and_copy(req->info, req->src, req->nbytes - ivsize,
+ivsize, 0);
+
kfree(edesc);
 
ablkcipher_request_complete(req, err);
-- 
2.12.3

[PATCH] crypto: caam - properly set IV after {en,de}crypt

[David Gstir]

Certain cipher modes like CTS expect the IV (req->info) of
ablkcipher_request (or equivalently req->iv of skcipher_request) to
contain the last ciphertext block when the {en,de}crypt operation is done.
This is currently not the case for the CAAM driver which in turn breaks
e.g. cts(cbc(aes)) when the CAAM driver is enabled.

This patch fixes the CAAM driver to properly set the IV after the
{en,de}crypt operation of ablkcipher finishes.

This issue was revealed by the changes in the SW CTS mode in commit
0605c41cc53ca ("crypto: cts - Convert to skcipher")

Cc:  # 4.8+
Signed-off-by: David Gstir 
---
 drivers/crypto/caam/caamalg.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 398807d1b77e..c45b5bf65254 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -882,10 +882,10 @@ static void ablkcipher_encrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 {
struct ablkcipher_request *req = context;
struct ablkcipher_edesc *edesc;
-#ifdef DEBUG
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
 
+#ifdef DEBUG
dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
 #endif
 
@@ -904,6 +904,14 @@ static void ablkcipher_encrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 #endif
 
ablkcipher_unmap(jrdev, edesc, req);
+
+   /*
+* The crypto API expects us to set the IV (req->info) to the last
+* ciphertext block. This is used e.g. by the CTS mode.
+*/
+   scatterwalk_map_and_copy(req->info, req->dst, req->nbytes - ivsize,
+ivsize, 0);
+
kfree(edesc);
 
ablkcipher_request_complete(req, err);
@@ -914,10 +922,10 @@ static void ablkcipher_decrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 {
struct ablkcipher_request *req = context;
struct ablkcipher_edesc *edesc;
-#ifdef DEBUG
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
 
+#ifdef DEBUG
dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
 #endif
 
@@ -935,6 +943,14 @@ static void ablkcipher_decrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 #endif
 
ablkcipher_unmap(jrdev, edesc, req);
+
+   /*
+* The crypto API expects us to set the IV (req->info) to the last
+* ciphertext block.
+*/
+   scatterwalk_map_and_copy(req->info, req->src, req->nbytes - ivsize,
+ivsize, 0);
+
kfree(edesc);
 
ablkcipher_request_complete(req, err);
-- 
2.12.3

Re: [RFC PATCH 1/2] crypto: caam - properly set IV after {en,de}crypt

[David Gstir]

Horia,

> On 28 Jun 2017, at 10:32, Horia Geantă  wrote:
> 
>>> +   sg_pcopy_to_buffer(req->dst, nents, req->info, ivsize,
>>> +  req->nbytes - ivsize);
>> 
>> scatterwalk_map_and_copy() should be used instead.
>> 
> David, IIUC this is the only change needed in this patch (applies both
> for encryption and decryption, of course).
> Will you formally resubmit?

Thanks for the reminder. I did not get arround it yet.
Will send the new patch within the next few hours.

Thanks,
David

Re: [RFC PATCH 1/2] crypto: caam - properly set IV after {en,de}crypt

[David Gstir]

Horia,

> On 28 Jun 2017, at 10:32, Horia Geantă  wrote:
> 
>>> +   sg_pcopy_to_buffer(req->dst, nents, req->info, ivsize,
>>> +  req->nbytes - ivsize);
>> 
>> scatterwalk_map_and_copy() should be used instead.
>> 
> David, IIUC this is the only change needed in this patch (applies both
> for encryption and decryption, of course).
> Will you formally resubmit?

Thanks for the reminder. I did not get arround it yet.
Will send the new patch within the next few hours.

Thanks,
David

Re: [RFC PATCH 1/2] crypto: caam - properly set IV after {en,de}crypt

[David Gstir]

Herbert,

> On 20 Jun 2017, at 03:28, Herbert Xu  wrote:
> 
> On Mon, Jun 19, 2017 at 10:31:27AM +, Horia Geantă wrote:
>> 
>> IIUC, IV update is required only in case of CBC.
>> Since this callback is used also for CTR, we should avoid the copy:
>> if ((ctx->cdata.algtype & OP_ALG_AAI_MASK) == OP_ALG_AAI_CBC) ...
> 
> No it is needed for CTR too.

So, am I correct in assuming that it is required for all modes including AEAD 
modes like GCM?
In that case I'll include a fix for the CAAM GCM mode too.

Thanks,
David

Re: [RFC PATCH 1/2] crypto: caam - properly set IV after {en,de}crypt

[David Gstir]

Herbert,

> On 20 Jun 2017, at 03:28, Herbert Xu  wrote:
> 
> On Mon, Jun 19, 2017 at 10:31:27AM +, Horia Geantă wrote:
>> 
>> IIUC, IV update is required only in case of CBC.
>> Since this callback is used also for CTR, we should avoid the copy:
>> if ((ctx->cdata.algtype & OP_ALG_AAI_MASK) == OP_ALG_AAI_CBC) ...
> 
> No it is needed for CTR too.

So, am I correct in assuming that it is required for all modes including AEAD 
modes like GCM?
In that case I'll include a fix for the CAAM GCM mode too.

Thanks,
David

[PATCH v5] fscrypt: Add support for AES-128-CBC

[David Gstir]

From: Daniel Walter <dwal...@sigma-star.at>

fscrypt provides facilities to use different encryption algorithms which
are selectable by userspace when setting the encryption policy. Currently,
only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
implemented. This is a clear case of kernel offers the mechanism and
userspace selects a policy. Similar to what dm-crypt and ecryptfs have.

This patch adds support for using AES-128-CBC for file contents and
AES-128-CBC-CTS for file name encryption. To mitigate watermarking
attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
actually slightly less secure than AES-XTS from a security point of view,
there is more widespread hardware support. Using AES-CBC gives us the
acceptable performance while still providing a moderate level of security
for persistent storage.

Especially low-powered embedded devices with crypto accelerators such as
CAAM or CESA often only support AES-CBC. Since using AES-CBC over AES-XTS
is basically thought of a last resort, we use AES-128-CBC over AES-256-CBC
since it has less encryption rounds and yields noticeable better
performance starting from a file size of just a few kB.

Signed-off-by: Daniel Walter <dwal...@sigma-star.at>
[da...@sigma-star.at: addressed review comments]
Signed-off-by: David Gstir <da...@sigma-star.at>
Reviewed-by: Eric Biggers <ebigg...@google.com>
---
 fs/crypto/Kconfig  |   1 +
 fs/crypto/crypto.c |  23 --
 fs/crypto/fscrypt_private.h|   9 ++-
 fs/crypto/keyinfo.c| 173 -
 fs/crypto/policy.c |   8 +-
 include/linux/fscrypt_common.h |  16 ++--
 include/uapi/linux/fs.h|   2 +
 7 files changed, 174 insertions(+), 58 deletions(-)

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index 08b46e6e3995..02b7d91c9231 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -7,6 +7,7 @@ config FS_ENCRYPTION
select CRYPTO_XTS
select CRYPTO_CTS
select CRYPTO_CTR
+   select CRYPTO_SHA256
select KEYS
help
  Enable encryption of files and directories.  This
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6d6eca394d4d..c7835df7e7b8 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "fscrypt_private.h"
 
 static unsigned int num_prealloc_crypto_pages = 32;
@@ -147,8 +148,8 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 {
struct {
__le64 index;
-   u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
-   } xts_tweak;
+   u8 padding[FS_IV_SIZE - sizeof(__le64)];
+   } iv;
struct skcipher_request *req = NULL;
DECLARE_FS_COMPLETION_RESULT(ecr);
struct scatterlist dst, src;
@@ -158,6 +159,16 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 
BUG_ON(len == 0);
 
+   BUILD_BUG_ON(sizeof(iv) != FS_IV_SIZE);
+   BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
+   iv.index = cpu_to_le64(lblk_num);
+   memset(iv.padding, 0, sizeof(iv.padding));
+
+   if (ci->ci_essiv_tfm != NULL) {
+   crypto_cipher_encrypt_one(ci->ci_essiv_tfm, (u8 *),
+ (u8 *));
+   }
+
req = skcipher_request_alloc(tfm, gfp_flags);
if (!req) {
printk_ratelimited(KERN_ERR
@@ -170,15 +181,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
page_crypt_complete, );
 
-   BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
-   xts_tweak.index = cpu_to_le64(lblk_num);
-   memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
-
sg_init_table(, 1);
sg_set_page(, dest_page, len, offs);
sg_init_table(, 1);
sg_set_page(, src_page, len, offs);
-   skcipher_request_set_crypt(req, , , len, _tweak);
+   skcipher_request_set_crypt(req, , , len, );
if (rw == FS_DECRYPT)
res = crypto_skcipher_decrypt(req);
else
@@ -477,6 +484,8 @@ static void __exit fscrypt_exit(void)
destroy_workqueue(fscrypt_read_workqueue);
kmem_cache_destroy(fscrypt_ctx_cachep);
kmem_cache_destroy(fscrypt_info_cachep);
+
+   fscrypt_essiv_cleanup();
 }
 module_exit(fscrypt_exit);
 
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 1e1f8a361b75..a1d5021c31ef 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -12,10 +12,13 @@
 #define _FSCRYPT_PRIVATE_H
 
 #include 
+#include 
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE  16
+#define FS_IV_SIZE 16
 #define FS_AES_128_ECB_KEY_SIZE16
+#define F

[PATCH v5] fscrypt: Add support for AES-128-CBC

[David Gstir]

From: Daniel Walter 

fscrypt provides facilities to use different encryption algorithms which
are selectable by userspace when setting the encryption policy. Currently,
only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
implemented. This is a clear case of kernel offers the mechanism and
userspace selects a policy. Similar to what dm-crypt and ecryptfs have.

This patch adds support for using AES-128-CBC for file contents and
AES-128-CBC-CTS for file name encryption. To mitigate watermarking
attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
actually slightly less secure than AES-XTS from a security point of view,
there is more widespread hardware support. Using AES-CBC gives us the
acceptable performance while still providing a moderate level of security
for persistent storage.

Especially low-powered embedded devices with crypto accelerators such as
CAAM or CESA often only support AES-CBC. Since using AES-CBC over AES-XTS
is basically thought of a last resort, we use AES-128-CBC over AES-256-CBC
since it has less encryption rounds and yields noticeable better
performance starting from a file size of just a few kB.

Signed-off-by: Daniel Walter 
[da...@sigma-star.at: addressed review comments]
Signed-off-by: David Gstir 
Reviewed-by: Eric Biggers 
---
 fs/crypto/Kconfig  |   1 +
 fs/crypto/crypto.c |  23 --
 fs/crypto/fscrypt_private.h|   9 ++-
 fs/crypto/keyinfo.c| 173 -
 fs/crypto/policy.c |   8 +-
 include/linux/fscrypt_common.h |  16 ++--
 include/uapi/linux/fs.h|   2 +
 7 files changed, 174 insertions(+), 58 deletions(-)

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index 08b46e6e3995..02b7d91c9231 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -7,6 +7,7 @@ config FS_ENCRYPTION
select CRYPTO_XTS
select CRYPTO_CTS
select CRYPTO_CTR
+   select CRYPTO_SHA256
select KEYS
help
  Enable encryption of files and directories.  This
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6d6eca394d4d..c7835df7e7b8 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "fscrypt_private.h"
 
 static unsigned int num_prealloc_crypto_pages = 32;
@@ -147,8 +148,8 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 {
struct {
__le64 index;
-   u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
-   } xts_tweak;
+   u8 padding[FS_IV_SIZE - sizeof(__le64)];
+   } iv;
struct skcipher_request *req = NULL;
DECLARE_FS_COMPLETION_RESULT(ecr);
struct scatterlist dst, src;
@@ -158,6 +159,16 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 
BUG_ON(len == 0);
 
+   BUILD_BUG_ON(sizeof(iv) != FS_IV_SIZE);
+   BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
+   iv.index = cpu_to_le64(lblk_num);
+   memset(iv.padding, 0, sizeof(iv.padding));
+
+   if (ci->ci_essiv_tfm != NULL) {
+   crypto_cipher_encrypt_one(ci->ci_essiv_tfm, (u8 *),
+ (u8 *));
+   }
+
req = skcipher_request_alloc(tfm, gfp_flags);
if (!req) {
printk_ratelimited(KERN_ERR
@@ -170,15 +181,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
page_crypt_complete, );
 
-   BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
-   xts_tweak.index = cpu_to_le64(lblk_num);
-   memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
-
sg_init_table(, 1);
sg_set_page(, dest_page, len, offs);
sg_init_table(, 1);
sg_set_page(, src_page, len, offs);
-   skcipher_request_set_crypt(req, , , len, _tweak);
+   skcipher_request_set_crypt(req, , , len, );
if (rw == FS_DECRYPT)
res = crypto_skcipher_decrypt(req);
else
@@ -477,6 +484,8 @@ static void __exit fscrypt_exit(void)
destroy_workqueue(fscrypt_read_workqueue);
kmem_cache_destroy(fscrypt_ctx_cachep);
kmem_cache_destroy(fscrypt_info_cachep);
+
+   fscrypt_essiv_cleanup();
 }
 module_exit(fscrypt_exit);
 
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 1e1f8a361b75..a1d5021c31ef 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -12,10 +12,13 @@
 #define _FSCRYPT_PRIVATE_H
 
 #include 
+#include 
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE  16
+#define FS_IV_SIZE 16
 #define FS_AES_128_ECB_KEY_SIZE16
+#define FS_AES_128_CBC_KEY_SIZE16
+#define FS_AES_128_CTS_KEY_SIZE16
 #define FS_AES_

Re: [RFC PATCH 0/2] crypto: caam - fix cts(cbc(aes)) with CAAM driver

[David Gstir]

Horia,

> On 16 Jun 2017, at 23:01, Horia Geantă  wrote:
> 
> On 6/16/2017 11:00 AM, Herbert Xu wrote:
>> On Fri, Jun 16, 2017 at 07:57:00AM +, Horia Geantă wrote:
>>> 
>>> Commit 0605c41cc53ca ("crypto: cts - Convert to skcipher") appends
>>> CRYPTO_TFM_REQ_MAY_BACKLOG to the original crypto request flags for the
>>> last block - when calling cts_cbc_encrypt().
>>> Is it really needed?
>> 
>> Yes, because at this point we cannot tell the sender to back off.
>> 
>>> For cts(cbc(aes)) with cbc(aes) offloaded in HW, i.e. running in async
>>> mode, we get the below stack for CAAM driver.
>>> Driver is told that it can sleep (CRYPTO_TFM_REQ_MAY_BACKLOG flag), so
>>> it uses GFP_KERNEL to allocate memory. However, this is incorrect, since
>>> driver runs in atomic context (softirq).
>> 
>> This is wrong.  Whether you can sleep or not is determined by
>> MAY_SLEEP, not MAY_BACKLOG.  MAY_BACKLOG only indicates that this
>> request must be queued, even if the queue is full.
>> 
> Indeed, CAAM driver incorrectly decides to use GFP_KERNEL for allocation
> when MAY_BACKLOG flag is set. This seems to be a long-standing issue, I
> will send a fix (separately).
> 
> Still I think we have a problem.
> David reported that the user is fscrypt. Looking into fscrypt code, I
> see that besides MAY_BACKLOG, MAY_SLEEP flag is also set. So we end up
> in the situation I described earlier: the last block is encrypted in
> atomic context and with MAY_SLEEP set.

Fixing the MAY_BACKLOG issue in the CAAM driver should get rid of the problem
altogether since the CTS code clears the MAY_SLEEP flag when encrypting the
last block [1].

David

[1] http://elixir.free-electrons.com/linux/v4.12-rc5/source/crypto/cts.c#L124

Re: [RFC PATCH 0/2] crypto: caam - fix cts(cbc(aes)) with CAAM driver

[David Gstir]

Horia,

> On 16 Jun 2017, at 23:01, Horia Geantă  wrote:
> 
> On 6/16/2017 11:00 AM, Herbert Xu wrote:
>> On Fri, Jun 16, 2017 at 07:57:00AM +, Horia Geantă wrote:
>>> 
>>> Commit 0605c41cc53ca ("crypto: cts - Convert to skcipher") appends
>>> CRYPTO_TFM_REQ_MAY_BACKLOG to the original crypto request flags for the
>>> last block - when calling cts_cbc_encrypt().
>>> Is it really needed?
>> 
>> Yes, because at this point we cannot tell the sender to back off.
>> 
>>> For cts(cbc(aes)) with cbc(aes) offloaded in HW, i.e. running in async
>>> mode, we get the below stack for CAAM driver.
>>> Driver is told that it can sleep (CRYPTO_TFM_REQ_MAY_BACKLOG flag), so
>>> it uses GFP_KERNEL to allocate memory. However, this is incorrect, since
>>> driver runs in atomic context (softirq).
>> 
>> This is wrong.  Whether you can sleep or not is determined by
>> MAY_SLEEP, not MAY_BACKLOG.  MAY_BACKLOG only indicates that this
>> request must be queued, even if the queue is full.
>> 
> Indeed, CAAM driver incorrectly decides to use GFP_KERNEL for allocation
> when MAY_BACKLOG flag is set. This seems to be a long-standing issue, I
> will send a fix (separately).
> 
> Still I think we have a problem.
> David reported that the user is fscrypt. Looking into fscrypt code, I
> see that besides MAY_BACKLOG, MAY_SLEEP flag is also set. So we end up
> in the situation I described earlier: the last block is encrypted in
> atomic context and with MAY_SLEEP set.

Fixing the MAY_BACKLOG issue in the CAAM driver should get rid of the problem
altogether since the CTS code clears the MAY_SLEEP flag when encrypting the
last block [1].

David

[1] http://elixir.free-electrons.com/linux/v4.12-rc5/source/crypto/cts.c#L124

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[David Gstir]

> On 15 Jun 2017, at 22:48, Eric Biggers  wrote:
> 
> On Thu, Jun 15, 2017 at 01:41:29PM -0700, Michael Halcrow wrote:
>>> static int validate_user_key(struct fscrypt_info *crypt_info,
>>> struct fscrypt_context *ctx, u8 *raw_key,
>>> -   const char *prefix)
>>> +   const char *prefix, int min_keysize)
>>> {
>>> char *description;
>>> struct key *keyring_key;
>>> @@ -111,50 +116,60 @@ static int validate_user_key(struct fscrypt_info 
>>> *crypt_info,
>>> master_key = (struct fscrypt_key *)ukp->data;
>>> BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
>>> 
>>> -   if (master_key->size != FS_AES_256_XTS_KEY_SIZE) {
>>> +   if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
>>> +   || master_key->size % AES_BLOCK_SIZE != 0) {
>> 
>> I suggest validating the provided key size directly against the mode.
>> Else, it looks to me that this code will accept a 128-bit key for
>> AES-256.
>> 
> 
> It's doing that already; min_keysize depends on the mode.

We are a bit more forgiving than the code was before: In case AES-128-CBC is
selected, we accept a longer key and use the first 128 bits of the derived key.
(see fscrypt_get_encryption_info())

The alternative is to make this check as strict as it was and just check for
master_key->size != min_keysize.

IMO the current check is okay. I will however add a comment that documents this.
We could also add a pr_warn_once(), but I don't think this is really necessary.

David

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[David Gstir]

> On 15 Jun 2017, at 22:48, Eric Biggers  wrote:
> 
> On Thu, Jun 15, 2017 at 01:41:29PM -0700, Michael Halcrow wrote:
>>> static int validate_user_key(struct fscrypt_info *crypt_info,
>>> struct fscrypt_context *ctx, u8 *raw_key,
>>> -   const char *prefix)
>>> +   const char *prefix, int min_keysize)
>>> {
>>> char *description;
>>> struct key *keyring_key;
>>> @@ -111,50 +116,60 @@ static int validate_user_key(struct fscrypt_info 
>>> *crypt_info,
>>> master_key = (struct fscrypt_key *)ukp->data;
>>> BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
>>> 
>>> -   if (master_key->size != FS_AES_256_XTS_KEY_SIZE) {
>>> +   if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
>>> +   || master_key->size % AES_BLOCK_SIZE != 0) {
>> 
>> I suggest validating the provided key size directly against the mode.
>> Else, it looks to me that this code will accept a 128-bit key for
>> AES-256.
>> 
> 
> It's doing that already; min_keysize depends on the mode.

We are a bit more forgiving than the code was before: In case AES-128-CBC is
selected, we accept a longer key and use the first 128 bits of the derived key.
(see fscrypt_get_encryption_info())

The alternative is to make this check as strict as it was and just check for
master_key->size != min_keysize.

IMO the current check is okay. I will however add a comment that documents this.
We could also add a pr_warn_once(), but I don't think this is really necessary.

David

Re: [RFC PATCH 0/2] crypto: caam - fix cts(cbc(aes)) with CAAM driver

[David Gstir]

Friendly ping. Any feedback on that?

Thanks,
David

> On 2 Jun 2017, at 14:24, David Gstir <da...@sigma-star.at> wrote:
> 
> Hi!
> 
> While testing fscrypt's filename encryption, I noticed that the implementation
> of cts(cbc(aes)) is broken when the CAAM hardware crypto driver is enabled.
> Some digging showed that the refactoring of crypto/cts.c in v4.8 
> (commit 0605c41cc53ca) exposed some problems with CAAM's aes-cbc
> implementation. This can be tested quite easily by loading the tcrypt
> module with mode=38. Looks like the cts mode is not used very often
> and this went unnoticed since 4.8...
> 
> This patch series is an attempt to fix that and get cts(cbc(aes)) working
> properly again when the CAAM driver is enabled.
> 
> Specifically, the issues are:
> 
> 1) The cts implementation assumes ->info of ablkcipher_request (or ->iv of
>   skcipher_request respectively) to be set to the last ciphertext block when
>   the aes-cbc encryption is finished. Meaning that ->info is changed by the
>   aes-cbc code. The CAAM driver does not do that and leaves ->info as-is.
> 
>   It is not fully clear to me yet if this is a requirement of the crypto API,
>   or if this is just a side effect that is exploited by the cts 
> implementation?
> 
>   For now, I assumed it is a requirement of the crypto API since I've seen
>   other crypto drivers (e.g. AMD's CCP) do that. So the patch fixes the CAAM
>   crypto driver accordingly.
> 
>   Also, the aead code in the CAAM driver, more specifically the gcm mode code
>   seems to have the same flaw, so it'll need a similar fix in case.
> 
> 
> 2) The cts implementation uses aes-cbc twice to perform its job. The second
>   time, it is called from within the callback of the first call to aes-cbc.
>   This usage is not properly handled in the CAAM driver which triggers the
>   BUG below.
> 
>   My current proposal is to use in_interrupt() to detect such cases and set
>   the k*alloc flags accordingly. However, since using in_interrupt() is not
>   really nice, I'm wondering if there is a better way to handle this?
> 
> 
> Thanks,
> David
> 
> 
> [  126.252543] BUG: sleeping function called from invalid context at 
> mm/slab.h:432
> [  126.260099] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
> [  126.266837] no locks held by swapper/0/0.
> [  126.270969] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
> 4.12.0-rc3-00052-g0ddec680d395 #287
> [  126.279226] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [  126.285821] Backtrace:
> [  126.288406] [] (dump_backtrace) from [] 
> (show_stack+0x18/0x1c)
> [  126.296057]  r7: r6:6113 r5: r4:c102ab48
> [  126.301798] [] (show_stack) from [] 
> (dump_stack+0xb4/0xe8)
> [  126.309106] [] (dump_stack) from [] 
> (___might_sleep+0x17c/0x2a0)
> [  126.316929]  r9: r8:c0a016dc r7:c101ee3e r6:01b0 r5:c0c12fac 
> r4:e000
> [  126.324751] [] (___might_sleep) from [] 
> (__might_sleep+0x64/0xa4)
> [  126.332655]  r7:014080c1 r6: r5:01b0 r4:c0c12fac
> [  126.338397] [] (__might_sleep) from [] 
> (__kmalloc+0x130/0x1b8)
> [  126.346039]  r6:c071a8ec r5:014080c1 r4:eec01e00
> [  126.350742] [] (__kmalloc) from [] 
> (ablkcipher_edesc_alloc.constprop.1+0x200/0x900)
> [  126.360213]  r10: r9: r8:c0a016dc r7:c0a016dc r6:ee05ac10 
> r5:edfdaec0
> [  126.368109]  r4:0001 r3:0020
> [  126.371765] [] (ablkcipher_edesc_alloc.constprop.1) from 
> [] (ablkcipher_encrypt+0x24/0x9c)
> [  126.381843]  r10: r9:0020 r8:0001 r7:ee05ac10 r6:ed597000 
> r5:edfdaec0
> [  126.389738]  r4:ed475d08
> [  126.392354] [] (ablkcipher_encrypt) from [] 
> (skcipher_encrypt_ablkcipher+0x68/0x6c)
> [  126.401822]  r7:ed475d08 r6:ed597000 r5:0400 r4:ed475d08
> [  126.407566] [] (skcipher_encrypt_ablkcipher) from [] 
> (cts_cbc_encrypt+0x118/0x124)
> [  126.416947]  r7:ed475d08 r6:c1001cc0 r5:0010 r4:edfdae00
> [  126.422686] [] (cts_cbc_encrypt) from [] 
> (crypto_cts_encrypt_done+0x20/0x54)
> [  126.431548]  r10: r9:ee05ac10 r8: r7:0010 r6:edc8e6c0 
> r5:edc8e6d8
> [  126.439443]  r4:edfdae00
> [  126.442056] [] (crypto_cts_encrypt_done) from [] 
> (ablkcipher_encrypt_done+0x88/0x9c)
> [  126.445180] fec 2188000.ethernet eth0: MDIO read timeout
> [  126.456948]  r5:edc8e6d8 r4:edfdaec0
> [  126.460604] [] (ablkcipher_encrypt_done) from [] 
> (caam_jr_dequeue+0x214/0x2d4)
> [  126.469639]  r9:0001 r8:ee088010 r7:01ff r6:0001 r5: 
> r4:edfdaec0
> [  126.477467] [] (caam_jr_dequeue) from [] 
> (tasklet_action+0x98/0x154)
> [  126.485160] fec 2188000.ethernet eth0: MDIO read timeout
> [  12

Re: [RFC PATCH 0/2] crypto: caam - fix cts(cbc(aes)) with CAAM driver

[David Gstir]

Friendly ping. Any feedback on that?

Thanks,
David

> On 2 Jun 2017, at 14:24, David Gstir  wrote:
> 
> Hi!
> 
> While testing fscrypt's filename encryption, I noticed that the implementation
> of cts(cbc(aes)) is broken when the CAAM hardware crypto driver is enabled.
> Some digging showed that the refactoring of crypto/cts.c in v4.8 
> (commit 0605c41cc53ca) exposed some problems with CAAM's aes-cbc
> implementation. This can be tested quite easily by loading the tcrypt
> module with mode=38. Looks like the cts mode is not used very often
> and this went unnoticed since 4.8...
> 
> This patch series is an attempt to fix that and get cts(cbc(aes)) working
> properly again when the CAAM driver is enabled.
> 
> Specifically, the issues are:
> 
> 1) The cts implementation assumes ->info of ablkcipher_request (or ->iv of
>   skcipher_request respectively) to be set to the last ciphertext block when
>   the aes-cbc encryption is finished. Meaning that ->info is changed by the
>   aes-cbc code. The CAAM driver does not do that and leaves ->info as-is.
> 
>   It is not fully clear to me yet if this is a requirement of the crypto API,
>   or if this is just a side effect that is exploited by the cts 
> implementation?
> 
>   For now, I assumed it is a requirement of the crypto API since I've seen
>   other crypto drivers (e.g. AMD's CCP) do that. So the patch fixes the CAAM
>   crypto driver accordingly.
> 
>   Also, the aead code in the CAAM driver, more specifically the gcm mode code
>   seems to have the same flaw, so it'll need a similar fix in case.
> 
> 
> 2) The cts implementation uses aes-cbc twice to perform its job. The second
>   time, it is called from within the callback of the first call to aes-cbc.
>   This usage is not properly handled in the CAAM driver which triggers the
>   BUG below.
> 
>   My current proposal is to use in_interrupt() to detect such cases and set
>   the k*alloc flags accordingly. However, since using in_interrupt() is not
>   really nice, I'm wondering if there is a better way to handle this?
> 
> 
> Thanks,
> David
> 
> 
> [  126.252543] BUG: sleeping function called from invalid context at 
> mm/slab.h:432
> [  126.260099] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
> [  126.266837] no locks held by swapper/0/0.
> [  126.270969] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
> 4.12.0-rc3-00052-g0ddec680d395 #287
> [  126.279226] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [  126.285821] Backtrace:
> [  126.288406] [] (dump_backtrace) from [] 
> (show_stack+0x18/0x1c)
> [  126.296057]  r7: r6:6113 r5: r4:c102ab48
> [  126.301798] [] (show_stack) from [] 
> (dump_stack+0xb4/0xe8)
> [  126.309106] [] (dump_stack) from [] 
> (___might_sleep+0x17c/0x2a0)
> [  126.316929]  r9: r8:c0a016dc r7:c101ee3e r6:01b0 r5:c0c12fac 
> r4:e000
> [  126.324751] [] (___might_sleep) from [] 
> (__might_sleep+0x64/0xa4)
> [  126.332655]  r7:014080c1 r6: r5:01b0 r4:c0c12fac
> [  126.338397] [] (__might_sleep) from [] 
> (__kmalloc+0x130/0x1b8)
> [  126.346039]  r6:c071a8ec r5:014080c1 r4:eec01e00
> [  126.350742] [] (__kmalloc) from [] 
> (ablkcipher_edesc_alloc.constprop.1+0x200/0x900)
> [  126.360213]  r10: r9: r8:c0a016dc r7:c0a016dc r6:ee05ac10 
> r5:edfdaec0
> [  126.368109]  r4:0001 r3:0020
> [  126.371765] [] (ablkcipher_edesc_alloc.constprop.1) from 
> [] (ablkcipher_encrypt+0x24/0x9c)
> [  126.381843]  r10: r9:0020 r8:0001 r7:ee05ac10 r6:ed597000 
> r5:edfdaec0
> [  126.389738]  r4:ed475d08
> [  126.392354] [] (ablkcipher_encrypt) from [] 
> (skcipher_encrypt_ablkcipher+0x68/0x6c)
> [  126.401822]  r7:ed475d08 r6:ed597000 r5:0400 r4:ed475d08
> [  126.407566] [] (skcipher_encrypt_ablkcipher) from [] 
> (cts_cbc_encrypt+0x118/0x124)
> [  126.416947]  r7:ed475d08 r6:c1001cc0 r5:0010 r4:edfdae00
> [  126.422686] [] (cts_cbc_encrypt) from [] 
> (crypto_cts_encrypt_done+0x20/0x54)
> [  126.431548]  r10: r9:ee05ac10 r8: r7:0010 r6:edc8e6c0 
> r5:edc8e6d8
> [  126.439443]  r4:edfdae00
> [  126.442056] [] (crypto_cts_encrypt_done) from [] 
> (ablkcipher_encrypt_done+0x88/0x9c)
> [  126.445180] fec 2188000.ethernet eth0: MDIO read timeout
> [  126.456948]  r5:edc8e6d8 r4:edfdaec0
> [  126.460604] [] (ablkcipher_encrypt_done) from [] 
> (caam_jr_dequeue+0x214/0x2d4)
> [  126.469639]  r9:0001 r8:ee088010 r7:01ff r6:0001 r5: 
> r4:edfdaec0
> [  126.477467] [] (caam_jr_dequeue) from [] 
> (tasklet_action+0x98/0x154)
> [  126.485160] fec 2188000.ethernet eth0: MDIO read timeout
> [  126.490975]  r10:c1080b80 r9:c10

[RFC PATCH 0/2] crypto: caam - fix cts(cbc(aes)) with CAAM driver

[David Gstir]

Hi!

While testing fscrypt's filename encryption, I noticed that the implementation
of cts(cbc(aes)) is broken when the CAAM hardware crypto driver is enabled.
Some digging showed that the refactoring of crypto/cts.c in v4.8 
(commit 0605c41cc53ca) exposed some problems with CAAM's aes-cbc
implementation. This can be tested quite easily by loading the tcrypt
module with mode=38. Looks like the cts mode is not used very often
and this went unnoticed since 4.8...

This patch series is an attempt to fix that and get cts(cbc(aes)) working
properly again when the CAAM driver is enabled.

Specifically, the issues are:

1) The cts implementation assumes ->info of ablkcipher_request (or ->iv of
   skcipher_request respectively) to be set to the last ciphertext block when
   the aes-cbc encryption is finished. Meaning that ->info is changed by the
   aes-cbc code. The CAAM driver does not do that and leaves ->info as-is.

   It is not fully clear to me yet if this is a requirement of the crypto API,
   or if this is just a side effect that is exploited by the cts implementation?

   For now, I assumed it is a requirement of the crypto API since I've seen
   other crypto drivers (e.g. AMD's CCP) do that. So the patch fixes the CAAM
   crypto driver accordingly.

   Also, the aead code in the CAAM driver, more specifically the gcm mode code
   seems to have the same flaw, so it'll need a similar fix in case.


2) The cts implementation uses aes-cbc twice to perform its job. The second
   time, it is called from within the callback of the first call to aes-cbc.
   This usage is not properly handled in the CAAM driver which triggers the
   BUG below.

   My current proposal is to use in_interrupt() to detect such cases and set
   the k*alloc flags accordingly. However, since using in_interrupt() is not
   really nice, I'm wondering if there is a better way to handle this?


Thanks,
David


[  126.252543] BUG: sleeping function called from invalid context at 
mm/slab.h:432
[  126.260099] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
[  126.266837] no locks held by swapper/0/0.
[  126.270969] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
4.12.0-rc3-00052-g0ddec680d395 #287
[  126.279226] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[  126.285821] Backtrace:
[  126.288406] [] (dump_backtrace) from [] 
(show_stack+0x18/0x1c)
[  126.296057]  r7: r6:6113 r5: r4:c102ab48
[  126.301798] [] (show_stack) from [] 
(dump_stack+0xb4/0xe8)
[  126.309106] [] (dump_stack) from [] 
(___might_sleep+0x17c/0x2a0)
[  126.316929]  r9: r8:c0a016dc r7:c101ee3e r6:01b0 r5:c0c12fac 
r4:e000
[  126.324751] [] (___might_sleep) from [] 
(__might_sleep+0x64/0xa4)
[  126.332655]  r7:014080c1 r6: r5:01b0 r4:c0c12fac
[  126.338397] [] (__might_sleep) from [] 
(__kmalloc+0x130/0x1b8)
[  126.346039]  r6:c071a8ec r5:014080c1 r4:eec01e00
[  126.350742] [] (__kmalloc) from [] 
(ablkcipher_edesc_alloc.constprop.1+0x200/0x900)
[  126.360213]  r10: r9: r8:c0a016dc r7:c0a016dc r6:ee05ac10 
r5:edfdaec0
[  126.368109]  r4:0001 r3:0020
[  126.371765] [] (ablkcipher_edesc_alloc.constprop.1) from 
[] (ablkcipher_encrypt+0x24/0x9c)
[  126.381843]  r10: r9:0020 r8:0001 r7:ee05ac10 r6:ed597000 
r5:edfdaec0
[  126.389738]  r4:ed475d08
[  126.392354] [] (ablkcipher_encrypt) from [] 
(skcipher_encrypt_ablkcipher+0x68/0x6c)
[  126.401822]  r7:ed475d08 r6:ed597000 r5:0400 r4:ed475d08
[  126.407566] [] (skcipher_encrypt_ablkcipher) from [] 
(cts_cbc_encrypt+0x118/0x124)
[  126.416947]  r7:ed475d08 r6:c1001cc0 r5:0010 r4:edfdae00
[  126.422686] [] (cts_cbc_encrypt) from [] 
(crypto_cts_encrypt_done+0x20/0x54)
[  126.431548]  r10: r9:ee05ac10 r8: r7:0010 r6:edc8e6c0 
r5:edc8e6d8
[  126.439443]  r4:edfdae00
[  126.442056] [] (crypto_cts_encrypt_done) from [] 
(ablkcipher_encrypt_done+0x88/0x9c)
[  126.445180] fec 2188000.ethernet eth0: MDIO read timeout
[  126.456948]  r5:edc8e6d8 r4:edfdaec0
[  126.460604] [] (ablkcipher_encrypt_done) from [] 
(caam_jr_dequeue+0x214/0x2d4)
[  126.469639]  r9:0001 r8:ee088010 r7:01ff r6:0001 r5: 
r4:edfdaec0
[  126.477467] [] (caam_jr_dequeue) from [] 
(tasklet_action+0x98/0x154)
[  126.485160] fec 2188000.ethernet eth0: MDIO read timeout
[  126.490975]  r10:c1080b80 r9:c1008b84 r8: r7:c100 r6: 
r5:ee088028
[  126.498870]  r4:ee088024
[  126.501484] [] (tasklet_action) from [] 
(__do_softirq+0xf0/0x2a4)
[  126.509390]  r10:4006 r9:c1002080 r8:0101 r7:c1002098 r6:c100 
r5:0006
[  126.517285]  r4:
[  126.519896] [] (__do_softirq) from [] 
(irq_exit+0xec/0x168)
[  126.525127] fec 2188000.ethernet eth0: MDIO write timeout
[  126.532709]  r10:c1008cf0 r9:eec08400 r8:0001 r7: r6:c1008b84 
r5:
[  126.540603]  r4:c0fd940c
[  126.543222] [] (irq_exit) from [] 
(__handle_domain_irq+0x74/0xe8)
[  126.551135] [] (__handle_domain_irq) 

[RFC PATCH 0/2] crypto: caam - fix cts(cbc(aes)) with CAAM driver

[David Gstir]

Hi!

While testing fscrypt's filename encryption, I noticed that the implementation
of cts(cbc(aes)) is broken when the CAAM hardware crypto driver is enabled.
Some digging showed that the refactoring of crypto/cts.c in v4.8 
(commit 0605c41cc53ca) exposed some problems with CAAM's aes-cbc
implementation. This can be tested quite easily by loading the tcrypt
module with mode=38. Looks like the cts mode is not used very often
and this went unnoticed since 4.8...

This patch series is an attempt to fix that and get cts(cbc(aes)) working
properly again when the CAAM driver is enabled.

Specifically, the issues are:

1) The cts implementation assumes ->info of ablkcipher_request (or ->iv of
   skcipher_request respectively) to be set to the last ciphertext block when
   the aes-cbc encryption is finished. Meaning that ->info is changed by the
   aes-cbc code. The CAAM driver does not do that and leaves ->info as-is.

   It is not fully clear to me yet if this is a requirement of the crypto API,
   or if this is just a side effect that is exploited by the cts implementation?

   For now, I assumed it is a requirement of the crypto API since I've seen
   other crypto drivers (e.g. AMD's CCP) do that. So the patch fixes the CAAM
   crypto driver accordingly.

   Also, the aead code in the CAAM driver, more specifically the gcm mode code
   seems to have the same flaw, so it'll need a similar fix in case.


2) The cts implementation uses aes-cbc twice to perform its job. The second
   time, it is called from within the callback of the first call to aes-cbc.
   This usage is not properly handled in the CAAM driver which triggers the
   BUG below.

   My current proposal is to use in_interrupt() to detect such cases and set
   the k*alloc flags accordingly. However, since using in_interrupt() is not
   really nice, I'm wondering if there is a better way to handle this?


Thanks,
David


[  126.252543] BUG: sleeping function called from invalid context at 
mm/slab.h:432
[  126.260099] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
[  126.266837] no locks held by swapper/0/0.
[  126.270969] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
4.12.0-rc3-00052-g0ddec680d395 #287
[  126.279226] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[  126.285821] Backtrace:
[  126.288406] [] (dump_backtrace) from [] 
(show_stack+0x18/0x1c)
[  126.296057]  r7: r6:6113 r5: r4:c102ab48
[  126.301798] [] (show_stack) from [] 
(dump_stack+0xb4/0xe8)
[  126.309106] [] (dump_stack) from [] 
(___might_sleep+0x17c/0x2a0)
[  126.316929]  r9: r8:c0a016dc r7:c101ee3e r6:01b0 r5:c0c12fac 
r4:e000
[  126.324751] [] (___might_sleep) from [] 
(__might_sleep+0x64/0xa4)
[  126.332655]  r7:014080c1 r6: r5:01b0 r4:c0c12fac
[  126.338397] [] (__might_sleep) from [] 
(__kmalloc+0x130/0x1b8)
[  126.346039]  r6:c071a8ec r5:014080c1 r4:eec01e00
[  126.350742] [] (__kmalloc) from [] 
(ablkcipher_edesc_alloc.constprop.1+0x200/0x900)
[  126.360213]  r10: r9: r8:c0a016dc r7:c0a016dc r6:ee05ac10 
r5:edfdaec0
[  126.368109]  r4:0001 r3:0020
[  126.371765] [] (ablkcipher_edesc_alloc.constprop.1) from 
[] (ablkcipher_encrypt+0x24/0x9c)
[  126.381843]  r10: r9:0020 r8:0001 r7:ee05ac10 r6:ed597000 
r5:edfdaec0
[  126.389738]  r4:ed475d08
[  126.392354] [] (ablkcipher_encrypt) from [] 
(skcipher_encrypt_ablkcipher+0x68/0x6c)
[  126.401822]  r7:ed475d08 r6:ed597000 r5:0400 r4:ed475d08
[  126.407566] [] (skcipher_encrypt_ablkcipher) from [] 
(cts_cbc_encrypt+0x118/0x124)
[  126.416947]  r7:ed475d08 r6:c1001cc0 r5:0010 r4:edfdae00
[  126.422686] [] (cts_cbc_encrypt) from [] 
(crypto_cts_encrypt_done+0x20/0x54)
[  126.431548]  r10: r9:ee05ac10 r8: r7:0010 r6:edc8e6c0 
r5:edc8e6d8
[  126.439443]  r4:edfdae00
[  126.442056] [] (crypto_cts_encrypt_done) from [] 
(ablkcipher_encrypt_done+0x88/0x9c)
[  126.445180] fec 2188000.ethernet eth0: MDIO read timeout
[  126.456948]  r5:edc8e6d8 r4:edfdaec0
[  126.460604] [] (ablkcipher_encrypt_done) from [] 
(caam_jr_dequeue+0x214/0x2d4)
[  126.469639]  r9:0001 r8:ee088010 r7:01ff r6:0001 r5: 
r4:edfdaec0
[  126.477467] [] (caam_jr_dequeue) from [] 
(tasklet_action+0x98/0x154)
[  126.485160] fec 2188000.ethernet eth0: MDIO read timeout
[  126.490975]  r10:c1080b80 r9:c1008b84 r8: r7:c100 r6: 
r5:ee088028
[  126.498870]  r4:ee088024
[  126.501484] [] (tasklet_action) from [] 
(__do_softirq+0xf0/0x2a4)
[  126.509390]  r10:4006 r9:c1002080 r8:0101 r7:c1002098 r6:c100 
r5:0006
[  126.517285]  r4:
[  126.519896] [] (__do_softirq) from [] 
(irq_exit+0xec/0x168)
[  126.525127] fec 2188000.ethernet eth0: MDIO write timeout
[  126.532709]  r10:c1008cf0 r9:eec08400 r8:0001 r7: r6:c1008b84 
r5:
[  126.540603]  r4:c0fd940c
[  126.543222] [] (irq_exit) from [] 
(__handle_domain_irq+0x74/0xe8)
[  126.551135] [] (__handle_domain_irq) 

[RFC PATCH 2/2] crypto: caam - fix k*alloc if called from own cipher callback

[David Gstir]

There are cases (e.g. the cts mode) where a cipher can be called again
from its own callback. In our case this callback is executed from within
a tasklet in the jobring, we must not sleep when allocating memory.

This patch detects such cases by using in_interrupt() to properly set the
k*alloc flags. In most cases we will still use GFP_KERNEL if the flags
CRYPTO_TFM_REQ_MAY_SLEEP or CRYPTO_TFM_REQ_MAY_BACKLOG are set for the
cipher request.

Signed-off-by: David Gstir <da...@sigma-star.at>
---
 drivers/crypto/caam/caamalg.c | 29 +
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index d13c1aee4427..4c4a5d1ad875 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -1209,13 +1209,18 @@ static struct aead_edesc *aead_edesc_alloc(struct 
aead_request *req,
struct crypto_aead *aead = crypto_aead_reqtfm(req);
struct caam_ctx *ctx = crypto_aead_ctx(aead);
struct device *jrdev = ctx->jrdev;
-   gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
-  CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+   gfp_t flags;
int src_nents, mapped_src_nents, dst_nents = 0, mapped_dst_nents = 0;
struct aead_edesc *edesc;
int sec4_sg_index, sec4_sg_len, sec4_sg_bytes;
unsigned int authsize = ctx->authsize;
 
+   if (!in_interrupt() && req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
+ CRYPTO_TFM_REQ_MAY_SLEEP))
+   flags = GFP_KERNEL;
+   else
+   flags = GFP_ATOMIC;
+
if (unlikely(req->dst != req->src)) {
src_nents = sg_nents_for_len(req->src, req->assoclen +
 req->cryptlen);
@@ -1497,9 +1502,7 @@ static struct ablkcipher_edesc 
*ablkcipher_edesc_alloc(struct ablkcipher_request
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
struct caam_ctx *ctx = crypto_ablkcipher_ctx(ablkcipher);
struct device *jrdev = ctx->jrdev;
-   gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ?
-  GFP_KERNEL : GFP_ATOMIC;
+   gfp_t flags;
int src_nents, mapped_src_nents, dst_nents = 0, mapped_dst_nents = 0;
struct ablkcipher_edesc *edesc;
dma_addr_t iv_dma = 0;
@@ -1507,6 +1510,12 @@ static struct ablkcipher_edesc 
*ablkcipher_edesc_alloc(struct ablkcipher_request
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
int dst_sg_idx, sec4_sg_ents, sec4_sg_bytes;
 
+   if (!in_interrupt() && req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
+ CRYPTO_TFM_REQ_MAY_SLEEP))
+   flags = GFP_KERNEL;
+   else
+   flags = GFP_ATOMIC;
+
src_nents = sg_nents_for_len(req->src, req->nbytes);
if (unlikely(src_nents < 0)) {
dev_err(jrdev, "Insufficient bytes (%d) in src S/G\n",
@@ -1703,9 +1712,7 @@ static struct ablkcipher_edesc 
*ablkcipher_giv_edesc_alloc(
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
struct caam_ctx *ctx = crypto_ablkcipher_ctx(ablkcipher);
struct device *jrdev = ctx->jrdev;
-   gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ?
-  GFP_KERNEL : GFP_ATOMIC;
+   gfp_t flags;
int src_nents, mapped_src_nents, dst_nents, mapped_dst_nents;
struct ablkcipher_edesc *edesc;
dma_addr_t iv_dma = 0;
@@ -1713,6 +1720,12 @@ static struct ablkcipher_edesc 
*ablkcipher_giv_edesc_alloc(
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
int dst_sg_idx, sec4_sg_ents, sec4_sg_bytes;
 
+   if (!in_interrupt() && req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
+ CRYPTO_TFM_REQ_MAY_SLEEP))
+   flags = GFP_KERNEL;
+   else
+   flags = GFP_ATOMIC;
+
src_nents = sg_nents_for_len(req->src, req->nbytes);
if (unlikely(src_nents < 0)) {
dev_err(jrdev, "Insufficient bytes (%d) in src S/G\n",
-- 
2.12.0

[RFC PATCH 2/2] crypto: caam - fix k*alloc if called from own cipher callback

[David Gstir]

There are cases (e.g. the cts mode) where a cipher can be called again
from its own callback. In our case this callback is executed from within
a tasklet in the jobring, we must not sleep when allocating memory.

This patch detects such cases by using in_interrupt() to properly set the
k*alloc flags. In most cases we will still use GFP_KERNEL if the flags
CRYPTO_TFM_REQ_MAY_SLEEP or CRYPTO_TFM_REQ_MAY_BACKLOG are set for the
cipher request.

Signed-off-by: David Gstir 
---
 drivers/crypto/caam/caamalg.c | 29 +
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index d13c1aee4427..4c4a5d1ad875 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -1209,13 +1209,18 @@ static struct aead_edesc *aead_edesc_alloc(struct 
aead_request *req,
struct crypto_aead *aead = crypto_aead_reqtfm(req);
struct caam_ctx *ctx = crypto_aead_ctx(aead);
struct device *jrdev = ctx->jrdev;
-   gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
-  CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+   gfp_t flags;
int src_nents, mapped_src_nents, dst_nents = 0, mapped_dst_nents = 0;
struct aead_edesc *edesc;
int sec4_sg_index, sec4_sg_len, sec4_sg_bytes;
unsigned int authsize = ctx->authsize;
 
+   if (!in_interrupt() && req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
+ CRYPTO_TFM_REQ_MAY_SLEEP))
+   flags = GFP_KERNEL;
+   else
+   flags = GFP_ATOMIC;
+
if (unlikely(req->dst != req->src)) {
src_nents = sg_nents_for_len(req->src, req->assoclen +
 req->cryptlen);
@@ -1497,9 +1502,7 @@ static struct ablkcipher_edesc 
*ablkcipher_edesc_alloc(struct ablkcipher_request
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
struct caam_ctx *ctx = crypto_ablkcipher_ctx(ablkcipher);
struct device *jrdev = ctx->jrdev;
-   gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ?
-  GFP_KERNEL : GFP_ATOMIC;
+   gfp_t flags;
int src_nents, mapped_src_nents, dst_nents = 0, mapped_dst_nents = 0;
struct ablkcipher_edesc *edesc;
dma_addr_t iv_dma = 0;
@@ -1507,6 +1510,12 @@ static struct ablkcipher_edesc 
*ablkcipher_edesc_alloc(struct ablkcipher_request
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
int dst_sg_idx, sec4_sg_ents, sec4_sg_bytes;
 
+   if (!in_interrupt() && req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
+ CRYPTO_TFM_REQ_MAY_SLEEP))
+   flags = GFP_KERNEL;
+   else
+   flags = GFP_ATOMIC;
+
src_nents = sg_nents_for_len(req->src, req->nbytes);
if (unlikely(src_nents < 0)) {
dev_err(jrdev, "Insufficient bytes (%d) in src S/G\n",
@@ -1703,9 +1712,7 @@ static struct ablkcipher_edesc 
*ablkcipher_giv_edesc_alloc(
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
struct caam_ctx *ctx = crypto_ablkcipher_ctx(ablkcipher);
struct device *jrdev = ctx->jrdev;
-   gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ?
-  GFP_KERNEL : GFP_ATOMIC;
+   gfp_t flags;
int src_nents, mapped_src_nents, dst_nents, mapped_dst_nents;
struct ablkcipher_edesc *edesc;
dma_addr_t iv_dma = 0;
@@ -1713,6 +1720,12 @@ static struct ablkcipher_edesc 
*ablkcipher_giv_edesc_alloc(
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
int dst_sg_idx, sec4_sg_ents, sec4_sg_bytes;
 
+   if (!in_interrupt() && req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
+ CRYPTO_TFM_REQ_MAY_SLEEP))
+   flags = GFP_KERNEL;
+   else
+   flags = GFP_ATOMIC;
+
src_nents = sg_nents_for_len(req->src, req->nbytes);
if (unlikely(src_nents < 0)) {
dev_err(jrdev, "Insufficient bytes (%d) in src S/G\n",
-- 
2.12.0

[RFC PATCH 1/2] crypto: caam - properly set IV after {en,de}crypt

[David Gstir]

Certain cipher modes like CTS expect the IV (req->info) of
ablkcipher_request (or equivalently req->iv of skcipher_request) to
contain the last ciphertext block when the {en,de}crypt operation is done.
This is currently not the case for the CAAM driver which in turn breaks
e.g. cts(cbc(aes)) when the CAAM driver is enabled.

This patch fixes the CAAM driver to properly set the IV after the
{en,de}crypt operation of ablkcipher finishes.

Signed-off-by: David Gstir <da...@sigma-star.at>
---
 drivers/crypto/caam/caamalg.c | 26 --
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 398807d1b77e..d13c1aee4427 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -882,10 +882,11 @@ static void ablkcipher_encrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 {
struct ablkcipher_request *req = context;
struct ablkcipher_edesc *edesc;
-#ifdef DEBUG
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
+   int nents;
 
+#ifdef DEBUG
dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
 #endif
 
@@ -904,6 +905,19 @@ static void ablkcipher_encrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 #endif
 
ablkcipher_unmap(jrdev, edesc, req);
+
+   if (req->src == req->dst)
+   nents = edesc->src_nents;
+   else
+   nents = edesc->dst_nents;
+
+   /*
+* The crypto API expects us to set the IV (req->info) to the last
+* ciphertext block. This is used e.g. by the CTS mode.
+*/
+   sg_pcopy_to_buffer(req->dst, nents, req->info, ivsize,
+  req->nbytes - ivsize);
+
kfree(edesc);
 
ablkcipher_request_complete(req, err);
@@ -914,10 +928,10 @@ static void ablkcipher_decrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 {
struct ablkcipher_request *req = context;
struct ablkcipher_edesc *edesc;
-#ifdef DEBUG
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
 
+#ifdef DEBUG
dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
 #endif
 
@@ -935,6 +949,14 @@ static void ablkcipher_decrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 #endif
 
ablkcipher_unmap(jrdev, edesc, req);
+
+   /*
+* The crypto API expects us to set the IV (req->info) to the last
+* ciphertext block.
+*/
+   sg_pcopy_to_buffer(req->src, edesc->src_nents, req->info, ivsize,
+  req->nbytes - ivsize);
+
kfree(edesc);
 
ablkcipher_request_complete(req, err);
-- 
2.12.0

[RFC PATCH 1/2] crypto: caam - properly set IV after {en,de}crypt

[David Gstir]

Certain cipher modes like CTS expect the IV (req->info) of
ablkcipher_request (or equivalently req->iv of skcipher_request) to
contain the last ciphertext block when the {en,de}crypt operation is done.
This is currently not the case for the CAAM driver which in turn breaks
e.g. cts(cbc(aes)) when the CAAM driver is enabled.

This patch fixes the CAAM driver to properly set the IV after the
{en,de}crypt operation of ablkcipher finishes.

Signed-off-by: David Gstir 
---
 drivers/crypto/caam/caamalg.c | 26 --
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 398807d1b77e..d13c1aee4427 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -882,10 +882,11 @@ static void ablkcipher_encrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 {
struct ablkcipher_request *req = context;
struct ablkcipher_edesc *edesc;
-#ifdef DEBUG
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
+   int nents;
 
+#ifdef DEBUG
dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
 #endif
 
@@ -904,6 +905,19 @@ static void ablkcipher_encrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 #endif
 
ablkcipher_unmap(jrdev, edesc, req);
+
+   if (req->src == req->dst)
+   nents = edesc->src_nents;
+   else
+   nents = edesc->dst_nents;
+
+   /*
+* The crypto API expects us to set the IV (req->info) to the last
+* ciphertext block. This is used e.g. by the CTS mode.
+*/
+   sg_pcopy_to_buffer(req->dst, nents, req->info, ivsize,
+  req->nbytes - ivsize);
+
kfree(edesc);
 
ablkcipher_request_complete(req, err);
@@ -914,10 +928,10 @@ static void ablkcipher_decrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 {
struct ablkcipher_request *req = context;
struct ablkcipher_edesc *edesc;
-#ifdef DEBUG
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
 
+#ifdef DEBUG
dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
 #endif
 
@@ -935,6 +949,14 @@ static void ablkcipher_decrypt_done(struct device *jrdev, 
u32 *desc, u32 err,
 #endif
 
ablkcipher_unmap(jrdev, edesc, req);
+
+   /*
+* The crypto API expects us to set the IV (req->info) to the last
+* ciphertext block.
+*/
+   sg_pcopy_to_buffer(req->src, edesc->src_nents, req->info, ivsize,
+  req->nbytes - ivsize);
+
kfree(edesc);
 
ablkcipher_request_complete(req, err);
-- 
2.12.0

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[David Gstir]

Hi Eric,

> On 23 May 2017, at 21:00, Eric Biggers <ebigge...@gmail.com> wrote:
> 
> Hi David,
> 
> On Tue, May 23, 2017 at 07:11:20AM +0200, David Gstir wrote:
>> From: Daniel Walter <dwal...@sigma-star.at>
>> 
>> fscrypt provides facilities to use different encryption algorithms which
>> are selectable by userspace when setting the encryption policy. Currently,
>> only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
>> implemented. This is a clear case of kernel offers the mechanism and
>> userspace selects a policy. Similar to what dm-crypt and ecryptfs have.
>> 
>> This patch adds support for using AES-128-CBC for file contents and
>> AES-128-CBC-CTS for file name encryption. To mitigate watermarking
>> attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
>> actually slightly less secure than AES-XTS from a security point of view,
>> there is more widespread hardware support. Using AES-CBC gives us the
>> acceptable performance while still providing a moderate level of security
>> for persistent storage.
>> 
>> Especially low-powered embedded devices with crypto accelerators such as
>> CAAM or CESA often only support AES-CBC. Since using AES-CBC over AES-XTS
>> is basically thought of a last resort, we use AES-128-CBC over AES-256-CBC
>> since it has less encryption rounds and yields noticeable better
>> performance starting from a file size of just a few kB.
>> 
>> Signed-off-by: Daniel Walter <dwal...@sigma-star.at>
>> [da...@sigma-star.at: addressed review comments]
>> Signed-off-by: David Gstir <da...@sigma-star.at>
> 
> Overall this looks good now; you can add
> 
> Reviewed-by: Eric Biggers <ebigg...@google.com>

Thanks! :)


> I did notice a couple minor improvements that can be made, though:
> 
>> 
>> +if (crypt_info->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
>> +res = init_essiv_generator(crypt_info, raw_key, keysize);
>> +if (res) {
>> +pr_debug("%s: error %d (inode %lu) allocating essiv 
>> tfm\n",
>> + __func__, res, inode->i_ino);
>> +goto out;
>> +}
>> +}
> 
> Since the ESSIV generator is only needed for contents encryption, it should 
> only
> be initialized when both 'S_ISREG(inode->i_mode) && crypt_info->ci_data_mode 
> ==
> FS_ENCRYPTION_MODE_AES_128_CBC'.  Otherwise ->ci_essiv_tfm will be allocated 
> for
> directories and symlinks too, then never used.
> 
>> +static int init_essiv_generator(struct fscrypt_info *ci, const u8 *raw_key,
>> +int keysize)
>> +{
>> +int err;
>> +struct crypto_cipher *essiv_tfm;
>> +u8 salt[SHA256_DIGEST_SIZE];
>> +
>> +if (WARN_ON_ONCE(keysize > sizeof(salt)))
>> +return -EINVAL;
>> +
> 
> The 'keysize > sizeof(salt)' check is now pointless and should be removed, 
> since
> we decided not to key the ESSIV cipher with 'keysize' bytes, but rather with
> sizeof(salt) bytes.  So this function is compatible with any 'keysize', not 
> just
> keysize <= sizeof(salt).

You're right. Just let me know if I should send a new version of this patch 
with these minor issues fixed.


> You should also consider how it should be made possible to test these new
> encryption modes in xfstests.  Currently, while the "set_encpolicy" xfs_io
> command allows specifying different encryption modes and flags, in general the
> tests in the "encrypt" group are hardcoded to use AES_256_XTS and AES_256_CTS.
> Similarly, those modes are also used with the test_dummy_encryption mount
> option, which causes all new files to be automatically encrypted, and is used 
> by
> the "encrypt" config for kvm-xfstests and gce-xfstests (currently 
> ext4-specific,
> but other filesystems could support it too).

Sure! I'll do that.

Thanks,
David

Re: [PATCH v4] fscrypt: Add support for AES-128-CBC

[David Gstir]

Hi Eric,

> On 23 May 2017, at 21:00, Eric Biggers  wrote:
> 
> Hi David,
> 
> On Tue, May 23, 2017 at 07:11:20AM +0200, David Gstir wrote:
>> From: Daniel Walter 
>> 
>> fscrypt provides facilities to use different encryption algorithms which
>> are selectable by userspace when setting the encryption policy. Currently,
>> only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
>> implemented. This is a clear case of kernel offers the mechanism and
>> userspace selects a policy. Similar to what dm-crypt and ecryptfs have.
>> 
>> This patch adds support for using AES-128-CBC for file contents and
>> AES-128-CBC-CTS for file name encryption. To mitigate watermarking
>> attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
>> actually slightly less secure than AES-XTS from a security point of view,
>> there is more widespread hardware support. Using AES-CBC gives us the
>> acceptable performance while still providing a moderate level of security
>> for persistent storage.
>> 
>> Especially low-powered embedded devices with crypto accelerators such as
>> CAAM or CESA often only support AES-CBC. Since using AES-CBC over AES-XTS
>> is basically thought of a last resort, we use AES-128-CBC over AES-256-CBC
>> since it has less encryption rounds and yields noticeable better
>> performance starting from a file size of just a few kB.
>> 
>> Signed-off-by: Daniel Walter 
>> [da...@sigma-star.at: addressed review comments]
>> Signed-off-by: David Gstir 
> 
> Overall this looks good now; you can add
> 
> Reviewed-by: Eric Biggers 

Thanks! :)


> I did notice a couple minor improvements that can be made, though:
> 
>> 
>> +if (crypt_info->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
>> +res = init_essiv_generator(crypt_info, raw_key, keysize);
>> +if (res) {
>> +pr_debug("%s: error %d (inode %lu) allocating essiv 
>> tfm\n",
>> + __func__, res, inode->i_ino);
>> +goto out;
>> +}
>> +}
> 
> Since the ESSIV generator is only needed for contents encryption, it should 
> only
> be initialized when both 'S_ISREG(inode->i_mode) && crypt_info->ci_data_mode 
> ==
> FS_ENCRYPTION_MODE_AES_128_CBC'.  Otherwise ->ci_essiv_tfm will be allocated 
> for
> directories and symlinks too, then never used.
> 
>> +static int init_essiv_generator(struct fscrypt_info *ci, const u8 *raw_key,
>> +int keysize)
>> +{
>> +int err;
>> +struct crypto_cipher *essiv_tfm;
>> +u8 salt[SHA256_DIGEST_SIZE];
>> +
>> +if (WARN_ON_ONCE(keysize > sizeof(salt)))
>> +return -EINVAL;
>> +
> 
> The 'keysize > sizeof(salt)' check is now pointless and should be removed, 
> since
> we decided not to key the ESSIV cipher with 'keysize' bytes, but rather with
> sizeof(salt) bytes.  So this function is compatible with any 'keysize', not 
> just
> keysize <= sizeof(salt).

You're right. Just let me know if I should send a new version of this patch 
with these minor issues fixed.


> You should also consider how it should be made possible to test these new
> encryption modes in xfstests.  Currently, while the "set_encpolicy" xfs_io
> command allows specifying different encryption modes and flags, in general the
> tests in the "encrypt" group are hardcoded to use AES_256_XTS and AES_256_CTS.
> Similarly, those modes are also used with the test_dummy_encryption mount
> option, which causes all new files to be automatically encrypted, and is used 
> by
> the "encrypt" config for kvm-xfstests and gce-xfstests (currently 
> ext4-specific,
> but other filesystems could support it too).

Sure! I'll do that.

Thanks,
David

[PATCH v4] fscrypt: Add support for AES-128-CBC

[David Gstir]

From: Daniel Walter <dwal...@sigma-star.at>

fscrypt provides facilities to use different encryption algorithms which
are selectable by userspace when setting the encryption policy. Currently,
only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
implemented. This is a clear case of kernel offers the mechanism and
userspace selects a policy. Similar to what dm-crypt and ecryptfs have.

This patch adds support for using AES-128-CBC for file contents and
AES-128-CBC-CTS for file name encryption. To mitigate watermarking
attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
actually slightly less secure than AES-XTS from a security point of view,
there is more widespread hardware support. Using AES-CBC gives us the
acceptable performance while still providing a moderate level of security
for persistent storage.

Especially low-powered embedded devices with crypto accelerators such as
CAAM or CESA often only support AES-CBC. Since using AES-CBC over AES-XTS
is basically thought of a last resort, we use AES-128-CBC over AES-256-CBC
since it has less encryption rounds and yields noticeable better
performance starting from a file size of just a few kB.

Signed-off-by: Daniel Walter <dwal...@sigma-star.at>
[da...@sigma-star.at: addressed review comments]
Signed-off-by: David Gstir <da...@sigma-star.at>
---
 fs/crypto/Kconfig  |   1 +
 fs/crypto/crypto.c |  23 --
 fs/crypto/fscrypt_private.h|   9 ++-
 fs/crypto/keyinfo.c| 171 -
 fs/crypto/policy.c |   8 +-
 include/linux/fscrypt_common.h |  16 ++--
 include/uapi/linux/fs.h|   2 +
 7 files changed, 172 insertions(+), 58 deletions(-)

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index 08b46e6e3995..02b7d91c9231 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -7,6 +7,7 @@ config FS_ENCRYPTION
select CRYPTO_XTS
select CRYPTO_CTS
select CRYPTO_CTR
+   select CRYPTO_SHA256
select KEYS
help
  Enable encryption of files and directories.  This
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6d6eca394d4d..c7835df7e7b8 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "fscrypt_private.h"
 
 static unsigned int num_prealloc_crypto_pages = 32;
@@ -147,8 +148,8 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 {
struct {
__le64 index;
-   u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
-   } xts_tweak;
+   u8 padding[FS_IV_SIZE - sizeof(__le64)];
+   } iv;
struct skcipher_request *req = NULL;
DECLARE_FS_COMPLETION_RESULT(ecr);
struct scatterlist dst, src;
@@ -158,6 +159,16 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 
BUG_ON(len == 0);
 
+   BUILD_BUG_ON(sizeof(iv) != FS_IV_SIZE);
+   BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
+   iv.index = cpu_to_le64(lblk_num);
+   memset(iv.padding, 0, sizeof(iv.padding));
+
+   if (ci->ci_essiv_tfm != NULL) {
+   crypto_cipher_encrypt_one(ci->ci_essiv_tfm, (u8 *),
+ (u8 *));
+   }
+
req = skcipher_request_alloc(tfm, gfp_flags);
if (!req) {
printk_ratelimited(KERN_ERR
@@ -170,15 +181,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
page_crypt_complete, );
 
-   BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
-   xts_tweak.index = cpu_to_le64(lblk_num);
-   memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
-
sg_init_table(, 1);
sg_set_page(, dest_page, len, offs);
sg_init_table(, 1);
sg_set_page(, src_page, len, offs);
-   skcipher_request_set_crypt(req, , , len, _tweak);
+   skcipher_request_set_crypt(req, , , len, );
if (rw == FS_DECRYPT)
res = crypto_skcipher_decrypt(req);
else
@@ -477,6 +484,8 @@ static void __exit fscrypt_exit(void)
destroy_workqueue(fscrypt_read_workqueue);
kmem_cache_destroy(fscrypt_ctx_cachep);
kmem_cache_destroy(fscrypt_info_cachep);
+
+   fscrypt_essiv_cleanup();
 }
 module_exit(fscrypt_exit);
 
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 1e1f8a361b75..a1d5021c31ef 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -12,10 +12,13 @@
 #define _FSCRYPT_PRIVATE_H
 
 #include 
+#include 
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE  16
+#define FS_IV_SIZE 16
 #define FS_AES_128_ECB_KEY_SIZE16
+#define FS_AES_128_CBC_KEY_SIZE1

[PATCH v4] fscrypt: Add support for AES-128-CBC

[David Gstir]

From: Daniel Walter 

fscrypt provides facilities to use different encryption algorithms which
are selectable by userspace when setting the encryption policy. Currently,
only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
implemented. This is a clear case of kernel offers the mechanism and
userspace selects a policy. Similar to what dm-crypt and ecryptfs have.

This patch adds support for using AES-128-CBC for file contents and
AES-128-CBC-CTS for file name encryption. To mitigate watermarking
attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
actually slightly less secure than AES-XTS from a security point of view,
there is more widespread hardware support. Using AES-CBC gives us the
acceptable performance while still providing a moderate level of security
for persistent storage.

Especially low-powered embedded devices with crypto accelerators such as
CAAM or CESA often only support AES-CBC. Since using AES-CBC over AES-XTS
is basically thought of a last resort, we use AES-128-CBC over AES-256-CBC
since it has less encryption rounds and yields noticeable better
performance starting from a file size of just a few kB.

Signed-off-by: Daniel Walter 
[da...@sigma-star.at: addressed review comments]
Signed-off-by: David Gstir 
---
 fs/crypto/Kconfig  |   1 +
 fs/crypto/crypto.c |  23 --
 fs/crypto/fscrypt_private.h|   9 ++-
 fs/crypto/keyinfo.c| 171 -
 fs/crypto/policy.c |   8 +-
 include/linux/fscrypt_common.h |  16 ++--
 include/uapi/linux/fs.h|   2 +
 7 files changed, 172 insertions(+), 58 deletions(-)

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index 08b46e6e3995..02b7d91c9231 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -7,6 +7,7 @@ config FS_ENCRYPTION
select CRYPTO_XTS
select CRYPTO_CTS
select CRYPTO_CTR
+   select CRYPTO_SHA256
select KEYS
help
  Enable encryption of files and directories.  This
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6d6eca394d4d..c7835df7e7b8 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "fscrypt_private.h"
 
 static unsigned int num_prealloc_crypto_pages = 32;
@@ -147,8 +148,8 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 {
struct {
__le64 index;
-   u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
-   } xts_tweak;
+   u8 padding[FS_IV_SIZE - sizeof(__le64)];
+   } iv;
struct skcipher_request *req = NULL;
DECLARE_FS_COMPLETION_RESULT(ecr);
struct scatterlist dst, src;
@@ -158,6 +159,16 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 
BUG_ON(len == 0);
 
+   BUILD_BUG_ON(sizeof(iv) != FS_IV_SIZE);
+   BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
+   iv.index = cpu_to_le64(lblk_num);
+   memset(iv.padding, 0, sizeof(iv.padding));
+
+   if (ci->ci_essiv_tfm != NULL) {
+   crypto_cipher_encrypt_one(ci->ci_essiv_tfm, (u8 *),
+ (u8 *));
+   }
+
req = skcipher_request_alloc(tfm, gfp_flags);
if (!req) {
printk_ratelimited(KERN_ERR
@@ -170,15 +181,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
page_crypt_complete, );
 
-   BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
-   xts_tweak.index = cpu_to_le64(lblk_num);
-   memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
-
sg_init_table(, 1);
sg_set_page(, dest_page, len, offs);
sg_init_table(, 1);
sg_set_page(, src_page, len, offs);
-   skcipher_request_set_crypt(req, , , len, _tweak);
+   skcipher_request_set_crypt(req, , , len, );
if (rw == FS_DECRYPT)
res = crypto_skcipher_decrypt(req);
else
@@ -477,6 +484,8 @@ static void __exit fscrypt_exit(void)
destroy_workqueue(fscrypt_read_workqueue);
kmem_cache_destroy(fscrypt_ctx_cachep);
kmem_cache_destroy(fscrypt_info_cachep);
+
+   fscrypt_essiv_cleanup();
 }
 module_exit(fscrypt_exit);
 
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 1e1f8a361b75..a1d5021c31ef 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -12,10 +12,13 @@
 #define _FSCRYPT_PRIVATE_H
 
 #include 
+#include 
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE  16
+#define FS_IV_SIZE 16
 #define FS_AES_128_ECB_KEY_SIZE16
+#define FS_AES_128_CBC_KEY_SIZE16
+#define FS_AES_128_CTS_KEY_SIZE16
 #define FS_AES_256_GCM_KEY_SIZE   

Re: [PATCH v3] fscrypt: Add support for AES-128-CBC

[David Gstir]

[resend without the HTML crap - sorry about that!]

Hi Eric!

Thanks for the thorough review! :)

> On 17 May 2017, at 20:08, Eric Biggers <ebigge...@gmail.com> wrote:
> 
> Hi David, thanks for the update!
> 
> On Wed, May 17, 2017 at 01:21:04PM +0200, David Gstir wrote:
>> From: Daniel Walter <dwal...@sigma-star.at>
>> 
>> fscrypt provides facilities to use different encryption algorithms which
>> are selectable by userspace when setting the encryption policy. Currently,
>> only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
>> implemented. This is a clear case of kernel offers the mechanism and
>> userspace selects a policy. Similar to what dm-crypt and ecryptfs have.
>> 
>> This patch adds support for using AES-128-CBC for file contents and
>> AES-128-CBC-CTS for file name encryption. To mitigate watermarking
>> attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
>> actually slightly less secure than AES-XTS from a security point of view,
>> there is more widespread hardware support. Especially low-powered embedded
>> devices with crypto accelerators such as CAAM or CESA support only
>> AES-CBC-128 with an acceptable speed. Using AES-CBC gives us the acceptable
>> performance while still providing a moderate level of security for
>> persistent storage.
> 
> You covered this briefly in an email, but can you include more detail in the
> commit message on the reasoning behind choosing AES-128 instead of AES-256?
> Note that this is independent of the decision of CBC vs. XTS.

Sure, I'll extend the commit message to include that.


> 
>> @@ -129,27 +136,37 @@ static int determine_cipher_type(struct fscrypt_info 
>> *ci, struct inode *inode,
>>   const char **cipher_str_ret, int *keysize_ret)
>> {
>>  if (S_ISREG(inode->i_mode)) {
>> -if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_256_XTS) {
>> +switch (ci->ci_data_mode) {
>> +case FS_ENCRYPTION_MODE_AES_256_XTS:
>>  *cipher_str_ret = "xts(aes)";
>>  *keysize_ret = FS_AES_256_XTS_KEY_SIZE;
>>  return 0;
>> +case FS_ENCRYPTION_MODE_AES_128_CBC:
>> +*cipher_str_ret = "cbc(aes)";
>> +*keysize_ret = FS_AES_128_CBC_KEY_SIZE;
>> +return 0;
>> +default:
>> +pr_warn_once("fscrypto: unsupported contents encryption 
>> mode %d for inode %lu\n",
>> + ci->ci_data_mode, inode->i_ino);
>> +return -ENOKEY;
>>  }
>> -pr_warn_once("fscrypto: unsupported contents encryption mode "
>> - "%d for inode %lu\n",
>> - ci->ci_data_mode, inode->i_ino);
>> -return -ENOKEY;
>>  }
>> 
>>  if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
>> -if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_256_CTS) {
>> +switch (ci->ci_filename_mode) {
>> +case FS_ENCRYPTION_MODE_AES_256_CTS:
>>  *cipher_str_ret = "cts(cbc(aes))";
>>  *keysize_ret = FS_AES_256_CTS_KEY_SIZE;
>>  return 0;
>> +case FS_ENCRYPTION_MODE_AES_128_CTS:
>> +*cipher_str_ret = "cts(cbc(aes))";
>> +*keysize_ret = FS_AES_128_CTS_KEY_SIZE;
>> +return 0;
>> +default:
>> +pr_warn_once("fscrypto: unsupported filenames 
>> encryption mode %d for inode %lu\n",
>> + ci->ci_filename_mode, inode->i_ino);
>> +return -ENOKEY;
>>  }
>> -pr_warn_once("fscrypto: unsupported filenames encryption mode "
>> - "%d for inode %lu\n",
>> - ci->ci_filename_mode, inode->i_ino);
>> -return -ENOKEY;
>>  }
> 
> With the added call to fscrypt_valid_enc_modes() earlier, the warnings about
> unsupported encryption modes are no longer reachable.  IMO, the
> fscrypt_valid_enc_modes() check should be moved into this function, a proper
> warning message added for it, and the redundant warnings removed.  Also now 
> that
> there will be more modes I think it would be appropriate to put the algorithm
> names and ke

Re: [PATCH v3] fscrypt: Add support for AES-128-CBC

[David Gstir]

[resend without the HTML crap - sorry about that!]

Hi Eric!

Thanks for the thorough review! :)

> On 17 May 2017, at 20:08, Eric Biggers  wrote:
> 
> Hi David, thanks for the update!
> 
> On Wed, May 17, 2017 at 01:21:04PM +0200, David Gstir wrote:
>> From: Daniel Walter 
>> 
>> fscrypt provides facilities to use different encryption algorithms which
>> are selectable by userspace when setting the encryption policy. Currently,
>> only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
>> implemented. This is a clear case of kernel offers the mechanism and
>> userspace selects a policy. Similar to what dm-crypt and ecryptfs have.
>> 
>> This patch adds support for using AES-128-CBC for file contents and
>> AES-128-CBC-CTS for file name encryption. To mitigate watermarking
>> attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
>> actually slightly less secure than AES-XTS from a security point of view,
>> there is more widespread hardware support. Especially low-powered embedded
>> devices with crypto accelerators such as CAAM or CESA support only
>> AES-CBC-128 with an acceptable speed. Using AES-CBC gives us the acceptable
>> performance while still providing a moderate level of security for
>> persistent storage.
> 
> You covered this briefly in an email, but can you include more detail in the
> commit message on the reasoning behind choosing AES-128 instead of AES-256?
> Note that this is independent of the decision of CBC vs. XTS.

Sure, I'll extend the commit message to include that.


> 
>> @@ -129,27 +136,37 @@ static int determine_cipher_type(struct fscrypt_info 
>> *ci, struct inode *inode,
>>   const char **cipher_str_ret, int *keysize_ret)
>> {
>>  if (S_ISREG(inode->i_mode)) {
>> -if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_256_XTS) {
>> +switch (ci->ci_data_mode) {
>> +case FS_ENCRYPTION_MODE_AES_256_XTS:
>>  *cipher_str_ret = "xts(aes)";
>>  *keysize_ret = FS_AES_256_XTS_KEY_SIZE;
>>  return 0;
>> +case FS_ENCRYPTION_MODE_AES_128_CBC:
>> +*cipher_str_ret = "cbc(aes)";
>> +*keysize_ret = FS_AES_128_CBC_KEY_SIZE;
>> +return 0;
>> +default:
>> +pr_warn_once("fscrypto: unsupported contents encryption 
>> mode %d for inode %lu\n",
>> + ci->ci_data_mode, inode->i_ino);
>> +return -ENOKEY;
>>  }
>> -pr_warn_once("fscrypto: unsupported contents encryption mode "
>> - "%d for inode %lu\n",
>> - ci->ci_data_mode, inode->i_ino);
>> -return -ENOKEY;
>>  }
>> 
>>  if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
>> -if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_256_CTS) {
>> +switch (ci->ci_filename_mode) {
>> +case FS_ENCRYPTION_MODE_AES_256_CTS:
>>  *cipher_str_ret = "cts(cbc(aes))";
>>  *keysize_ret = FS_AES_256_CTS_KEY_SIZE;
>>  return 0;
>> +case FS_ENCRYPTION_MODE_AES_128_CTS:
>> +*cipher_str_ret = "cts(cbc(aes))";
>> +*keysize_ret = FS_AES_128_CTS_KEY_SIZE;
>> +return 0;
>> +default:
>> +pr_warn_once("fscrypto: unsupported filenames 
>> encryption mode %d for inode %lu\n",
>> + ci->ci_filename_mode, inode->i_ino);
>> +return -ENOKEY;
>>  }
>> -pr_warn_once("fscrypto: unsupported filenames encryption mode "
>> - "%d for inode %lu\n",
>> - ci->ci_filename_mode, inode->i_ino);
>> -return -ENOKEY;
>>  }
> 
> With the added call to fscrypt_valid_enc_modes() earlier, the warnings about
> unsupported encryption modes are no longer reachable.  IMO, the
> fscrypt_valid_enc_modes() check should be moved into this function, a proper
> warning message added for it, and the redundant warnings removed.  Also now 
> that
> there will be more modes I think it would be appropriate to put the algorithm
> names and key sizes in a table, to avoid the ugly switch statements.  

I

[PATCH] ubifs: Don't encrypt special files on creation

[David Gstir]

When a new inode is created, we check if the containing folder has a encryption
policy set and inherit that. This should however only be done for regular
files, links and subdirectories. Not for sockes fifos etc.

Fixes: d475a507457b ("ubifs: Add skeleton for fscrypto")
Cc: sta...@vger.kernel.org
Signed-off-by: David Gstir <da...@sigma-star.at>
---
 fs/ubifs/dir.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 566079d9b402..c67f6a3a606c 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -143,6 +143,7 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct 
inode *dir,
case S_IFBLK:
case S_IFCHR:
inode->i_op  = _file_inode_operations;
+   encrypted = false;
break;
default:
BUG();
-- 
2.12.0

[PATCH] ubifs: Don't encrypt special files on creation

[David Gstir]

When a new inode is created, we check if the containing folder has a encryption
policy set and inherit that. This should however only be done for regular
files, links and subdirectories. Not for sockes fifos etc.

Fixes: d475a507457b ("ubifs: Add skeleton for fscrypto")
Cc: sta...@vger.kernel.org
Signed-off-by: David Gstir 
---
 fs/ubifs/dir.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 566079d9b402..c67f6a3a606c 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -143,6 +143,7 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct 
inode *dir,
case S_IFBLK:
case S_IFCHR:
inode->i_op  = _file_inode_operations;
+   encrypted = false;
break;
default:
BUG();
-- 
2.12.0

[PATCH v3] fscrypt: Add support for AES-128-CBC

[David Gstir]

From: Daniel Walter <dwal...@sigma-star.at>

fscrypt provides facilities to use different encryption algorithms which
are selectable by userspace when setting the encryption policy. Currently,
only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
implemented. This is a clear case of kernel offers the mechanism and
userspace selects a policy. Similar to what dm-crypt and ecryptfs have.

This patch adds support for using AES-128-CBC for file contents and
AES-128-CBC-CTS for file name encryption. To mitigate watermarking
attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
actually slightly less secure than AES-XTS from a security point of view,
there is more widespread hardware support. Especially low-powered embedded
devices with crypto accelerators such as CAAM or CESA support only
AES-CBC-128 with an acceptable speed. Using AES-CBC gives us the acceptable
performance while still providing a moderate level of security for
persistent storage.

Signed-off-by: Daniel Walter <dwal...@sigma-star.at>
[da...@sigma-star.at: addressed review comments]
Signed-off-by: David Gstir <da...@sigma-star.at>
---
 fs/crypto/Kconfig  |   1 +
 fs/crypto/crypto.c |  23 +--
 fs/crypto/fscrypt_private.h|   9 ++-
 fs/crypto/keyinfo.c| 147 ++---
 fs/crypto/policy.c |   8 +--
 include/linux/fscrypt_common.h |  13 ++--
 include/uapi/linux/fs.h|   2 +
 7 files changed, 157 insertions(+), 46 deletions(-)

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index 08b46e6e3995..02b7d91c9231 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -7,6 +7,7 @@ config FS_ENCRYPTION
select CRYPTO_XTS
select CRYPTO_CTS
select CRYPTO_CTR
+   select CRYPTO_SHA256
select KEYS
help
  Enable encryption of files and directories.  This
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6d6eca394d4d..0d4582c3aef1 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "fscrypt_private.h"
 
 static unsigned int num_prealloc_crypto_pages = 32;
@@ -147,8 +148,8 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 {
struct {
__le64 index;
-   u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
-   } xts_tweak;
+   u8 padding[FS_IV_SIZE - sizeof(__le64)];
+   } iv;
struct skcipher_request *req = NULL;
DECLARE_FS_COMPLETION_RESULT(ecr);
struct scatterlist dst, src;
@@ -158,6 +159,16 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 
BUG_ON(len == 0);
 
+   BUILD_BUG_ON(sizeof(iv) != FS_IV_SIZE);
+   BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
+   iv.index = cpu_to_le64(lblk_num);
+   memset(iv.padding, 0, sizeof(iv.padding));
+
+   if (ci->ci_essiv_tfm != NULL) {
+   crypto_cipher_encrypt_one(ci->ci_essiv_tfm, (u8 *),
+ (u8 *));
+   }
+
req = skcipher_request_alloc(tfm, gfp_flags);
if (!req) {
printk_ratelimited(KERN_ERR
@@ -170,15 +181,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
page_crypt_complete, );
 
-   BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
-   xts_tweak.index = cpu_to_le64(lblk_num);
-   memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
-
sg_init_table(, 1);
sg_set_page(, dest_page, len, offs);
sg_init_table(, 1);
sg_set_page(, src_page, len, offs);
-   skcipher_request_set_crypt(req, , , len, _tweak);
+   skcipher_request_set_crypt(req, , , len, );
if (rw == FS_DECRYPT)
res = crypto_skcipher_decrypt(req);
else
@@ -388,6 +395,8 @@ static void fscrypt_destroy(void)
INIT_LIST_HEAD(_free_ctxs);
mempool_destroy(fscrypt_bounce_page_pool);
fscrypt_bounce_page_pool = NULL;
+
+   fscrypt_essiv_cleanup();
 }
 
 /**
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 1e1f8a361b75..68e605613352 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -12,10 +12,13 @@
 #define _FSCRYPT_PRIVATE_H
 
 #include 
+#include 
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE  16
+#define FS_IV_SIZE 16
 #define FS_AES_128_ECB_KEY_SIZE16
+#define FS_AES_128_CBC_KEY_SIZE16
+#define FS_AES_128_CTS_KEY_SIZE16
 #define FS_AES_256_GCM_KEY_SIZE32
 #define FS_AES_256_CBC_KEY_SIZE32
 #define FS_AES_256_CTS_KEY_SIZE32
@@ -54,6 +57,7 @@ struct fscrypt_info {
u8 ci_

[PATCH v3] fscrypt: Add support for AES-128-CBC

[David Gstir]

From: Daniel Walter 

fscrypt provides facilities to use different encryption algorithms which
are selectable by userspace when setting the encryption policy. Currently,
only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
implemented. This is a clear case of kernel offers the mechanism and
userspace selects a policy. Similar to what dm-crypt and ecryptfs have.

This patch adds support for using AES-128-CBC for file contents and
AES-128-CBC-CTS for file name encryption. To mitigate watermarking
attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
actually slightly less secure than AES-XTS from a security point of view,
there is more widespread hardware support. Especially low-powered embedded
devices with crypto accelerators such as CAAM or CESA support only
AES-CBC-128 with an acceptable speed. Using AES-CBC gives us the acceptable
performance while still providing a moderate level of security for
persistent storage.

Signed-off-by: Daniel Walter 
[da...@sigma-star.at: addressed review comments]
Signed-off-by: David Gstir 
---
 fs/crypto/Kconfig  |   1 +
 fs/crypto/crypto.c |  23 +--
 fs/crypto/fscrypt_private.h|   9 ++-
 fs/crypto/keyinfo.c| 147 ++---
 fs/crypto/policy.c |   8 +--
 include/linux/fscrypt_common.h |  13 ++--
 include/uapi/linux/fs.h|   2 +
 7 files changed, 157 insertions(+), 46 deletions(-)

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index 08b46e6e3995..02b7d91c9231 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -7,6 +7,7 @@ config FS_ENCRYPTION
select CRYPTO_XTS
select CRYPTO_CTS
select CRYPTO_CTR
+   select CRYPTO_SHA256
select KEYS
help
  Enable encryption of files and directories.  This
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6d6eca394d4d..0d4582c3aef1 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "fscrypt_private.h"
 
 static unsigned int num_prealloc_crypto_pages = 32;
@@ -147,8 +148,8 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 {
struct {
__le64 index;
-   u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
-   } xts_tweak;
+   u8 padding[FS_IV_SIZE - sizeof(__le64)];
+   } iv;
struct skcipher_request *req = NULL;
DECLARE_FS_COMPLETION_RESULT(ecr);
struct scatterlist dst, src;
@@ -158,6 +159,16 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 
BUG_ON(len == 0);
 
+   BUILD_BUG_ON(sizeof(iv) != FS_IV_SIZE);
+   BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
+   iv.index = cpu_to_le64(lblk_num);
+   memset(iv.padding, 0, sizeof(iv.padding));
+
+   if (ci->ci_essiv_tfm != NULL) {
+   crypto_cipher_encrypt_one(ci->ci_essiv_tfm, (u8 *),
+ (u8 *));
+   }
+
req = skcipher_request_alloc(tfm, gfp_flags);
if (!req) {
printk_ratelimited(KERN_ERR
@@ -170,15 +181,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
page_crypt_complete, );
 
-   BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
-   xts_tweak.index = cpu_to_le64(lblk_num);
-   memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
-
sg_init_table(, 1);
sg_set_page(, dest_page, len, offs);
sg_init_table(, 1);
sg_set_page(, src_page, len, offs);
-   skcipher_request_set_crypt(req, , , len, _tweak);
+   skcipher_request_set_crypt(req, , , len, );
if (rw == FS_DECRYPT)
res = crypto_skcipher_decrypt(req);
else
@@ -388,6 +395,8 @@ static void fscrypt_destroy(void)
INIT_LIST_HEAD(_free_ctxs);
mempool_destroy(fscrypt_bounce_page_pool);
fscrypt_bounce_page_pool = NULL;
+
+   fscrypt_essiv_cleanup();
 }
 
 /**
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 1e1f8a361b75..68e605613352 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -12,10 +12,13 @@
 #define _FSCRYPT_PRIVATE_H
 
 #include 
+#include 
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE  16
+#define FS_IV_SIZE 16
 #define FS_AES_128_ECB_KEY_SIZE16
+#define FS_AES_128_CBC_KEY_SIZE16
+#define FS_AES_128_CTS_KEY_SIZE16
 #define FS_AES_256_GCM_KEY_SIZE32
 #define FS_AES_256_CBC_KEY_SIZE32
 #define FS_AES_256_CTS_KEY_SIZE32
@@ -54,6 +57,7 @@ struct fscrypt_info {
u8 ci_filename_mode;
u8 ci_flags;
struct crypto_skcipher *ci_ctfm;
+   stru

Re: [PATCH v2] fscrypt: Add support for AES-128-CBC

[David Gstir]

Hi Eric!

Thanks for the feedback!

> On 25 Apr 2017, at 22:10, Eric Biggers <ebigge...@gmail.com> wrote:
> 
> Hi Daniel and David,
> 
> On Tue, Apr 25, 2017 at 04:41:00PM +0200, David Gstir wrote:
>> @@ -147,17 +148,28 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
>> fscrypt_direction_t rw,
>> {
>>  struct {
>>  __le64 index;
>> -u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
>> -} xts_tweak;
>> +u8 padding[FS_IV_SIZE - sizeof(__le64)];
>> +} blk_desc;
>>  struct skcipher_request *req = NULL;
>>  DECLARE_FS_COMPLETION_RESULT(ecr);
>>  struct scatterlist dst, src;
>>  struct fscrypt_info *ci = inode->i_crypt_info;
>>  struct crypto_skcipher *tfm = ci->ci_ctfm;
>>  int res = 0;
>> +u8 *iv = (u8 *)_desc;
>> 
>>  BUG_ON(len == 0);
>> 
>> +BUILD_BUG_ON(sizeof(blk_desc) != FS_IV_SIZE);
>> +BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
>> +blk_desc.index = cpu_to_le64(lblk_num);
>> +memset(blk_desc.padding, 0, sizeof(blk_desc.padding));
>> +
>> +if (ci->ci_essiv_tfm != NULL) {
>> +memset(iv, 0, sizeof(blk_desc));
>> +crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv, iv);
>> +}
>> +
>>  req = skcipher_request_alloc(tfm, gfp_flags);
>>  if (!req) {
>>  printk_ratelimited(KERN_ERR
>> @@ -170,15 +182,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
>> fscrypt_direction_t rw,
>>  req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
>>  page_crypt_complete, );
>> 
>> -BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
>> -xts_tweak.index = cpu_to_le64(lblk_num);
>> -memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
>> -
>>  sg_init_table(, 1);
>>  sg_set_page(, dest_page, len, offs);
>>  sg_init_table(, 1);
>>  sg_set_page(, src_page, len, offs);
>> -skcipher_request_set_crypt(req, , , len, _tweak);
>> +skcipher_request_set_crypt(req, , , len, );
>>  if (rw == FS_DECRYPT)
>>  res = crypto_skcipher_decrypt(req);
>>  else
> 
> There are two critical bugs here.  First the IV is being zeroed before being
> encrypted with the ESSIV tfm, so the resulting IV will always be the same for 
> a
> given file.  It should be encrypting the block number instead.  Second what's
> actually being passed into the crypto API is '' where IV is a pointer to
> something on the stack... I really doubt that does what's intended :)
> 
> Probably the cleanest way to do this correctly is to just have the struct be 
> the
> 'iv', so there's no extra pointer to deal with:
> 
>   struct {
>   __le64 index;
>   u8 padding[FS_IV_SIZE - sizeof(__le64)];
>   } iv;
> 
>   [...]
> 
>   BUILD_BUG_ON(sizeof(iv) != FS_IV_SIZE);
>   BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
>   iv.index = cpu_to_le64(lblk_num);
>   memset(iv.padding, 0, sizeof(iv.padding));
> 
>   if (ci->ci_essiv_tfm != NULL) {
>   crypto_cipher_encrypt_one(ci->ci_essiv_tfm, (u8 *),
> (u8 *));
>   }
> 
>   [...]
> 
>   skcipher_request_set_crypt(req, , , len, );

You are totally right. Somehow I completely missed that. :-/



> 
>> +static int derive_essiv_salt(u8 *raw_key, int keysize, u8 **salt_out,
>> +unsigned int *saltsize)
>> +{
>> +int res;
>> +struct crypto_ahash *hash_tfm;
>> +unsigned int digestsize;
>> +u8 *salt = NULL;
>> +struct scatterlist sg;
>> +struct ahash_request *req = NULL;
>> +
>> +hash_tfm = crypto_alloc_ahash("sha256", 0, 0);
>> +if (IS_ERR(hash_tfm))
>> +return PTR_ERR(hash_tfm);
>> +
>> +digestsize = crypto_ahash_digestsize(hash_tfm);
>> +salt = kzalloc(digestsize, GFP_NOFS);
>> +if (!salt) {
>> +res = -ENOMEM;
>> +goto out;
>> +}
>> +
>> +req = ahash_request_alloc(hash_tfm, GFP_NOFS);
>> +if (!req) {
>> +kfree(salt);
>> +res = -ENOMEM;
>> +goto out;
>> +}
>> +
>> +sg_init_one(, raw_key, keysize);
>> +ahash_request_set_callback(req,
>> +CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
>> +NULL, NULL);
>> +ahash_request_set_crypt(req, , salt, k

Re: [PATCH v2] fscrypt: Add support for AES-128-CBC

[David Gstir]

Hi Eric!

Thanks for the feedback!

> On 25 Apr 2017, at 22:10, Eric Biggers  wrote:
> 
> Hi Daniel and David,
> 
> On Tue, Apr 25, 2017 at 04:41:00PM +0200, David Gstir wrote:
>> @@ -147,17 +148,28 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
>> fscrypt_direction_t rw,
>> {
>>  struct {
>>  __le64 index;
>> -u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
>> -} xts_tweak;
>> +u8 padding[FS_IV_SIZE - sizeof(__le64)];
>> +} blk_desc;
>>  struct skcipher_request *req = NULL;
>>  DECLARE_FS_COMPLETION_RESULT(ecr);
>>  struct scatterlist dst, src;
>>  struct fscrypt_info *ci = inode->i_crypt_info;
>>  struct crypto_skcipher *tfm = ci->ci_ctfm;
>>  int res = 0;
>> +u8 *iv = (u8 *)_desc;
>> 
>>  BUG_ON(len == 0);
>> 
>> +BUILD_BUG_ON(sizeof(blk_desc) != FS_IV_SIZE);
>> +BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
>> +blk_desc.index = cpu_to_le64(lblk_num);
>> +memset(blk_desc.padding, 0, sizeof(blk_desc.padding));
>> +
>> +if (ci->ci_essiv_tfm != NULL) {
>> +memset(iv, 0, sizeof(blk_desc));
>> +crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv, iv);
>> +}
>> +
>>  req = skcipher_request_alloc(tfm, gfp_flags);
>>  if (!req) {
>>  printk_ratelimited(KERN_ERR
>> @@ -170,15 +182,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
>> fscrypt_direction_t rw,
>>  req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
>>  page_crypt_complete, );
>> 
>> -BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
>> -xts_tweak.index = cpu_to_le64(lblk_num);
>> -memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
>> -
>>  sg_init_table(, 1);
>>  sg_set_page(, dest_page, len, offs);
>>  sg_init_table(, 1);
>>  sg_set_page(, src_page, len, offs);
>> -skcipher_request_set_crypt(req, , , len, _tweak);
>> +skcipher_request_set_crypt(req, , , len, );
>>  if (rw == FS_DECRYPT)
>>  res = crypto_skcipher_decrypt(req);
>>  else
> 
> There are two critical bugs here.  First the IV is being zeroed before being
> encrypted with the ESSIV tfm, so the resulting IV will always be the same for 
> a
> given file.  It should be encrypting the block number instead.  Second what's
> actually being passed into the crypto API is '' where IV is a pointer to
> something on the stack... I really doubt that does what's intended :)
> 
> Probably the cleanest way to do this correctly is to just have the struct be 
> the
> 'iv', so there's no extra pointer to deal with:
> 
>   struct {
>   __le64 index;
>   u8 padding[FS_IV_SIZE - sizeof(__le64)];
>   } iv;
> 
>   [...]
> 
>   BUILD_BUG_ON(sizeof(iv) != FS_IV_SIZE);
>   BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
>   iv.index = cpu_to_le64(lblk_num);
>   memset(iv.padding, 0, sizeof(iv.padding));
> 
>   if (ci->ci_essiv_tfm != NULL) {
>   crypto_cipher_encrypt_one(ci->ci_essiv_tfm, (u8 *),
> (u8 *));
>   }
> 
>   [...]
> 
>   skcipher_request_set_crypt(req, , , len, );

You are totally right. Somehow I completely missed that. :-/



> 
>> +static int derive_essiv_salt(u8 *raw_key, int keysize, u8 **salt_out,
>> +unsigned int *saltsize)
>> +{
>> +int res;
>> +struct crypto_ahash *hash_tfm;
>> +unsigned int digestsize;
>> +u8 *salt = NULL;
>> +struct scatterlist sg;
>> +struct ahash_request *req = NULL;
>> +
>> +hash_tfm = crypto_alloc_ahash("sha256", 0, 0);
>> +if (IS_ERR(hash_tfm))
>> +return PTR_ERR(hash_tfm);
>> +
>> +digestsize = crypto_ahash_digestsize(hash_tfm);
>> +salt = kzalloc(digestsize, GFP_NOFS);
>> +if (!salt) {
>> +res = -ENOMEM;
>> +goto out;
>> +}
>> +
>> +req = ahash_request_alloc(hash_tfm, GFP_NOFS);
>> +if (!req) {
>> +kfree(salt);
>> +res = -ENOMEM;
>> +goto out;
>> +}
>> +
>> +sg_init_one(, raw_key, keysize);
>> +ahash_request_set_callback(req,
>> +CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
>> +NULL, NULL);
>> +ahash_request_set_crypt(req, , salt, keysize

[PATCH v2] fscrypt: Add support for AES-128-CBC

[David Gstir]

From: Daniel Walter <dwal...@sigma-star.at>

fscrypt provides facilities to use different encryption algorithms which
are selectable by userspace when setting the encryption policy. Currently,
only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
implemented. This is a clear case of kernel offers the mechanism and
userspace selects a policy. Similar to what dm-crypt and ecryptfs have.

This patch adds support for using AES-128-CBC for file contents and
AES-128-CBC-CTS for file name encryption. To mitigate watermarking
attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
actually slightly less secure than AES-XTS from a security point of view,
there is more widespread hardware support. Especially low-powered embedded
devices with crypto accelerators such as CAAM or CESA support only
AES-CBC-128 with an acceptable speed. Using AES-CBC gives us the acceptable
performance while still providing a moderate level of security for
persistent storage.

Signed-off-by: Daniel Walter <dwal...@sigma-star.at>
[da...@sigma-star.at: massaged commit message]
Signed-off-by: David Gstir <da...@sigma-star.at>
---
v2: Compute ESSIV salt using SHA256 instead of SHA1 and improve style
as pointed out by Eric Biggers [1].

[1] https://lkml.kernel.org/r/20170331062149.GA32409%20()%20zzz

 fs/crypto/crypto.c |  22 --
 fs/crypto/fscrypt_private.h|   5 +-
 fs/crypto/keyinfo.c| 165 ++---
 fs/crypto/policy.c |   7 +-
 include/linux/fscrypt_common.h |  13 ++--
 include/uapi/linux/fs.h|   2 +
 6 files changed, 169 insertions(+), 45 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6d6eca394d4d..9dd5f23a55a5 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "fscrypt_private.h"
 
 static unsigned int num_prealloc_crypto_pages = 32;
@@ -147,17 +148,28 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 {
struct {
__le64 index;
-   u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
-   } xts_tweak;
+   u8 padding[FS_IV_SIZE - sizeof(__le64)];
+   } blk_desc;
struct skcipher_request *req = NULL;
DECLARE_FS_COMPLETION_RESULT(ecr);
struct scatterlist dst, src;
struct fscrypt_info *ci = inode->i_crypt_info;
struct crypto_skcipher *tfm = ci->ci_ctfm;
int res = 0;
+   u8 *iv = (u8 *)_desc;
 
BUG_ON(len == 0);
 
+   BUILD_BUG_ON(sizeof(blk_desc) != FS_IV_SIZE);
+   BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
+   blk_desc.index = cpu_to_le64(lblk_num);
+   memset(blk_desc.padding, 0, sizeof(blk_desc.padding));
+
+   if (ci->ci_essiv_tfm != NULL) {
+   memset(iv, 0, sizeof(blk_desc));
+   crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv, iv);
+   }
+
req = skcipher_request_alloc(tfm, gfp_flags);
if (!req) {
printk_ratelimited(KERN_ERR
@@ -170,15 +182,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
page_crypt_complete, );
 
-   BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
-   xts_tweak.index = cpu_to_le64(lblk_num);
-   memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
-
sg_init_table(, 1);
sg_set_page(, dest_page, len, offs);
sg_init_table(, 1);
sg_set_page(, src_page, len, offs);
-   skcipher_request_set_crypt(req, , , len, _tweak);
+   skcipher_request_set_crypt(req, , , len, );
if (rw == FS_DECRYPT)
res = crypto_skcipher_decrypt(req);
else
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index e39696e64494..81ce9af449cb 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -16,8 +16,10 @@
 #define FS_FNAME_CRYPTO_DIGEST_SIZE32
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE  16
+#define FS_IV_SIZE 16
 #define FS_AES_128_ECB_KEY_SIZE16
+#define FS_AES_128_CBC_KEY_SIZE16
+#define FS_AES_128_CTS_KEY_SIZE16
 #define FS_AES_256_GCM_KEY_SIZE32
 #define FS_AES_256_CBC_KEY_SIZE32
 #define FS_AES_256_CTS_KEY_SIZE32
@@ -67,6 +69,7 @@ struct fscrypt_info {
u8 ci_filename_mode;
u8 ci_flags;
struct crypto_skcipher *ci_ctfm;
+   struct crypto_cipher *ci_essiv_tfm;
u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
 };
 
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 8cdfddce2b34..590efb0a891d 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -10,6 +10,8 @@
 
 #include 
 #include 
+#include 
+#include 
 #in

[PATCH v2] fscrypt: Add support for AES-128-CBC

[David Gstir]

From: Daniel Walter 

fscrypt provides facilities to use different encryption algorithms which
are selectable by userspace when setting the encryption policy. Currently,
only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
implemented. This is a clear case of kernel offers the mechanism and
userspace selects a policy. Similar to what dm-crypt and ecryptfs have.

This patch adds support for using AES-128-CBC for file contents and
AES-128-CBC-CTS for file name encryption. To mitigate watermarking
attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
actually slightly less secure than AES-XTS from a security point of view,
there is more widespread hardware support. Especially low-powered embedded
devices with crypto accelerators such as CAAM or CESA support only
AES-CBC-128 with an acceptable speed. Using AES-CBC gives us the acceptable
performance while still providing a moderate level of security for
persistent storage.

Signed-off-by: Daniel Walter 
[da...@sigma-star.at: massaged commit message]
Signed-off-by: David Gstir 
---
v2: Compute ESSIV salt using SHA256 instead of SHA1 and improve style
as pointed out by Eric Biggers [1].

[1] https://lkml.kernel.org/r/20170331062149.GA32409%20()%20zzz

 fs/crypto/crypto.c |  22 --
 fs/crypto/fscrypt_private.h|   5 +-
 fs/crypto/keyinfo.c| 165 ++---
 fs/crypto/policy.c |   7 +-
 include/linux/fscrypt_common.h |  13 ++--
 include/uapi/linux/fs.h|   2 +
 6 files changed, 169 insertions(+), 45 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6d6eca394d4d..9dd5f23a55a5 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "fscrypt_private.h"
 
 static unsigned int num_prealloc_crypto_pages = 32;
@@ -147,17 +148,28 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 {
struct {
__le64 index;
-   u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
-   } xts_tweak;
+   u8 padding[FS_IV_SIZE - sizeof(__le64)];
+   } blk_desc;
struct skcipher_request *req = NULL;
DECLARE_FS_COMPLETION_RESULT(ecr);
struct scatterlist dst, src;
struct fscrypt_info *ci = inode->i_crypt_info;
struct crypto_skcipher *tfm = ci->ci_ctfm;
int res = 0;
+   u8 *iv = (u8 *)_desc;
 
BUG_ON(len == 0);
 
+   BUILD_BUG_ON(sizeof(blk_desc) != FS_IV_SIZE);
+   BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
+   blk_desc.index = cpu_to_le64(lblk_num);
+   memset(blk_desc.padding, 0, sizeof(blk_desc.padding));
+
+   if (ci->ci_essiv_tfm != NULL) {
+   memset(iv, 0, sizeof(blk_desc));
+   crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv, iv);
+   }
+
req = skcipher_request_alloc(tfm, gfp_flags);
if (!req) {
printk_ratelimited(KERN_ERR
@@ -170,15 +182,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
page_crypt_complete, );
 
-   BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
-   xts_tweak.index = cpu_to_le64(lblk_num);
-   memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
-
sg_init_table(, 1);
sg_set_page(, dest_page, len, offs);
sg_init_table(, 1);
sg_set_page(, src_page, len, offs);
-   skcipher_request_set_crypt(req, , , len, _tweak);
+   skcipher_request_set_crypt(req, , , len, );
if (rw == FS_DECRYPT)
res = crypto_skcipher_decrypt(req);
else
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index e39696e64494..81ce9af449cb 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -16,8 +16,10 @@
 #define FS_FNAME_CRYPTO_DIGEST_SIZE32
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE  16
+#define FS_IV_SIZE 16
 #define FS_AES_128_ECB_KEY_SIZE16
+#define FS_AES_128_CBC_KEY_SIZE16
+#define FS_AES_128_CTS_KEY_SIZE16
 #define FS_AES_256_GCM_KEY_SIZE32
 #define FS_AES_256_CBC_KEY_SIZE32
 #define FS_AES_256_CTS_KEY_SIZE32
@@ -67,6 +69,7 @@ struct fscrypt_info {
u8 ci_filename_mode;
u8 ci_flags;
struct crypto_skcipher *ci_ctfm;
+   struct crypto_cipher *ci_essiv_tfm;
u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
 };
 
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 8cdfddce2b34..590efb0a891d 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -10,6 +10,8 @@
 
 #include 
 #include 
+#include 
+#include 
 #include "fscrypt_private.h"
 
 static void derive_crypt_complete(struct crypto_async

Re: [PATCH] fscrypt: Add support for AES-128-CBC

[David Gstir]

Hi Eric,

thanks for the feedback!

> On 31.03.2017, at 08:21, Eric Biggers <ebigge...@gmail.com> wrote:
> 
> [+Cc linux-fscrypt]

Oh, I didn't know about that list. I think MAINTAINERS should be updated to 
reflect that. :)

> 
> Hi David and Daniel,
> 
> On Thu, Mar 30, 2017 at 07:38:40PM +0200, David Gstir wrote:
>> From: Daniel Walter <dwal...@sigma-star.at>
>> 
>> fscrypt provides facilities to use different encryption algorithms which are
>> selectable by userspace when setting the encryption policy. Currently, only
>> AES-256-XTS for file contents and AES-256-CBC-CTS for file names are 
>> implemented.
>> Which is a clear case of kernel offers the mechanism and userspace selects a
>> policy. Similar to what dm-crypt and ecryptfs have.
>> 
>> This patch adds support for using AES-128-CBC for file contents and
>> AES-128-CBC-CTS for file name encryption. To mitigate watermarking attacks, 
>> IVs
>> are generated using the ESSIV algorithm. While AES-CBC is actually slightly
>> less secure than AES-XTS from a security point of view, there is more
>> widespread hardware support. Especially low-powered embedded devices crypto
>> accelerators such as CAAM or CESA support only AES-CBC-128 with an acceptable
>> speed. Using AES-CBC gives us the acceptable performance while still 
>> providing
>> a moderate level of security for persistent storage.
>> 
> 
> Thanks for sending this!  I can't object too much to adding AES-CBC-128 if you
> find it useful, though of course AES-256-XTS will remain the recommendation 
> for
> general use.  

Yes, AES-256-XTS should definitely be the recommendation and default here!
AES-128-CBC is a last resort if XTS is not possible for whatever reason.


> And I don't suppose AES-256-CBC is an option for you?

We went for AES-128 since it has less rounds and yields better performance. At 
least on the hardware we looked at, there was quite a difference in speed 
between AES-128-CBC and AES-256-CBC.

Anyways, AES-256-CBC could be added with just a few lines after this patch. :)


> Anyway, more comments below:

[...]

>> +if (ctx.contents_encryption_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
>> +ctx.filenames_encryption_mode != FS_ENCRYPTION_MODE_AES_128_CTS)
>> +return -EINVAL;
>> +
> 
> I think for now we should only allow the two pairs:
> 
>   contents_encryption_mode=FS_ENCRYPTION_MODE_AES_256_XTS
>   filenames_encryption_mode=FS_ENCRYPTION_MODE_AES_256_CTS
> 
> and
> 
>   contents_encryption_mode=FS_ENCRYPTION_MODE_AES_128_CBC
>   filenames_encryption_mode=FS_ENCRYPTION_MODE_AES_128_CTS
> 
> Other combinations like AES-256-XTS paired with AES-128-CTS should be 
> forbidden.

Yes, I agree.


> This also needs to be enforced in create_encryption_context_from_policy() so
> that FS_IOC_SET_ENCRYPTION_POLICY fails with bad combinations.
> 
>> +if (crypt_info->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
>> +/* init ESSIV generator */
>> +essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
>> +if (!essiv_tfm || IS_ERR(essiv_tfm)) {
>> +res = essiv_tfm ? PTR_ERR(essiv_tfm) : -ENOMEM;
>> +printk(KERN_DEBUG
>> +   "%s: error %d (inode %u) allocating essiv tfm\n",
>> +   __func__, res, (unsigned) inode->i_ino);
>> +goto out;
>> +}
>> +/* calc sha of key for essiv generation */
>> +memset(sha_ws, 0, sizeof(sha_ws));
>> +sha_init(essiv_key);
>> +sha_transform(essiv_key, raw_key, sha_ws);
>> +res = crypto_cipher_setkey(essiv_tfm, (u8 *)essiv_key, keysize);
>> +if (res)
>> +goto out;
>> +
>> +crypt_info->ci_essiv_tfm = essiv_tfm;
>> +}
> 
> I think the ESSIV hash should be SHA-256 not SHA-1.  SHA-1 is becoming more 
> and
> more obsolete these days.  Another issue with SHA-1 is that it only produces a
> 20 byte hash, which means it couldn't be used if someone ever wanted to add
> AES-256-CBC as another mode.

Good point! We'll change this to always use sha-256.


I'll wait for some more feedback and will provide a v2 which includes all your 
comments.

Thanks,
David

Re: [PATCH] fscrypt: Add support for AES-128-CBC

[David Gstir]

Hi Eric,

thanks for the feedback!

> On 31.03.2017, at 08:21, Eric Biggers  wrote:
> 
> [+Cc linux-fscrypt]

Oh, I didn't know about that list. I think MAINTAINERS should be updated to 
reflect that. :)

> 
> Hi David and Daniel,
> 
> On Thu, Mar 30, 2017 at 07:38:40PM +0200, David Gstir wrote:
>> From: Daniel Walter 
>> 
>> fscrypt provides facilities to use different encryption algorithms which are
>> selectable by userspace when setting the encryption policy. Currently, only
>> AES-256-XTS for file contents and AES-256-CBC-CTS for file names are 
>> implemented.
>> Which is a clear case of kernel offers the mechanism and userspace selects a
>> policy. Similar to what dm-crypt and ecryptfs have.
>> 
>> This patch adds support for using AES-128-CBC for file contents and
>> AES-128-CBC-CTS for file name encryption. To mitigate watermarking attacks, 
>> IVs
>> are generated using the ESSIV algorithm. While AES-CBC is actually slightly
>> less secure than AES-XTS from a security point of view, there is more
>> widespread hardware support. Especially low-powered embedded devices crypto
>> accelerators such as CAAM or CESA support only AES-CBC-128 with an acceptable
>> speed. Using AES-CBC gives us the acceptable performance while still 
>> providing
>> a moderate level of security for persistent storage.
>> 
> 
> Thanks for sending this!  I can't object too much to adding AES-CBC-128 if you
> find it useful, though of course AES-256-XTS will remain the recommendation 
> for
> general use.  

Yes, AES-256-XTS should definitely be the recommendation and default here!
AES-128-CBC is a last resort if XTS is not possible for whatever reason.


> And I don't suppose AES-256-CBC is an option for you?

We went for AES-128 since it has less rounds and yields better performance. At 
least on the hardware we looked at, there was quite a difference in speed 
between AES-128-CBC and AES-256-CBC.

Anyways, AES-256-CBC could be added with just a few lines after this patch. :)


> Anyway, more comments below:

[...]

>> +if (ctx.contents_encryption_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
>> +ctx.filenames_encryption_mode != FS_ENCRYPTION_MODE_AES_128_CTS)
>> +return -EINVAL;
>> +
> 
> I think for now we should only allow the two pairs:
> 
>   contents_encryption_mode=FS_ENCRYPTION_MODE_AES_256_XTS
>   filenames_encryption_mode=FS_ENCRYPTION_MODE_AES_256_CTS
> 
> and
> 
>   contents_encryption_mode=FS_ENCRYPTION_MODE_AES_128_CBC
>   filenames_encryption_mode=FS_ENCRYPTION_MODE_AES_128_CTS
> 
> Other combinations like AES-256-XTS paired with AES-128-CTS should be 
> forbidden.

Yes, I agree.


> This also needs to be enforced in create_encryption_context_from_policy() so
> that FS_IOC_SET_ENCRYPTION_POLICY fails with bad combinations.
> 
>> +if (crypt_info->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
>> +/* init ESSIV generator */
>> +essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
>> +if (!essiv_tfm || IS_ERR(essiv_tfm)) {
>> +res = essiv_tfm ? PTR_ERR(essiv_tfm) : -ENOMEM;
>> +printk(KERN_DEBUG
>> +   "%s: error %d (inode %u) allocating essiv tfm\n",
>> +   __func__, res, (unsigned) inode->i_ino);
>> +goto out;
>> +}
>> +/* calc sha of key for essiv generation */
>> +memset(sha_ws, 0, sizeof(sha_ws));
>> +sha_init(essiv_key);
>> +sha_transform(essiv_key, raw_key, sha_ws);
>> +res = crypto_cipher_setkey(essiv_tfm, (u8 *)essiv_key, keysize);
>> +if (res)
>> +goto out;
>> +
>> +crypt_info->ci_essiv_tfm = essiv_tfm;
>> +}
> 
> I think the ESSIV hash should be SHA-256 not SHA-1.  SHA-1 is becoming more 
> and
> more obsolete these days.  Another issue with SHA-1 is that it only produces a
> 20 byte hash, which means it couldn't be used if someone ever wanted to add
> AES-256-CBC as another mode.

Good point! We'll change this to always use sha-256.


I'll wait for some more feedback and will provide a v2 which includes all your 
comments.

Thanks,
David

[PATCH] fscrypt: Add support for AES-128-CBC

[David Gstir]

From: Daniel Walter <dwal...@sigma-star.at>

fscrypt provides facilities to use different encryption algorithms which are
selectable by userspace when setting the encryption policy. Currently, only
AES-256-XTS for file contents and AES-256-CBC-CTS for file names are 
implemented.
Which is a clear case of kernel offers the mechanism and userspace selects a
policy. Similar to what dm-crypt and ecryptfs have.

This patch adds support for using AES-128-CBC for file contents and
AES-128-CBC-CTS for file name encryption. To mitigate watermarking attacks, IVs
are generated using the ESSIV algorithm. While AES-CBC is actually slightly
less secure than AES-XTS from a security point of view, there is more
widespread hardware support. Especially low-powered embedded devices crypto
accelerators such as CAAM or CESA support only AES-CBC-128 with an acceptable
speed. Using AES-CBC gives us the acceptable performance while still providing
a moderate level of security for persistent storage.

[david: massaged commit message]

Signed-off-by: Daniel Walter <dwal...@sigma-star.at>
Signed-off-by: David Gstir <da...@sigma-star.at>
---
 fs/crypto/crypto.c | 25 
 fs/crypto/fscrypt_private.h|  5 ++-
 fs/crypto/keyinfo.c| 87 --
 include/linux/fscrypt_common.h |  6 ++-
 include/uapi/linux/fs.h|  2 +
 5 files changed, 103 insertions(+), 22 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6d6eca3..210b586 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "fscrypt_private.h"
 
 static unsigned int num_prealloc_crypto_pages = 32;
@@ -147,17 +148,31 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 {
struct {
__le64 index;
-   u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
-   } xts_tweak;
+   u8 padding[FS_IV_SIZE - sizeof(__le64)];
+   } blk_desc;
struct skcipher_request *req = NULL;
DECLARE_FS_COMPLETION_RESULT(ecr);
struct scatterlist dst, src;
struct fscrypt_info *ci = inode->i_crypt_info;
struct crypto_skcipher *tfm = ci->ci_ctfm;
+   u8 iv[FS_IV_SIZE];
int res = 0;
 
BUG_ON(len == 0);
 
+   BUILD_BUG_ON(sizeof(blk_desc) != FS_IV_SIZE);
+   BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
+   blk_desc.index = cpu_to_le64(lblk_num);
+   memset(blk_desc.padding, 0, sizeof(blk_desc.padding));
+
+   if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
+   BUG_ON(ci->ci_essiv_tfm == NULL);
+   memset(iv, 0, sizeof(iv));
+   crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv, (u8 
*)_desc);
+   } else {
+   memcpy(iv, _desc, sizeof(iv));
+   }
+
req = skcipher_request_alloc(tfm, gfp_flags);
if (!req) {
printk_ratelimited(KERN_ERR
@@ -170,15 +185,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
page_crypt_complete, );
 
-   BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
-   xts_tweak.index = cpu_to_le64(lblk_num);
-   memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
-
sg_init_table(, 1);
sg_set_page(, dest_page, len, offs);
sg_init_table(, 1);
sg_set_page(, src_page, len, offs);
-   skcipher_request_set_crypt(req, , , len, _tweak);
+   skcipher_request_set_crypt(req, , , len, );
if (rw == FS_DECRYPT)
res = crypto_skcipher_decrypt(req);
else
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index e39696e..81ce9af 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -16,8 +16,10 @@
 #define FS_FNAME_CRYPTO_DIGEST_SIZE32
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE  16
+#define FS_IV_SIZE 16
 #define FS_AES_128_ECB_KEY_SIZE16
+#define FS_AES_128_CBC_KEY_SIZE16
+#define FS_AES_128_CTS_KEY_SIZE16
 #define FS_AES_256_GCM_KEY_SIZE32
 #define FS_AES_256_CBC_KEY_SIZE32
 #define FS_AES_256_CTS_KEY_SIZE32
@@ -67,6 +69,7 @@ struct fscrypt_info {
u8 ci_filename_mode;
u8 ci_flags;
struct crypto_skcipher *ci_ctfm;
+   struct crypto_cipher *ci_essiv_tfm;
u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
 };
 
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 8cdfddc..cc515e9 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -10,6 +10,7 @@
 
 #include 
 #include 
+#include 
 #include "fscrypt_private.h"
 
 static void derive_crypt_complete(struct crypto_async_request *req, int rc)
@@ -27,

[PATCH] fscrypt: Add support for AES-128-CBC

[David Gstir]

From: Daniel Walter 

fscrypt provides facilities to use different encryption algorithms which are
selectable by userspace when setting the encryption policy. Currently, only
AES-256-XTS for file contents and AES-256-CBC-CTS for file names are 
implemented.
Which is a clear case of kernel offers the mechanism and userspace selects a
policy. Similar to what dm-crypt and ecryptfs have.

This patch adds support for using AES-128-CBC for file contents and
AES-128-CBC-CTS for file name encryption. To mitigate watermarking attacks, IVs
are generated using the ESSIV algorithm. While AES-CBC is actually slightly
less secure than AES-XTS from a security point of view, there is more
widespread hardware support. Especially low-powered embedded devices crypto
accelerators such as CAAM or CESA support only AES-CBC-128 with an acceptable
speed. Using AES-CBC gives us the acceptable performance while still providing
a moderate level of security for persistent storage.

[david: massaged commit message]

Signed-off-by: Daniel Walter 
Signed-off-by: David Gstir 
---
 fs/crypto/crypto.c | 25 
 fs/crypto/fscrypt_private.h|  5 ++-
 fs/crypto/keyinfo.c| 87 --
 include/linux/fscrypt_common.h |  6 ++-
 include/uapi/linux/fs.h|  2 +
 5 files changed, 103 insertions(+), 22 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6d6eca3..210b586 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "fscrypt_private.h"
 
 static unsigned int num_prealloc_crypto_pages = 32;
@@ -147,17 +148,31 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
 {
struct {
__le64 index;
-   u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
-   } xts_tweak;
+   u8 padding[FS_IV_SIZE - sizeof(__le64)];
+   } blk_desc;
struct skcipher_request *req = NULL;
DECLARE_FS_COMPLETION_RESULT(ecr);
struct scatterlist dst, src;
struct fscrypt_info *ci = inode->i_crypt_info;
struct crypto_skcipher *tfm = ci->ci_ctfm;
+   u8 iv[FS_IV_SIZE];
int res = 0;
 
BUG_ON(len == 0);
 
+   BUILD_BUG_ON(sizeof(blk_desc) != FS_IV_SIZE);
+   BUILD_BUG_ON(AES_BLOCK_SIZE != FS_IV_SIZE);
+   blk_desc.index = cpu_to_le64(lblk_num);
+   memset(blk_desc.padding, 0, sizeof(blk_desc.padding));
+
+   if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
+   BUG_ON(ci->ci_essiv_tfm == NULL);
+   memset(iv, 0, sizeof(iv));
+   crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv, (u8 
*)_desc);
+   } else {
+   memcpy(iv, _desc, sizeof(iv));
+   }
+
req = skcipher_request_alloc(tfm, gfp_flags);
if (!req) {
printk_ratelimited(KERN_ERR
@@ -170,15 +185,11 @@ int fscrypt_do_page_crypto(const struct inode *inode, 
fscrypt_direction_t rw,
req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
page_crypt_complete, );
 
-   BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
-   xts_tweak.index = cpu_to_le64(lblk_num);
-   memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
-
sg_init_table(, 1);
sg_set_page(, dest_page, len, offs);
sg_init_table(, 1);
sg_set_page(, src_page, len, offs);
-   skcipher_request_set_crypt(req, , , len, _tweak);
+   skcipher_request_set_crypt(req, , , len, );
if (rw == FS_DECRYPT)
res = crypto_skcipher_decrypt(req);
else
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index e39696e..81ce9af 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -16,8 +16,10 @@
 #define FS_FNAME_CRYPTO_DIGEST_SIZE32
 
 /* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE  16
+#define FS_IV_SIZE 16
 #define FS_AES_128_ECB_KEY_SIZE16
+#define FS_AES_128_CBC_KEY_SIZE16
+#define FS_AES_128_CTS_KEY_SIZE16
 #define FS_AES_256_GCM_KEY_SIZE32
 #define FS_AES_256_CBC_KEY_SIZE32
 #define FS_AES_256_CTS_KEY_SIZE32
@@ -67,6 +69,7 @@ struct fscrypt_info {
u8 ci_filename_mode;
u8 ci_flags;
struct crypto_skcipher *ci_ctfm;
+   struct crypto_cipher *ci_essiv_tfm;
u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
 };
 
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 8cdfddc..cc515e9 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -10,6 +10,7 @@
 
 #include 
 #include 
+#include 
 #include "fscrypt_private.h"
 
 static void derive_crypt_complete(struct crypto_async_request *req, int rc)
@@ -27,13 +28,13 @@ static void derive_crypt_complete(struct 
crypto_async_request *req, int rc)
  * derive

Re: Geode LX AES/RNG driver triggers warning

[David Gstir]

PrasannaKumar,

> On 06.01.2017, at 10:40, PrasannaKumar Muralidharan 
>  wrote:
> 
>>> I narrowed it down to commit 6e9b5e76882c ("hwrng: geode - Migrate to 
>>> managed API") which seems to introduce this. It looks to me like some issue 
>>> between devres, the Geode hwrng and AES drivers which both use the same PCI 
>>> device.
>> 
>> It does
>> 
>>> I'm no expert here, but I curious if this will cause any issues when using 
>>> the hardware crypto drivers and also what's the best way to get rid of this?
>> 
>> Probably to create an mfd device that turns the PCI device into two MFD
>> devices and bind AES and hwrng one to each MFD device. Take a look in
>> drivers/mfd. That would also fix the uglies in mod_init for the rng
>> driver.
> 
> I am the author of that commit. Code before commit 6e9b5e76882c had
> ioremap, wondering why there was no warning message before. Just want
> to know if that commit uncovered existing issue or introduced a new
> issue? As far as I understand the commit did not change the
> functionality. Please feel free to correct if I have missed something.

This warning is because commit 6e9b5e76882c introduces devres (devm_ioremap vs 
ioremap). As soon as I have some spare time, I'll look into resolving this via 
MFD like Alan suggested.

Thanks,
-David

Re: Geode LX AES/RNG driver triggers warning

[David Gstir]

PrasannaKumar,

> On 06.01.2017, at 10:40, PrasannaKumar Muralidharan 
>  wrote:
> 
>>> I narrowed it down to commit 6e9b5e76882c ("hwrng: geode - Migrate to 
>>> managed API") which seems to introduce this. It looks to me like some issue 
>>> between devres, the Geode hwrng and AES drivers which both use the same PCI 
>>> device.
>> 
>> It does
>> 
>>> I'm no expert here, but I curious if this will cause any issues when using 
>>> the hardware crypto drivers and also what's the best way to get rid of this?
>> 
>> Probably to create an mfd device that turns the PCI device into two MFD
>> devices and bind AES and hwrng one to each MFD device. Take a look in
>> drivers/mfd. That would also fix the uglies in mod_init for the rng
>> driver.
> 
> I am the author of that commit. Code before commit 6e9b5e76882c had
> ioremap, wondering why there was no warning message before. Just want
> to know if that commit uncovered existing issue or introduced a new
> issue? As far as I understand the commit did not change the
> functionality. Please feel free to correct if I have missed something.

This warning is because commit 6e9b5e76882c introduces devres (devm_ioremap vs 
ioremap). As soon as I have some spare time, I'll look into resolving this via 
MFD like Alan suggested.

Thanks,
-David

Re: [PATCH] clockevents/drivers/cs5535: Un-break driver with 'set-state' interface

[David Gstir]

Hi Daniel,

> On 04.01.2017, at 15:19, Daniel Lezcano  wrote:
> 
> On 02/01/2017 10:34, Viresh Kumar wrote:
> 
> [ ... ]
> 
>>> --- a/drivers/clocksource/cs5535-clockevt.c
>>> +++ b/drivers/clocksource/cs5535-clockevt.c
>>> @@ -117,7 +117,8 @@ static irqreturn_t mfgpt_tick(int irq, void *dev_id)
>>> /* Turn off the clock (and clear the event) */
>>> disable_timer(cs5535_event_clock);
>>> 
>>> -   if (clockevent_state_shutdown(_clockevent))
>>> +   if (clockevent_state_shutdown(_clockevent) ||
>>> +   clockevent_state_detached(_clockevent))
>>> return IRQ_HANDLED;
>>> 
>>> /* Clear the counter */
>> 
>> Sorry for breaking it, but it looks we have unearthed a bug because of
>> my patch.
>> 
>> I wouldn't enable the IRQ unless the clockevents core has asked for
>> it, i.e. by calling set_state_periodic() or set_state_oneshot().
>> 
>> The driver is currently enabling the IRQ from its init code and that's
>> where the problem is IMHO.
> 
> Hi David,
> 
> can you look at this ?

Yes, sure! I’ll get back to you with a new patch.

- David

Re: [PATCH] clockevents/drivers/cs5535: Un-break driver with 'set-state' interface

[David Gstir]

Hi Daniel,

> On 04.01.2017, at 15:19, Daniel Lezcano  wrote:
> 
> On 02/01/2017 10:34, Viresh Kumar wrote:
> 
> [ ... ]
> 
>>> --- a/drivers/clocksource/cs5535-clockevt.c
>>> +++ b/drivers/clocksource/cs5535-clockevt.c
>>> @@ -117,7 +117,8 @@ static irqreturn_t mfgpt_tick(int irq, void *dev_id)
>>> /* Turn off the clock (and clear the event) */
>>> disable_timer(cs5535_event_clock);
>>> 
>>> -   if (clockevent_state_shutdown(_clockevent))
>>> +   if (clockevent_state_shutdown(_clockevent) ||
>>> +   clockevent_state_detached(_clockevent))
>>> return IRQ_HANDLED;
>>> 
>>> /* Clear the counter */
>> 
>> Sorry for breaking it, but it looks we have unearthed a bug because of
>> my patch.
>> 
>> I wouldn't enable the IRQ unless the clockevents core has asked for
>> it, i.e. by calling set_state_periodic() or set_state_oneshot().
>> 
>> The driver is currently enabling the IRQ from its init code and that's
>> where the problem is IMHO.
> 
> Hi David,
> 
> can you look at this ?

Yes, sure! I’ll get back to you with a new patch.

- David

Geode LX AES/RNG driver triggers warning

[David Gstir]

Hi!

I recently tested kernel v4.9 on my AMD Geode platform and noticed that its AES 
hardware driver triggers this warning on initialization:

[1.265708] [ cut here ]
[1.267932] WARNING: CPU: 0 PID: 1 at drivers/base/dd.c:344 
driver_probe_device+0x5d/0x1ad
[1.272427] CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.0 #2
[1.277416]  cf82be70 c1153046 cf82be8c c102a98b 0158 c11ad3ab cf8b9868 
c14342b4
[1.297179]  c14342b4 cf82bea0 c102aa0e 0009   cf82beb8 
c11ad3ab
[1.316912]   cf8b9868 c14342b4 cf8b989c cf82becc c11ad553  
c14342b4
[1.336645] Call Trace:
[1.340044]  [] dump_stack+0x16/0x18
[1.345423]  [] __warn+0xa0/0xb7
[1.349743]  [] ? driver_probe_device+0x5d/0x1ad
[1.354224]  [] warn_slowpath_null+0x11/0x16
[1.357663]  [] driver_probe_device+0x5d/0x1ad
[1.361621]  [] __driver_attach+0x58/0x74
[1.364282]  [] bus_for_each_dev+0x4b/0x68
[1.367202]  [] driver_attach+0x14/0x16
[1.373345]  [] ? driver_probe_device+0x1ad/0x1ad
[1.378087]  [] bus_add_driver+0xaf/0x181
[1.380750]  [] ? firmware_map_add_early+0xaa/0xaa
[1.385746]  [] driver_register+0x6f/0xa4
[1.388405]  [] ? firmware_map_add_early+0xaa/0xaa
[1.393414]  [] __pci_register_driver+0x27/0x2a
[1.397628]  [] geode_aes_driver_init+0x14/0x16
[1.401846]  [] do_one_initcall+0x7c/0xec
[1.404516]  [] ? parse_args+0x1c3/0x283
[1.410913]  [] ? kernel_init_freeable+0xba/0x157
[1.411646]  [] kernel_init_freeable+0xda/0x157
[1.415872]  [] ? rest_init+0x59/0x59
[1.421489]  [] kernel_init+0x8/0xcb
[1.426856]  [] ret_from_fork+0x1b/0x28
[1.428999] ---[ end trace 24dfe638898c8e1f ]---

I narrowed it down to commit 6e9b5e76882c ("hwrng: geode - Migrate to managed 
API") which seems to introduce this. It looks to me like some issue between 
devres, the Geode hwrng and AES drivers which both use the same PCI device.

I'm no expert here, but I curious if this will cause any issues when using the 
hardware crypto drivers and also what's the best way to get rid of this?

Thanks,
David

Geode LX AES/RNG driver triggers warning

[David Gstir]

Hi!

I recently tested kernel v4.9 on my AMD Geode platform and noticed that its AES 
hardware driver triggers this warning on initialization:

[1.265708] [ cut here ]
[1.267932] WARNING: CPU: 0 PID: 1 at drivers/base/dd.c:344 
driver_probe_device+0x5d/0x1ad
[1.272427] CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.0 #2
[1.277416]  cf82be70 c1153046 cf82be8c c102a98b 0158 c11ad3ab cf8b9868 
c14342b4
[1.297179]  c14342b4 cf82bea0 c102aa0e 0009   cf82beb8 
c11ad3ab
[1.316912]   cf8b9868 c14342b4 cf8b989c cf82becc c11ad553  
c14342b4
[1.336645] Call Trace:
[1.340044]  [] dump_stack+0x16/0x18
[1.345423]  [] __warn+0xa0/0xb7
[1.349743]  [] ? driver_probe_device+0x5d/0x1ad
[1.354224]  [] warn_slowpath_null+0x11/0x16
[1.357663]  [] driver_probe_device+0x5d/0x1ad
[1.361621]  [] __driver_attach+0x58/0x74
[1.364282]  [] bus_for_each_dev+0x4b/0x68
[1.367202]  [] driver_attach+0x14/0x16
[1.373345]  [] ? driver_probe_device+0x1ad/0x1ad
[1.378087]  [] bus_add_driver+0xaf/0x181
[1.380750]  [] ? firmware_map_add_early+0xaa/0xaa
[1.385746]  [] driver_register+0x6f/0xa4
[1.388405]  [] ? firmware_map_add_early+0xaa/0xaa
[1.393414]  [] __pci_register_driver+0x27/0x2a
[1.397628]  [] geode_aes_driver_init+0x14/0x16
[1.401846]  [] do_one_initcall+0x7c/0xec
[1.404516]  [] ? parse_args+0x1c3/0x283
[1.410913]  [] ? kernel_init_freeable+0xba/0x157
[1.411646]  [] kernel_init_freeable+0xda/0x157
[1.415872]  [] ? rest_init+0x59/0x59
[1.421489]  [] kernel_init+0x8/0xcb
[1.426856]  [] ret_from_fork+0x1b/0x28
[1.428999] ---[ end trace 24dfe638898c8e1f ]---

I narrowed it down to commit 6e9b5e76882c ("hwrng: geode - Migrate to managed 
API") which seems to introduce this. It looks to me like some issue between 
devres, the Geode hwrng and AES drivers which both use the same PCI device.

I'm no expert here, but I curious if this will cause any issues when using the 
hardware crypto drivers and also what's the best way to get rid of this?

Thanks,
David

[PATCH] clockevents/drivers/cs5535: Un-break driver with 'set-state' interface

[David Gstir]

Since migrating to the 'set-state' interface the cs5535 driver causes a
crash after loading: Right after initialization, the IRQ handler
(mfgpt_tick) is triggered with clock event device in detached state. This
state not properly handled and causes a crash through NULL pointer
dereference upon calling the clockevent's event_handler.

This patch fixes this by handling the detached state the same way the
shutdown state is handled.

Fixes: 8f9327cbb6e8 ("clockevents/drivers/cs5535: Migrate to new 'set-state' 
interface")
Cc: sta...@vger.kernel.org
Cc: Andres Salomon <dilin...@queued.net>
Cc: Viresh Kumar <viresh.ku...@linaro.org>
Signed-off-by: David Gstir <da...@sigma-star.at>
---
 drivers/clocksource/cs5535-clockevt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/cs5535-clockevt.c 
b/drivers/clocksource/cs5535-clockevt.c
index 9a7e37cf56b0..649e0cd90805 100644
--- a/drivers/clocksource/cs5535-clockevt.c
+++ b/drivers/clocksource/cs5535-clockevt.c
@@ -117,7 +117,8 @@ static irqreturn_t mfgpt_tick(int irq, void *dev_id)
/* Turn off the clock (and clear the event) */
disable_timer(cs5535_event_clock);
 
-   if (clockevent_state_shutdown(_clockevent))
+   if (clockevent_state_shutdown(_clockevent) ||
+   clockevent_state_detached(_clockevent))
return IRQ_HANDLED;
 
/* Clear the counter */
-- 
2.11.0

[PATCH] clockevents/drivers/cs5535: Un-break driver with 'set-state' interface

[David Gstir]

Since migrating to the 'set-state' interface the cs5535 driver causes a
crash after loading: Right after initialization, the IRQ handler
(mfgpt_tick) is triggered with clock event device in detached state. This
state not properly handled and causes a crash through NULL pointer
dereference upon calling the clockevent's event_handler.

This patch fixes this by handling the detached state the same way the
shutdown state is handled.

Fixes: 8f9327cbb6e8 ("clockevents/drivers/cs5535: Migrate to new 'set-state' 
interface")
Cc: sta...@vger.kernel.org
Cc: Andres Salomon 
Cc: Viresh Kumar 
Signed-off-by: David Gstir 
---
 drivers/clocksource/cs5535-clockevt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/cs5535-clockevt.c 
b/drivers/clocksource/cs5535-clockevt.c
index 9a7e37cf56b0..649e0cd90805 100644
--- a/drivers/clocksource/cs5535-clockevt.c
+++ b/drivers/clocksource/cs5535-clockevt.c
@@ -117,7 +117,8 @@ static irqreturn_t mfgpt_tick(int irq, void *dev_id)
/* Turn off the clock (and clear the event) */
disable_timer(cs5535_event_clock);
 
-   if (clockevent_state_shutdown(_clockevent))
+   if (clockevent_state_shutdown(_clockevent) ||
+   clockevent_state_detached(_clockevent))
return IRQ_HANDLED;
 
/* Clear the counter */
-- 
2.11.0

Re: [PATCH v2] fscrypt: Factor out bio specific functions

[David Gstir]

Hi,

> On 19.12.2016, at 12:25, Richard Weinberger <rich...@nod.at> wrote:
> 
> That way we can get rid of the direct dependency on CONFIG_BLOCK.
> 
> Reported-by: Arnd Bergmann <a...@arndb.de>
> Reported-by: Randy Dunlap <rdun...@infradead.org>
> Suggested-by: Christoph Hellwig <h...@infradead.org>
> Fixes: d475a507457b ("ubifs: Add skeleton for fscrypto")
> Signed-off-by: Richard Weinberger <rich...@nod.at>
> ---
> Changes since v1:
> - Moved fscrypt_zeroout_range() also to bio.c

Looks good to me.

Reviewed-by: David Gstir <da...@sigma-star.at>

- David

Re: [PATCH v2] fscrypt: Factor out bio specific functions

[David Gstir]

Hi,

> On 19.12.2016, at 12:25, Richard Weinberger  wrote:
> 
> That way we can get rid of the direct dependency on CONFIG_BLOCK.
> 
> Reported-by: Arnd Bergmann 
> Reported-by: Randy Dunlap 
> Suggested-by: Christoph Hellwig 
> Fixes: d475a507457b ("ubifs: Add skeleton for fscrypto")
> Signed-off-by: Richard Weinberger 
> ---
> Changes since v1:
> - Moved fscrypt_zeroout_range() also to bio.c

Looks good to me.

Reviewed-by: David Gstir 

- David

Re: [PATCH] fscrypt: Factor out bio specific functions

[David Gstir]

Hi,

> On 16.12.2016, at 11:50, Richard Weinberger  wrote:
> 
> That way we can get rid of the direct dependency on CONFIG_BLOCK.
> 
> Reported-by: Arnd Bergmann 
> Reported-by: Randy Dunlap 
> Suggested-by: Christoph Hellwig 
> Fixes: d475a507457b ("ubifs: Add skeleton for fscrypto")
> Signed-off-by: Richard Weinberger 
> ---
> fs/crypto/Kconfig   |   1 -
> fs/crypto/Makefile  |   1 +
> fs/crypto/bio.c | 115 
> fs/crypto/crypto.c  |  89 ++
> fs/crypto/fscrypt_private.h |  14 ++
> include/linux/fscrypto.h|   6 ++-
> 6 files changed, 137 insertions(+), 89 deletions(-)
> create mode 100644 fs/crypto/bio.c
> 
> diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> index f514978f6688..08b46e6e3995 100644
> --- a/fs/crypto/Kconfig
> +++ b/fs/crypto/Kconfig
> @@ -1,6 +1,5 @@
> config FS_ENCRYPTION
>   tristate "FS Encryption (Per-file encryption)"
> - depends on BLOCK
>   select CRYPTO
>   select CRYPTO_AES
>   select CRYPTO_CBC
> diff --git a/fs/crypto/Makefile b/fs/crypto/Makefile
> index f17684c48739..9f6607f17b53 100644
> --- a/fs/crypto/Makefile
> +++ b/fs/crypto/Makefile
> @@ -1,3 +1,4 @@
> obj-$(CONFIG_FS_ENCRYPTION)   += fscrypto.o
> 
> fscrypto-y := crypto.o fname.o policy.o keyinfo.o
> +fscrypto-$(CONFIG_BLOCK) += bio.o
> diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
> new file mode 100644
> index ..6a6e694c6879
> --- /dev/null
> +++ b/fs/crypto/bio.c
> @@ -0,0 +1,115 @@
> +/*
> + * This contains encryption functions for per-file encryption.
> + *
> + * Copyright (C) 2015, Google, Inc.
> + * Copyright (C) 2015, Motorola Mobility
> + *
> + * Written by Michael Halcrow, 2014.
> + *
> + * Filename encryption additions
> + *   Uday Savagaonkar, 2014
> + * Encryption policy handling additions
> + *   Ildar Muslukhov, 2014
> + * Add fscrypt_pullback_bio_page()
> + *   Jaegeuk Kim, 2015.
> + *
> + * This has not yet undergone a rigorous security audit.
> + *
> + * The usage of AES-XTS should conform to recommendations in NIST
> + * Special Publication 800-38E and IEEE P1619/D16.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "fscrypt_private.h"
> +
> +/*
> + * Call fscrypt_decrypt_page on every single page, reusing the encryption
> + * context.
> + */
> +static void completion_pages(struct work_struct *work)
> +{
> + struct fscrypt_ctx *ctx =
> + container_of(work, struct fscrypt_ctx, r.work);
> + struct bio *bio = ctx->r.bio;
> + struct bio_vec *bv;
> + int i;
> +
> + bio_for_each_segment_all(bv, bio, i) {
> + struct page *page = bv->bv_page;
> + int ret = fscrypt_decrypt_page(page->mapping->host, page,
> + PAGE_SIZE, 0, page->index);
> +
> + if (ret) {
> + WARN_ON_ONCE(1);
> + SetPageError(page);
> + } else {
> + SetPageUptodate(page);
> + }
> + unlock_page(page);
> + }
> + fscrypt_release_ctx(ctx);
> + bio_put(bio);
> +}
> +
> +void fscrypt_decrypt_bio_pages(struct fscrypt_ctx *ctx, struct bio *bio)
> +{
> + INIT_WORK(>r.work, completion_pages);
> + ctx->r.bio = bio;
> + queue_work(fscrypt_read_workqueue, >r.work);
> +}
> +EXPORT_SYMBOL(fscrypt_decrypt_bio_pages);
> +
> +void fscrypt_pullback_bio_page(struct page **page, bool restore)
> +{
> + struct fscrypt_ctx *ctx;
> + struct page *bounce_page;
> +
> + /* The bounce data pages are unmapped. */
> + if ((*page)->mapping)
> + return;
> +
> + /* The bounce data page is unmapped. */
> + bounce_page = *page;
> + ctx = (struct fscrypt_ctx *)page_private(bounce_page);
> +
> + /* restore control page */
> + *page = ctx->w.control_page;
> +
> + if (restore)
> + fscrypt_restore_control_page(bounce_page);
> +}
> +EXPORT_SYMBOL(fscrypt_pullback_bio_page);
> +
> +int fscrypt_bio_submit_page(const struct inode *inode, sector_t blk,
> + struct page *page)
> +{
> + struct bio *bio;
> + int ret;
> +
> + bio = bio_alloc(GFP_NOWAIT, 1);
> + if (!bio) {
> + ret = -ENOMEM;
> + goto errout;
> + }
> + bio->bi_bdev = inode->i_sb->s_bdev;
> + bio->bi_iter.bi_sector = blk << (inode->i_sb->s_blocksize_bits - 9);
> + bio_set_op_attrs(bio, REQ_OP_WRITE, 0);
> + ret = bio_add_page(bio, page, inode->i_sb->s_blocksize, 0);
> + if (ret != inode->i_sb->s_blocksize) {
> + /* should never happen! */
> + WARN_ON(1);
> + bio_put(bio);
> + ret = -EIO;
> + goto errout;
> + }
> + ret = submit_bio_wait(bio);
> + if ((ret == 0) && bio->bi_error)
> + ret = -EIO;
> + bio_put(bio);
> +

Re: [PATCH] fscrypt: Factor out bio specific functions

[David Gstir]

Hi,

> On 16.12.2016, at 11:50, Richard Weinberger  wrote:
> 
> That way we can get rid of the direct dependency on CONFIG_BLOCK.
> 
> Reported-by: Arnd Bergmann 
> Reported-by: Randy Dunlap 
> Suggested-by: Christoph Hellwig 
> Fixes: d475a507457b ("ubifs: Add skeleton for fscrypto")
> Signed-off-by: Richard Weinberger 
> ---
> fs/crypto/Kconfig   |   1 -
> fs/crypto/Makefile  |   1 +
> fs/crypto/bio.c | 115 
> fs/crypto/crypto.c  |  89 ++
> fs/crypto/fscrypt_private.h |  14 ++
> include/linux/fscrypto.h|   6 ++-
> 6 files changed, 137 insertions(+), 89 deletions(-)
> create mode 100644 fs/crypto/bio.c
> 
> diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> index f514978f6688..08b46e6e3995 100644
> --- a/fs/crypto/Kconfig
> +++ b/fs/crypto/Kconfig
> @@ -1,6 +1,5 @@
> config FS_ENCRYPTION
>   tristate "FS Encryption (Per-file encryption)"
> - depends on BLOCK
>   select CRYPTO
>   select CRYPTO_AES
>   select CRYPTO_CBC
> diff --git a/fs/crypto/Makefile b/fs/crypto/Makefile
> index f17684c48739..9f6607f17b53 100644
> --- a/fs/crypto/Makefile
> +++ b/fs/crypto/Makefile
> @@ -1,3 +1,4 @@
> obj-$(CONFIG_FS_ENCRYPTION)   += fscrypto.o
> 
> fscrypto-y := crypto.o fname.o policy.o keyinfo.o
> +fscrypto-$(CONFIG_BLOCK) += bio.o
> diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
> new file mode 100644
> index ..6a6e694c6879
> --- /dev/null
> +++ b/fs/crypto/bio.c
> @@ -0,0 +1,115 @@
> +/*
> + * This contains encryption functions for per-file encryption.
> + *
> + * Copyright (C) 2015, Google, Inc.
> + * Copyright (C) 2015, Motorola Mobility
> + *
> + * Written by Michael Halcrow, 2014.
> + *
> + * Filename encryption additions
> + *   Uday Savagaonkar, 2014
> + * Encryption policy handling additions
> + *   Ildar Muslukhov, 2014
> + * Add fscrypt_pullback_bio_page()
> + *   Jaegeuk Kim, 2015.
> + *
> + * This has not yet undergone a rigorous security audit.
> + *
> + * The usage of AES-XTS should conform to recommendations in NIST
> + * Special Publication 800-38E and IEEE P1619/D16.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "fscrypt_private.h"
> +
> +/*
> + * Call fscrypt_decrypt_page on every single page, reusing the encryption
> + * context.
> + */
> +static void completion_pages(struct work_struct *work)
> +{
> + struct fscrypt_ctx *ctx =
> + container_of(work, struct fscrypt_ctx, r.work);
> + struct bio *bio = ctx->r.bio;
> + struct bio_vec *bv;
> + int i;
> +
> + bio_for_each_segment_all(bv, bio, i) {
> + struct page *page = bv->bv_page;
> + int ret = fscrypt_decrypt_page(page->mapping->host, page,
> + PAGE_SIZE, 0, page->index);
> +
> + if (ret) {
> + WARN_ON_ONCE(1);
> + SetPageError(page);
> + } else {
> + SetPageUptodate(page);
> + }
> + unlock_page(page);
> + }
> + fscrypt_release_ctx(ctx);
> + bio_put(bio);
> +}
> +
> +void fscrypt_decrypt_bio_pages(struct fscrypt_ctx *ctx, struct bio *bio)
> +{
> + INIT_WORK(>r.work, completion_pages);
> + ctx->r.bio = bio;
> + queue_work(fscrypt_read_workqueue, >r.work);
> +}
> +EXPORT_SYMBOL(fscrypt_decrypt_bio_pages);
> +
> +void fscrypt_pullback_bio_page(struct page **page, bool restore)
> +{
> + struct fscrypt_ctx *ctx;
> + struct page *bounce_page;
> +
> + /* The bounce data pages are unmapped. */
> + if ((*page)->mapping)
> + return;
> +
> + /* The bounce data page is unmapped. */
> + bounce_page = *page;
> + ctx = (struct fscrypt_ctx *)page_private(bounce_page);
> +
> + /* restore control page */
> + *page = ctx->w.control_page;
> +
> + if (restore)
> + fscrypt_restore_control_page(bounce_page);
> +}
> +EXPORT_SYMBOL(fscrypt_pullback_bio_page);
> +
> +int fscrypt_bio_submit_page(const struct inode *inode, sector_t blk,
> + struct page *page)
> +{
> + struct bio *bio;
> + int ret;
> +
> + bio = bio_alloc(GFP_NOWAIT, 1);
> + if (!bio) {
> + ret = -ENOMEM;
> + goto errout;
> + }
> + bio->bi_bdev = inode->i_sb->s_bdev;
> + bio->bi_iter.bi_sector = blk << (inode->i_sb->s_blocksize_bits - 9);
> + bio_set_op_attrs(bio, REQ_OP_WRITE, 0);
> + ret = bio_add_page(bio, page, inode->i_sb->s_blocksize, 0);
> + if (ret != inode->i_sb->s_blocksize) {
> + /* should never happen! */
> + WARN_ON(1);
> + bio_put(bio);
> + ret = -EIO;
> + goto errout;
> + }
> + ret = submit_bio_wait(bio);
> + if ((ret == 0) && bio->bi_error)
> + ret = -EIO;
> + bio_put(bio);
> +
> +errout:
> + return ret;
> +}
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c

Re: [PATCH 2/2] ubifs: Use fscrypt ioctl() helpers

[David Gstir]

> On 13.12.2016, at 00:27, Richard Weinberger <rich...@nod.at> wrote:
> 
> Commit db717d8e26c2 ("fscrypto: move ioctl processing more fully into
> common code") moved ioctl() related functions into fscrypt and offers
> us now a set of helper functions.
> 
> Signed-off-by: Richard Weinberger <rich...@nod.at>
> ---
> fs/ubifs/ioctl.c | 24 ++--
> fs/ubifs/ubifs.h |  4 ++--
> 2 files changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/ubifs/ioctl.c b/fs/ubifs/ioctl.c
> index 3d10f5525274..78d713644df3 100644
> --- a/fs/ubifs/ioctl.c
> +++ b/fs/ubifs/ioctl.c
> @@ -184,39 +184,19 @@ long ubifs_ioctl(struct file *file, unsigned int cmd, 
> unsigned long arg)
>   case FS_IOC_SET_ENCRYPTION_POLICY: {
> #ifdef CONFIG_UBIFS_FS_ENCRYPTION
>   struct ubifs_info *c = inode->i_sb->s_fs_info;
> - struct fscrypt_policy policy;
> -
> - if (copy_from_user(,
> -(struct fscrypt_policy __user *)arg,
> -sizeof(policy)))
> - return -EFAULT;
> 
>   err = ubifs_enable_encryption(c);
>   if (err)
>   return err;
> 
> - err = fscrypt_process_policy(file, );
> -
> - return err;
> + return fscrypt_ioctl_set_policy(file, (const void __user *)arg);
> #else
>   return -EOPNOTSUPP;
> #endif
>   }
>   case FS_IOC_GET_ENCRYPTION_POLICY: {
> #ifdef CONFIG_UBIFS_FS_ENCRYPTION
> - struct fscrypt_policy policy;
> -
> - if (!ubifs_crypt_is_encrypted(inode))
> - return -ENOENT;
> -
> - err = fscrypt_get_policy(inode, );
> - if (err)
> - return err;
> -
> - if (copy_to_user((void __user *)arg, , sizeof(policy)))
> - return -EFAULT;
> -
> - return 0;
> + return fscrypt_ioctl_get_policy(file, (void __user *)arg);
> #else
>   return -EOPNOTSUPP;
> #endif
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index 8d0e4818e3ea..ca72382ce6cc 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -1806,8 +1806,8 @@ int ubifs_decompress(const struct ubifs_info *c, const 
> void *buf, int len,
> #define fscrypt_pullback_bio_page   fscrypt_notsupp_pullback_bio_page
> #define fscrypt_restore_control_pagefscrypt_notsupp_restore_control_page
> #define fscrypt_zeroout_range   fscrypt_notsupp_zeroout_range
> -#define fscrypt_process_policy  fscrypt_notsupp_process_policy
> -#define fscrypt_get_policy  fscrypt_notsupp_get_policy
> +#define fscrypt_ioctl_set_policy fscrypt_notsupp_ioctl_set_policy
> +#define fscrypt_ioctl_get_policy fscrypt_notsupp_ioctl_get_policy
> #define fscrypt_has_permitted_context   fscrypt_notsupp_has_permitted_context
> #define fscrypt_inherit_context fscrypt_notsupp_inherit_context
> #define fscrypt_get_encryption_info fscrypt_notsupp_get_encryption_info
> -- 
> 2.10.2

Looks good to me.

Reviewed-by: David Gstir <da...@sigma-star.at>

- David

Re: [PATCH 2/2] ubifs: Use fscrypt ioctl() helpers

[David Gstir]

> On 13.12.2016, at 00:27, Richard Weinberger  wrote:
> 
> Commit db717d8e26c2 ("fscrypto: move ioctl processing more fully into
> common code") moved ioctl() related functions into fscrypt and offers
> us now a set of helper functions.
> 
> Signed-off-by: Richard Weinberger 
> ---
> fs/ubifs/ioctl.c | 24 ++--
> fs/ubifs/ubifs.h |  4 ++--
> 2 files changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/ubifs/ioctl.c b/fs/ubifs/ioctl.c
> index 3d10f5525274..78d713644df3 100644
> --- a/fs/ubifs/ioctl.c
> +++ b/fs/ubifs/ioctl.c
> @@ -184,39 +184,19 @@ long ubifs_ioctl(struct file *file, unsigned int cmd, 
> unsigned long arg)
>   case FS_IOC_SET_ENCRYPTION_POLICY: {
> #ifdef CONFIG_UBIFS_FS_ENCRYPTION
>   struct ubifs_info *c = inode->i_sb->s_fs_info;
> - struct fscrypt_policy policy;
> -
> - if (copy_from_user(,
> -(struct fscrypt_policy __user *)arg,
> -sizeof(policy)))
> - return -EFAULT;
> 
>   err = ubifs_enable_encryption(c);
>   if (err)
>   return err;
> 
> - err = fscrypt_process_policy(file, );
> -
> - return err;
> + return fscrypt_ioctl_set_policy(file, (const void __user *)arg);
> #else
>   return -EOPNOTSUPP;
> #endif
>   }
>   case FS_IOC_GET_ENCRYPTION_POLICY: {
> #ifdef CONFIG_UBIFS_FS_ENCRYPTION
> - struct fscrypt_policy policy;
> -
> - if (!ubifs_crypt_is_encrypted(inode))
> - return -ENOENT;
> -
> - err = fscrypt_get_policy(inode, );
> - if (err)
> - return err;
> -
> - if (copy_to_user((void __user *)arg, , sizeof(policy)))
> - return -EFAULT;
> -
> - return 0;
> + return fscrypt_ioctl_get_policy(file, (void __user *)arg);
> #else
>   return -EOPNOTSUPP;
> #endif
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index 8d0e4818e3ea..ca72382ce6cc 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -1806,8 +1806,8 @@ int ubifs_decompress(const struct ubifs_info *c, const 
> void *buf, int len,
> #define fscrypt_pullback_bio_page   fscrypt_notsupp_pullback_bio_page
> #define fscrypt_restore_control_pagefscrypt_notsupp_restore_control_page
> #define fscrypt_zeroout_range   fscrypt_notsupp_zeroout_range
> -#define fscrypt_process_policy  fscrypt_notsupp_process_policy
> -#define fscrypt_get_policy  fscrypt_notsupp_get_policy
> +#define fscrypt_ioctl_set_policy fscrypt_notsupp_ioctl_set_policy
> +#define fscrypt_ioctl_get_policy fscrypt_notsupp_ioctl_get_policy
> #define fscrypt_has_permitted_context   fscrypt_notsupp_has_permitted_context
> #define fscrypt_inherit_context fscrypt_notsupp_inherit_context
> #define fscrypt_get_encryption_info fscrypt_notsupp_get_encryption_info
> -- 
> 2.10.2

Looks good to me.

Reviewed-by: David Gstir 

- David

[PATCH v2 5/6] fscrypt: Delay bounce page pool allocation until needed

[David Gstir]

Since fscrypt users can now indicated if fscrypt_encrypt_page() should
use a bounce page, we can delay the bounce page pool initialization util
it is really needed. That is until fscrypt_operations has no
FS_CFLG_OWN_PAGES flag set.

Signed-off-by: David Gstir <da...@sigma-star.at>
---
 fs/crypto/crypto.c   | 9 +++--
 fs/crypto/keyinfo.c  | 2 +-
 include/linux/fscrypto.h | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6f5c3a8df70b..c1e4316045e9 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -525,17 +525,22 @@ static void fscrypt_destroy(void)
 
 /**
  * fscrypt_initialize() - allocate major buffers for fs encryption.
+ * @cop_flags:  fscrypt operations flags
  *
  * We only call this when we start accessing encrypted files, since it
  * results in memory getting allocated that wouldn't otherwise be used.
  *
  * Return: Zero on success, non-zero otherwise.
  */
-int fscrypt_initialize(void)
+int fscrypt_initialize(unsigned int cop_flags)
 {
int i, res = -ENOMEM;
 
-   if (fscrypt_bounce_page_pool)
+   /*
+* No need to allocate a bounce page pool if there already is one or
+* this FS won't use it.
+*/
+   if (cop_flags & FS_CFLG_OWN_PAGES || fscrypt_bounce_page_pool)
return 0;
 
mutex_lock(_init_mutex);
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 67fb6d8876d0..5951f4ebf2a9 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -188,7 +188,7 @@ int get_crypt_info(struct inode *inode)
u8 *raw_key = NULL;
int res;
 
-   res = fscrypt_initialize();
+   res = fscrypt_initialize(inode->i_sb->s_cop->flags);
if (res)
return res;
 
diff --git a/include/linux/fscrypto.h b/include/linux/fscrypto.h
index 65be74f3afb6..6039c86ff849 100644
--- a/include/linux/fscrypto.h
+++ b/include/linux/fscrypto.h
@@ -244,7 +244,7 @@ static inline void fscrypt_set_d_op(struct dentry *dentry)
 #if IS_ENABLED(CONFIG_FS_ENCRYPTION)
 /* crypto.c */
 extern struct kmem_cache *fscrypt_info_cachep;
-int fscrypt_initialize(void);
+int fscrypt_initialize(unsigned int cop_flags);
 
 extern struct fscrypt_ctx *fscrypt_get_ctx(const struct inode *, gfp_t);
 extern void fscrypt_release_ctx(struct fscrypt_ctx *);
-- 
2.10.1

[PATCH v2 2/6] fscrypt: Never allocate fscrypt_ctx on in-place encryption

[David Gstir]

In case of in-place encryption fscrypt_ctx was allocated but never
released. Since we don't need it for in-place encryption, we skip
allocating it.

Fixes: 1c7dcf69eea3 ("fscrypt: Add in-place encryption mode")

Signed-off-by: David Gstir <da...@sigma-star.at>
---
 fs/crypto/crypto.c | 30 +++---
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 3c1124beae28..8536ac0b50d3 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -246,16 +246,26 @@ struct page *fscrypt_encrypt_page(const struct inode 
*inode,
 
BUG_ON(plaintext_len % FS_CRYPTO_BLOCK_SIZE != 0);
 
+   if (inode->i_sb->s_cop->flags & FS_CFLG_INPLACE_ENCRYPTION) {
+   /* with inplace-encryption we just encrypt the page */
+   err = do_page_crypto(inode, FS_ENCRYPT, index,
+   plaintext_page, ciphertext_page,
+   plaintext_len, plaintext_offset,
+   gfp_flags);
+   if (err)
+   return ERR_PTR(err);
+
+   return ciphertext_page;
+   }
+
ctx = fscrypt_get_ctx(inode, gfp_flags);
if (IS_ERR(ctx))
return (struct page *)ctx;
 
-   if (!(inode->i_sb->s_cop->flags & FS_CFLG_INPLACE_ENCRYPTION)) {
-   /* The encryption operation will require a bounce page. */
-   ciphertext_page = alloc_bounce_page(ctx, gfp_flags);
-   if (IS_ERR(ciphertext_page))
-   goto errout;
-   }
+   /* The encryption operation will require a bounce page. */
+   ciphertext_page = alloc_bounce_page(ctx, gfp_flags);
+   if (IS_ERR(ciphertext_page))
+   goto errout;
 
ctx->w.control_page = plaintext_page;
err = do_page_crypto(inode, FS_ENCRYPT, index,
@@ -266,11 +276,9 @@ struct page *fscrypt_encrypt_page(const struct inode 
*inode,
ciphertext_page = ERR_PTR(err);
goto errout;
}
-   if (!(inode->i_sb->s_cop->flags & FS_CFLG_INPLACE_ENCRYPTION)) {
-   SetPagePrivate(ciphertext_page);
-   set_page_private(ciphertext_page, (unsigned long)ctx);
-   lock_page(ciphertext_page);
-   }
+   SetPagePrivate(ciphertext_page);
+   set_page_private(ciphertext_page, (unsigned long)ctx);
+   lock_page(ciphertext_page);
return ciphertext_page;
 
 errout:
-- 
2.10.1

[PATCH v2 5/6] fscrypt: Delay bounce page pool allocation until needed

[David Gstir]

Since fscrypt users can now indicated if fscrypt_encrypt_page() should
use a bounce page, we can delay the bounce page pool initialization util
it is really needed. That is until fscrypt_operations has no
FS_CFLG_OWN_PAGES flag set.

Signed-off-by: David Gstir 
---
 fs/crypto/crypto.c   | 9 +++--
 fs/crypto/keyinfo.c  | 2 +-
 include/linux/fscrypto.h | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6f5c3a8df70b..c1e4316045e9 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -525,17 +525,22 @@ static void fscrypt_destroy(void)
 
 /**
  * fscrypt_initialize() - allocate major buffers for fs encryption.
+ * @cop_flags:  fscrypt operations flags
  *
  * We only call this when we start accessing encrypted files, since it
  * results in memory getting allocated that wouldn't otherwise be used.
  *
  * Return: Zero on success, non-zero otherwise.
  */
-int fscrypt_initialize(void)
+int fscrypt_initialize(unsigned int cop_flags)
 {
int i, res = -ENOMEM;
 
-   if (fscrypt_bounce_page_pool)
+   /*
+* No need to allocate a bounce page pool if there already is one or
+* this FS won't use it.
+*/
+   if (cop_flags & FS_CFLG_OWN_PAGES || fscrypt_bounce_page_pool)
return 0;
 
mutex_lock(_init_mutex);
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 67fb6d8876d0..5951f4ebf2a9 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -188,7 +188,7 @@ int get_crypt_info(struct inode *inode)
u8 *raw_key = NULL;
int res;
 
-   res = fscrypt_initialize();
+   res = fscrypt_initialize(inode->i_sb->s_cop->flags);
if (res)
return res;
 
diff --git a/include/linux/fscrypto.h b/include/linux/fscrypto.h
index 65be74f3afb6..6039c86ff849 100644
--- a/include/linux/fscrypto.h
+++ b/include/linux/fscrypto.h
@@ -244,7 +244,7 @@ static inline void fscrypt_set_d_op(struct dentry *dentry)
 #if IS_ENABLED(CONFIG_FS_ENCRYPTION)
 /* crypto.c */
 extern struct kmem_cache *fscrypt_info_cachep;
-int fscrypt_initialize(void);
+int fscrypt_initialize(unsigned int cop_flags);
 
 extern struct fscrypt_ctx *fscrypt_get_ctx(const struct inode *, gfp_t);
 extern void fscrypt_release_ctx(struct fscrypt_ctx *);
-- 
2.10.1

[PATCH v2 2/6] fscrypt: Never allocate fscrypt_ctx on in-place encryption

[David Gstir]

In case of in-place encryption fscrypt_ctx was allocated but never
released. Since we don't need it for in-place encryption, we skip
allocating it.

Fixes: 1c7dcf69eea3 ("fscrypt: Add in-place encryption mode")

Signed-off-by: David Gstir 
---
 fs/crypto/crypto.c | 30 +++---
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 3c1124beae28..8536ac0b50d3 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -246,16 +246,26 @@ struct page *fscrypt_encrypt_page(const struct inode 
*inode,
 
BUG_ON(plaintext_len % FS_CRYPTO_BLOCK_SIZE != 0);
 
+   if (inode->i_sb->s_cop->flags & FS_CFLG_INPLACE_ENCRYPTION) {
+   /* with inplace-encryption we just encrypt the page */
+   err = do_page_crypto(inode, FS_ENCRYPT, index,
+   plaintext_page, ciphertext_page,
+   plaintext_len, plaintext_offset,
+   gfp_flags);
+   if (err)
+   return ERR_PTR(err);
+
+   return ciphertext_page;
+   }
+
ctx = fscrypt_get_ctx(inode, gfp_flags);
if (IS_ERR(ctx))
return (struct page *)ctx;
 
-   if (!(inode->i_sb->s_cop->flags & FS_CFLG_INPLACE_ENCRYPTION)) {
-   /* The encryption operation will require a bounce page. */
-   ciphertext_page = alloc_bounce_page(ctx, gfp_flags);
-   if (IS_ERR(ciphertext_page))
-   goto errout;
-   }
+   /* The encryption operation will require a bounce page. */
+   ciphertext_page = alloc_bounce_page(ctx, gfp_flags);
+   if (IS_ERR(ciphertext_page))
+   goto errout;
 
ctx->w.control_page = plaintext_page;
err = do_page_crypto(inode, FS_ENCRYPT, index,
@@ -266,11 +276,9 @@ struct page *fscrypt_encrypt_page(const struct inode 
*inode,
ciphertext_page = ERR_PTR(err);
goto errout;
}
-   if (!(inode->i_sb->s_cop->flags & FS_CFLG_INPLACE_ENCRYPTION)) {
-   SetPagePrivate(ciphertext_page);
-   set_page_private(ciphertext_page, (unsigned long)ctx);
-   lock_page(ciphertext_page);
-   }
+   SetPagePrivate(ciphertext_page);
+   set_page_private(ciphertext_page, (unsigned long)ctx);
+   lock_page(ciphertext_page);
return ciphertext_page;
 
 errout:
-- 
2.10.1

[PATCH v2 4/6] fscrypt: Cleanup page locking requirements for fscrypt_{decrypt,encrypt}_page()

[David Gstir]

Rename the FS_CFLG_INPLACE_ENCRYPTION flag to FS_CFLG_OWN_PAGES which,
when set, indicates that the fs uses pages under its own control as
opposed to writeback pages which require locking and a bounce buffer for
encryption.

Signed-off-by: David Gstir <da...@sigma-star.at>
---
 fs/crypto/crypto.c   | 11 ---
 fs/ext4/inode.c  |  1 -
 fs/f2fs/data.c   |  1 -
 include/linux/fscrypto.h |  2 +-
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 8805ec5d7de2..6f5c3a8df70b 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -238,7 +238,7 @@ static struct page *alloc_bounce_page(struct fscrypt_ctx 
*ctx, gfp_t gfp_flags)
  * fscrypt_restore_control_page() on the returned ciphertext page to
  * release the bounce buffer and the encryption context.
  *
- * In-place encryption is used by setting the FS_CFLG_INPLACE_ENCRYPTION flag 
in
+ * In-place encryption is used by setting the FS_CFLG_OWN_PAGES flag in
  * fscrypt_operations. Here, the input-page is returned with its content
  * encrypted.
  *
@@ -258,7 +258,7 @@ struct page *fscrypt_encrypt_page(const struct inode *inode,
 
BUG_ON(len % FS_CRYPTO_BLOCK_SIZE != 0);
 
-   if (inode->i_sb->s_cop->flags & FS_CFLG_INPLACE_ENCRYPTION) {
+   if (inode->i_sb->s_cop->flags & FS_CFLG_OWN_PAGES) {
/* with inplace-encryption we just encrypt the page */
err = do_page_crypto(inode, FS_ENCRYPT, lblk_num,
page, ciphertext_page,
@@ -269,6 +269,8 @@ struct page *fscrypt_encrypt_page(const struct inode *inode,
return ciphertext_page;
}
 
+   BUG_ON(!PageLocked(page));
+
ctx = fscrypt_get_ctx(inode, gfp_flags);
if (IS_ERR(ctx))
return (struct page *)ctx;
@@ -301,7 +303,7 @@ EXPORT_SYMBOL(fscrypt_encrypt_page);
  * fscrypt_decrypt_page() - Decrypts a page in-place
  * @inode: The corresponding inode for the page to decrypt.
  * @page:  The page to decrypt. Must be locked in case
- * it is a writeback page.
+ * it is a writeback page (FS_CFLG_OWN_PAGES unset).
  * @len:   Number of bytes in @page to be decrypted.
  * @offs:  Start of data in @page.
  * @lblk_num:  Logical block number.
@@ -315,6 +317,9 @@ EXPORT_SYMBOL(fscrypt_encrypt_page);
 int fscrypt_decrypt_page(const struct inode *inode, struct page *page,
unsigned int len, unsigned int offs, u64 lblk_num)
 {
+   if (!(inode->i_sb->s_cop->flags & FS_CFLG_OWN_PAGES))
+   BUG_ON(!PageLocked(page));
+
return do_page_crypto(inode, FS_DECRYPT, lblk_num, page, page, len,
offs, GFP_NOFS);
 }
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 1485ac273bfb..fb2b514f675b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3744,7 +3744,6 @@ static int __ext4_block_zero_page_range(handle_t *handle,
/* We expect the key to be set. */
BUG_ON(!fscrypt_has_encryption_key(inode));
BUG_ON(blocksize != PAGE_SIZE);
-   BUG_ON(!PageLocked(page));
WARN_ON_ONCE(fscrypt_decrypt_page(page->mapping->host,
page, PAGE_SIZE, 0, 
page->index));
}
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 435590c4b341..9f0ba90b92e4 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1194,7 +1194,6 @@ int do_write_data_page(struct f2fs_io_info *fio)
f2fs_wait_on_encrypted_page_writeback(F2FS_I_SB(inode),
fio->old_blkaddr);
 retry_encrypt:
-   BUG_ON(!PageLocked(fio->page));
fio->encrypted_page = fscrypt_encrypt_page(inode, fio->page,
PAGE_SIZE, 0,
fio->page->index,
diff --git a/include/linux/fscrypto.h b/include/linux/fscrypto.h
index e9648b62eca6..65be74f3afb6 100644
--- a/include/linux/fscrypto.h
+++ b/include/linux/fscrypto.h
@@ -156,7 +156,7 @@ struct fscrypt_name {
 /*
  * fscrypt superblock flags
  */
-#define FS_CFLG_INPLACE_ENCRYPTION (1U << 1)
+#define FS_CFLG_OWN_PAGES (1U << 1)
 
 /*
  * crypto opertions for filesystems
-- 
2.10.1

[PATCH v2 0/6] UBIFS related fscrypt updates

[David Gstir]

This series applies on top of Ted's fscrypt tree[0] addresses the review
comments from Eric.

Changes since v1:
- Never allocate fscrypt_ctx in "own pages" mode
- Fix code comments
- Fix issues reported by checkpatch.pl

[0] git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git fscrypt

David Gstir (6):
  fscrypt: Use correct index in decrypt path.
  fscrypt: Never allocate fscrypt_ctx on in-place encryption
  fscrypt: Cleanup fscrypt_{decrypt,encrypt}_page()
  fscrypt: Cleanup page locking requirements for
fscrypt_{decrypt,encrypt}_page()
  fscrypt: Delay bounce page pool allocation until needed
  fscrypt: Rename FS_WRITE_PATH_FL to FS_CTX_HAS_BOUNCE_BUFFER_FL

 fs/crypto/crypto.c   | 135 ---
 fs/crypto/keyinfo.c  |   2 +-
 fs/ext4/inode.c  |   1 -
 fs/f2fs/data.c   |   1 -
 include/linux/fscrypto.h |  14 ++---
 5 files changed, 90 insertions(+), 63 deletions(-)

-- 
2.10.1

[PATCH v2 3/6] fscrypt: Cleanup fscrypt_{decrypt,encrypt}_page()

[David Gstir]

- Improve documentation
- Add BUG_ON(len == 0) to avoid accidental switch of offs and len
parameters
- Improve variable names for readability

Signed-off-by: David Gstir <da...@sigma-star.at>
---
 fs/crypto/crypto.c   | 93 +++-
 include/linux/fscrypto.h |  8 ++---
 2 files changed, 56 insertions(+), 45 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 8536ac0b50d3..8805ec5d7de2 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -147,9 +147,9 @@ typedef enum {
 } fscrypt_direction_t;
 
 static int do_page_crypto(const struct inode *inode,
-   fscrypt_direction_t rw, pgoff_t index,
+   fscrypt_direction_t rw, u64 lblk_num,
struct page *src_page, struct page *dest_page,
-   unsigned int src_len, unsigned int src_offset,
+   unsigned int len, unsigned int offs,
gfp_t gfp_flags)
 {
struct {
@@ -163,6 +163,8 @@ static int do_page_crypto(const struct inode *inode,
struct crypto_skcipher *tfm = ci->ci_ctfm;
int res = 0;
 
+   BUG_ON(len == 0);
+
req = skcipher_request_alloc(tfm, gfp_flags);
if (!req) {
printk_ratelimited(KERN_ERR
@@ -176,14 +178,14 @@ static int do_page_crypto(const struct inode *inode,
page_crypt_complete, );
 
BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
-   xts_tweak.index = cpu_to_le64(index);
+   xts_tweak.index = cpu_to_le64(lblk_num);
memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
 
sg_init_table(, 1);
-   sg_set_page(, dest_page, src_len, src_offset);
+   sg_set_page(, dest_page, len, offs);
sg_init_table(, 1);
-   sg_set_page(, src_page, src_len, src_offset);
-   skcipher_request_set_crypt(req, , , src_len, _tweak);
+   sg_set_page(, src_page, len, offs);
+   skcipher_request_set_crypt(req, , , len, _tweak);
if (rw == FS_DECRYPT)
res = crypto_skcipher_decrypt(req);
else
@@ -214,44 +216,53 @@ static struct page *alloc_bounce_page(struct fscrypt_ctx 
*ctx, gfp_t gfp_flags)
 
 /**
  * fscypt_encrypt_page() - Encrypts a page
- * @inode:The inode for which the encryption should take place
- * @plaintext_page:   The page to encrypt. Must be locked.
- * @plaintext_len:Length of plaintext within page
- * @plaintext_offset: Offset of plaintext within page
- * @index:Index for encryption. This is mainly the page index, but
- *but might be different for multiple calls on same page.
- * @gfp_flags:The gfp flag for memory allocation
+ * @inode: The inode for which the encryption should take place
+ * @page:  The page to encrypt. Must be locked for bounce-page
+ * encryption.
+ * @len:   Length of data to encrypt in @page and encrypted
+ * data in returned page.
+ * @offs:  Offset of data within @page and returned
+ * page holding encrypted data.
+ * @lblk_num:  Logical block number. This must be unique for multiple
+ * calls with same inode, except when overwriting
+ * previously written data.
+ * @gfp_flags: The gfp flag for memory allocation
  *
- * Encrypts plaintext_page using the ctx encryption context. If
- * the filesystem supports it, encryption is performed in-place, otherwise a
- * new ciphertext_page is allocated and returned.
+ * Encrypts @page using the ctx encryption context. Performs encryption
+ * either in-place or into a newly allocated bounce page.
+ * Called on the page write path.
  *
- * Called on the page write path.  The caller must call
+ * Bounce page allocation is the default.
+ * In this case, the contents of @page are encrypted and stored in an
+ * allocated bounce page. @page has to be locked and the caller must call
  * fscrypt_restore_control_page() on the returned ciphertext page to
  * release the bounce buffer and the encryption context.
  *
- * Return: An allocated page with the encrypted content on success. Else, an
+ * In-place encryption is used by setting the FS_CFLG_INPLACE_ENCRYPTION flag 
in
+ * fscrypt_operations. Here, the input-page is returned with its content
+ * encrypted.
+ *
+ * Return: A page with the encrypted content on success. Else, an
  * error value or NULL.
  */
 struct page *fscrypt_encrypt_page(const struct inode *inode,
-   struct page *plaintext_page,
-   unsigned int plaintext_len,
-   unsigned int plaintext_offset,
-   pgoff_t index, gfp_t gfp_flags)
+   struct page *page,
+   unsigned int len,
+   unsigned int offs,
+   u64 lblk_num, gfp_t gfp_flags)
 
 {
struct fscrypt

[PATCH v2 0/6] UBIFS related fscrypt updates

[David Gstir]

This series applies on top of Ted's fscrypt tree[0] addresses the review
comments from Eric.

Changes since v1:
- Never allocate fscrypt_ctx in "own pages" mode
- Fix code comments
- Fix issues reported by checkpatch.pl

[0] git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git fscrypt

David Gstir (6):
  fscrypt: Use correct index in decrypt path.
  fscrypt: Never allocate fscrypt_ctx on in-place encryption
  fscrypt: Cleanup fscrypt_{decrypt,encrypt}_page()
  fscrypt: Cleanup page locking requirements for
fscrypt_{decrypt,encrypt}_page()
  fscrypt: Delay bounce page pool allocation until needed
  fscrypt: Rename FS_WRITE_PATH_FL to FS_CTX_HAS_BOUNCE_BUFFER_FL

 fs/crypto/crypto.c   | 135 ---
 fs/crypto/keyinfo.c  |   2 +-
 fs/ext4/inode.c  |   1 -
 fs/f2fs/data.c   |   1 -
 include/linux/fscrypto.h |  14 ++---
 5 files changed, 90 insertions(+), 63 deletions(-)

-- 
2.10.1

[PATCH v2 3/6] fscrypt: Cleanup fscrypt_{decrypt,encrypt}_page()

[David Gstir]

- Improve documentation
- Add BUG_ON(len == 0) to avoid accidental switch of offs and len
parameters
- Improve variable names for readability

Signed-off-by: David Gstir 
---
 fs/crypto/crypto.c   | 93 +++-
 include/linux/fscrypto.h |  8 ++---
 2 files changed, 56 insertions(+), 45 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 8536ac0b50d3..8805ec5d7de2 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -147,9 +147,9 @@ typedef enum {
 } fscrypt_direction_t;
 
 static int do_page_crypto(const struct inode *inode,
-   fscrypt_direction_t rw, pgoff_t index,
+   fscrypt_direction_t rw, u64 lblk_num,
struct page *src_page, struct page *dest_page,
-   unsigned int src_len, unsigned int src_offset,
+   unsigned int len, unsigned int offs,
gfp_t gfp_flags)
 {
struct {
@@ -163,6 +163,8 @@ static int do_page_crypto(const struct inode *inode,
struct crypto_skcipher *tfm = ci->ci_ctfm;
int res = 0;
 
+   BUG_ON(len == 0);
+
req = skcipher_request_alloc(tfm, gfp_flags);
if (!req) {
printk_ratelimited(KERN_ERR
@@ -176,14 +178,14 @@ static int do_page_crypto(const struct inode *inode,
page_crypt_complete, );
 
BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE);
-   xts_tweak.index = cpu_to_le64(index);
+   xts_tweak.index = cpu_to_le64(lblk_num);
memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding));
 
sg_init_table(, 1);
-   sg_set_page(, dest_page, src_len, src_offset);
+   sg_set_page(, dest_page, len, offs);
sg_init_table(, 1);
-   sg_set_page(, src_page, src_len, src_offset);
-   skcipher_request_set_crypt(req, , , src_len, _tweak);
+   sg_set_page(, src_page, len, offs);
+   skcipher_request_set_crypt(req, , , len, _tweak);
if (rw == FS_DECRYPT)
res = crypto_skcipher_decrypt(req);
else
@@ -214,44 +216,53 @@ static struct page *alloc_bounce_page(struct fscrypt_ctx 
*ctx, gfp_t gfp_flags)
 
 /**
  * fscypt_encrypt_page() - Encrypts a page
- * @inode:The inode for which the encryption should take place
- * @plaintext_page:   The page to encrypt. Must be locked.
- * @plaintext_len:Length of plaintext within page
- * @plaintext_offset: Offset of plaintext within page
- * @index:Index for encryption. This is mainly the page index, but
- *but might be different for multiple calls on same page.
- * @gfp_flags:The gfp flag for memory allocation
+ * @inode: The inode for which the encryption should take place
+ * @page:  The page to encrypt. Must be locked for bounce-page
+ * encryption.
+ * @len:   Length of data to encrypt in @page and encrypted
+ * data in returned page.
+ * @offs:  Offset of data within @page and returned
+ * page holding encrypted data.
+ * @lblk_num:  Logical block number. This must be unique for multiple
+ * calls with same inode, except when overwriting
+ * previously written data.
+ * @gfp_flags: The gfp flag for memory allocation
  *
- * Encrypts plaintext_page using the ctx encryption context. If
- * the filesystem supports it, encryption is performed in-place, otherwise a
- * new ciphertext_page is allocated and returned.
+ * Encrypts @page using the ctx encryption context. Performs encryption
+ * either in-place or into a newly allocated bounce page.
+ * Called on the page write path.
  *
- * Called on the page write path.  The caller must call
+ * Bounce page allocation is the default.
+ * In this case, the contents of @page are encrypted and stored in an
+ * allocated bounce page. @page has to be locked and the caller must call
  * fscrypt_restore_control_page() on the returned ciphertext page to
  * release the bounce buffer and the encryption context.
  *
- * Return: An allocated page with the encrypted content on success. Else, an
+ * In-place encryption is used by setting the FS_CFLG_INPLACE_ENCRYPTION flag 
in
+ * fscrypt_operations. Here, the input-page is returned with its content
+ * encrypted.
+ *
+ * Return: A page with the encrypted content on success. Else, an
  * error value or NULL.
  */
 struct page *fscrypt_encrypt_page(const struct inode *inode,
-   struct page *plaintext_page,
-   unsigned int plaintext_len,
-   unsigned int plaintext_offset,
-   pgoff_t index, gfp_t gfp_flags)
+   struct page *page,
+   unsigned int len,
+   unsigned int offs,
+   u64 lblk_num, gfp_t gfp_flags)
 
 {
struct fscrypt_ctx *ctx;
-   struct p

[PATCH v2 4/6] fscrypt: Cleanup page locking requirements for fscrypt_{decrypt,encrypt}_page()

[David Gstir]

Rename the FS_CFLG_INPLACE_ENCRYPTION flag to FS_CFLG_OWN_PAGES which,
when set, indicates that the fs uses pages under its own control as
opposed to writeback pages which require locking and a bounce buffer for
encryption.

Signed-off-by: David Gstir 
---
 fs/crypto/crypto.c   | 11 ---
 fs/ext4/inode.c  |  1 -
 fs/f2fs/data.c   |  1 -
 include/linux/fscrypto.h |  2 +-
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 8805ec5d7de2..6f5c3a8df70b 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -238,7 +238,7 @@ static struct page *alloc_bounce_page(struct fscrypt_ctx 
*ctx, gfp_t gfp_flags)
  * fscrypt_restore_control_page() on the returned ciphertext page to
  * release the bounce buffer and the encryption context.
  *
- * In-place encryption is used by setting the FS_CFLG_INPLACE_ENCRYPTION flag 
in
+ * In-place encryption is used by setting the FS_CFLG_OWN_PAGES flag in
  * fscrypt_operations. Here, the input-page is returned with its content
  * encrypted.
  *
@@ -258,7 +258,7 @@ struct page *fscrypt_encrypt_page(const struct inode *inode,
 
BUG_ON(len % FS_CRYPTO_BLOCK_SIZE != 0);
 
-   if (inode->i_sb->s_cop->flags & FS_CFLG_INPLACE_ENCRYPTION) {
+   if (inode->i_sb->s_cop->flags & FS_CFLG_OWN_PAGES) {
/* with inplace-encryption we just encrypt the page */
err = do_page_crypto(inode, FS_ENCRYPT, lblk_num,
page, ciphertext_page,
@@ -269,6 +269,8 @@ struct page *fscrypt_encrypt_page(const struct inode *inode,
return ciphertext_page;
}
 
+   BUG_ON(!PageLocked(page));
+
ctx = fscrypt_get_ctx(inode, gfp_flags);
if (IS_ERR(ctx))
return (struct page *)ctx;
@@ -301,7 +303,7 @@ EXPORT_SYMBOL(fscrypt_encrypt_page);
  * fscrypt_decrypt_page() - Decrypts a page in-place
  * @inode: The corresponding inode for the page to decrypt.
  * @page:  The page to decrypt. Must be locked in case
- * it is a writeback page.
+ * it is a writeback page (FS_CFLG_OWN_PAGES unset).
  * @len:   Number of bytes in @page to be decrypted.
  * @offs:  Start of data in @page.
  * @lblk_num:  Logical block number.
@@ -315,6 +317,9 @@ EXPORT_SYMBOL(fscrypt_encrypt_page);
 int fscrypt_decrypt_page(const struct inode *inode, struct page *page,
unsigned int len, unsigned int offs, u64 lblk_num)
 {
+   if (!(inode->i_sb->s_cop->flags & FS_CFLG_OWN_PAGES))
+   BUG_ON(!PageLocked(page));
+
return do_page_crypto(inode, FS_DECRYPT, lblk_num, page, page, len,
offs, GFP_NOFS);
 }
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 1485ac273bfb..fb2b514f675b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3744,7 +3744,6 @@ static int __ext4_block_zero_page_range(handle_t *handle,
/* We expect the key to be set. */
BUG_ON(!fscrypt_has_encryption_key(inode));
BUG_ON(blocksize != PAGE_SIZE);
-   BUG_ON(!PageLocked(page));
WARN_ON_ONCE(fscrypt_decrypt_page(page->mapping->host,
page, PAGE_SIZE, 0, 
page->index));
}
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 435590c4b341..9f0ba90b92e4 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1194,7 +1194,6 @@ int do_write_data_page(struct f2fs_io_info *fio)
f2fs_wait_on_encrypted_page_writeback(F2FS_I_SB(inode),
fio->old_blkaddr);
 retry_encrypt:
-   BUG_ON(!PageLocked(fio->page));
fio->encrypted_page = fscrypt_encrypt_page(inode, fio->page,
PAGE_SIZE, 0,
fio->page->index,
diff --git a/include/linux/fscrypto.h b/include/linux/fscrypto.h
index e9648b62eca6..65be74f3afb6 100644
--- a/include/linux/fscrypto.h
+++ b/include/linux/fscrypto.h
@@ -156,7 +156,7 @@ struct fscrypt_name {
 /*
  * fscrypt superblock flags
  */
-#define FS_CFLG_INPLACE_ENCRYPTION (1U << 1)
+#define FS_CFLG_OWN_PAGES (1U << 1)
 
 /*
  * crypto opertions for filesystems
-- 
2.10.1

[PATCH v2 1/6] fscrypt: Use correct index in decrypt path.

[David Gstir]

Actually use the fs-provided index instead of always using page->index
which is only set for page-cache pages.

Fixes: 9c4bb8a3a9b4 ("fscrypt: Let fs select encryption index/tweak")

Signed-off-by: David Gstir <da...@sigma-star.at>
---
 fs/crypto/crypto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index b6029785714c..3c1124beae28 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -296,7 +296,7 @@ EXPORT_SYMBOL(fscrypt_encrypt_page);
 int fscrypt_decrypt_page(const struct inode *inode, struct page *page,
unsigned int len, unsigned int offs, pgoff_t index)
 {
-   return do_page_crypto(inode, FS_DECRYPT, page->index, page, page, len, 
offs,
+   return do_page_crypto(inode, FS_DECRYPT, index, page, page, len, offs,
GFP_NOFS);
 }
 EXPORT_SYMBOL(fscrypt_decrypt_page);
-- 
2.10.1

[PATCH v2 6/6] fscrypt: Rename FS_WRITE_PATH_FL to FS_CTX_HAS_BOUNCE_BUFFER_FL

[David Gstir]

... to better explain its purpose after introducing in-place encryption
without bounce buffer.

Signed-off-by: David Gstir <da...@sigma-star.at>
---
 fs/crypto/crypto.c   | 6 +++---
 include/linux/fscrypto.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index c1e4316045e9..20dd4402af42 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -63,7 +63,7 @@ void fscrypt_release_ctx(struct fscrypt_ctx *ctx)
 {
unsigned long flags;
 
-   if (ctx->flags & FS_WRITE_PATH_FL && ctx->w.bounce_page) {
+   if (ctx->flags & FS_CTX_HAS_BOUNCE_BUFFER_FL && ctx->w.bounce_page) {
mempool_free(ctx->w.bounce_page, fscrypt_bounce_page_pool);
ctx->w.bounce_page = NULL;
}
@@ -121,7 +121,7 @@ struct fscrypt_ctx *fscrypt_get_ctx(const struct inode 
*inode, gfp_t gfp_flags)
} else {
ctx->flags &= ~FS_CTX_REQUIRES_FREE_ENCRYPT_FL;
}
-   ctx->flags &= ~FS_WRITE_PATH_FL;
+   ctx->flags &= ~FS_CTX_HAS_BOUNCE_BUFFER_FL;
return ctx;
 }
 EXPORT_SYMBOL(fscrypt_get_ctx);
@@ -210,7 +210,7 @@ static struct page *alloc_bounce_page(struct fscrypt_ctx 
*ctx, gfp_t gfp_flags)
ctx->w.bounce_page = mempool_alloc(fscrypt_bounce_page_pool, gfp_flags);
if (ctx->w.bounce_page == NULL)
return ERR_PTR(-ENOMEM);
-   ctx->flags |= FS_WRITE_PATH_FL;
+   ctx->flags |= FS_CTX_HAS_BOUNCE_BUFFER_FL;
return ctx->w.bounce_page;
 }
 
diff --git a/include/linux/fscrypto.h b/include/linux/fscrypto.h
index 6039c86ff849..00aa834bf2d9 100644
--- a/include/linux/fscrypto.h
+++ b/include/linux/fscrypto.h
@@ -84,7 +84,7 @@ struct fscrypt_info {
 };
 
 #define FS_CTX_REQUIRES_FREE_ENCRYPT_FL0x0001
-#define FS_WRITE_PATH_FL   0x0002
+#define FS_CTX_HAS_BOUNCE_BUFFER_FL0x0002
 
 struct fscrypt_ctx {
union {
-- 
2.10.1

[PATCH v2 1/6] fscrypt: Use correct index in decrypt path.

[David Gstir]

Actually use the fs-provided index instead of always using page->index
which is only set for page-cache pages.

Fixes: 9c4bb8a3a9b4 ("fscrypt: Let fs select encryption index/tweak")

Signed-off-by: David Gstir 
---
 fs/crypto/crypto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index b6029785714c..3c1124beae28 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -296,7 +296,7 @@ EXPORT_SYMBOL(fscrypt_encrypt_page);
 int fscrypt_decrypt_page(const struct inode *inode, struct page *page,
unsigned int len, unsigned int offs, pgoff_t index)
 {
-   return do_page_crypto(inode, FS_DECRYPT, page->index, page, page, len, 
offs,
+   return do_page_crypto(inode, FS_DECRYPT, index, page, page, len, offs,
GFP_NOFS);
 }
 EXPORT_SYMBOL(fscrypt_decrypt_page);
-- 
2.10.1

[PATCH v2 6/6] fscrypt: Rename FS_WRITE_PATH_FL to FS_CTX_HAS_BOUNCE_BUFFER_FL

[David Gstir]

... to better explain its purpose after introducing in-place encryption
without bounce buffer.

Signed-off-by: David Gstir 
---
 fs/crypto/crypto.c   | 6 +++---
 include/linux/fscrypto.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index c1e4316045e9..20dd4402af42 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -63,7 +63,7 @@ void fscrypt_release_ctx(struct fscrypt_ctx *ctx)
 {
unsigned long flags;
 
-   if (ctx->flags & FS_WRITE_PATH_FL && ctx->w.bounce_page) {
+   if (ctx->flags & FS_CTX_HAS_BOUNCE_BUFFER_FL && ctx->w.bounce_page) {
mempool_free(ctx->w.bounce_page, fscrypt_bounce_page_pool);
ctx->w.bounce_page = NULL;
}
@@ -121,7 +121,7 @@ struct fscrypt_ctx *fscrypt_get_ctx(const struct inode 
*inode, gfp_t gfp_flags)
} else {
ctx->flags &= ~FS_CTX_REQUIRES_FREE_ENCRYPT_FL;
}
-   ctx->flags &= ~FS_WRITE_PATH_FL;
+   ctx->flags &= ~FS_CTX_HAS_BOUNCE_BUFFER_FL;
return ctx;
 }
 EXPORT_SYMBOL(fscrypt_get_ctx);
@@ -210,7 +210,7 @@ static struct page *alloc_bounce_page(struct fscrypt_ctx 
*ctx, gfp_t gfp_flags)
ctx->w.bounce_page = mempool_alloc(fscrypt_bounce_page_pool, gfp_flags);
if (ctx->w.bounce_page == NULL)
return ERR_PTR(-ENOMEM);
-   ctx->flags |= FS_WRITE_PATH_FL;
+   ctx->flags |= FS_CTX_HAS_BOUNCE_BUFFER_FL;
return ctx->w.bounce_page;
 }
 
diff --git a/include/linux/fscrypto.h b/include/linux/fscrypto.h
index 6039c86ff849..00aa834bf2d9 100644
--- a/include/linux/fscrypto.h
+++ b/include/linux/fscrypto.h
@@ -84,7 +84,7 @@ struct fscrypt_info {
 };
 
 #define FS_CTX_REQUIRES_FREE_ENCRYPT_FL0x0001
-#define FS_WRITE_PATH_FL   0x0002
+#define FS_CTX_HAS_BOUNCE_BUFFER_FL0x0002
 
 struct fscrypt_ctx {
union {
-- 
2.10.1

Re: [PATCH 3/6] fscrypt: Cleanup fscrypt_{decrypt,encrypt}_page()

[David Gstir]

> On 02.12.2016, at 09:19, Eric Biggers  wrote:
> 
> On Thu, Dec 01, 2016 at 11:14:55PM +0100, Richard Weinberger wrote:
>> + * @lblk_num:  Logical block number. This must be unique for multiple
>> + * calls with same page.
> 
> Must be unique for all calls with the same *inode*, except when overwriting
> previously written data.

Yes, of course! Will fix that.

Thanks,
David

Re: [PATCH 3/6] fscrypt: Cleanup fscrypt_{decrypt,encrypt}_page()

[David Gstir]

> On 02.12.2016, at 09:19, Eric Biggers  wrote:
> 
> On Thu, Dec 01, 2016 at 11:14:55PM +0100, Richard Weinberger wrote:
>> + * @lblk_num:  Logical block number. This must be unique for multiple
>> + * calls with same page.
> 
> Must be unique for all calls with the same *inode*, except when overwriting
> previously written data.

Yes, of course! Will fix that.

Thanks,
David

Re: [PATCH 01/29] fscrypt: Add in-place encryption mode

[David Gstir]

Eric,

> On 15.11.2016, at 19:14, Eric Biggers <ebigg...@google.com> wrote:
> 
> Hi,
> 
> On Sun, Nov 13, 2016 at 10:20:44PM +0100, Richard Weinberger wrote:
>> From: David Gstir <da...@sigma-star.at>
>> 
>> ext4 and f2fs require a bounce page when encrypting pages. However, not
>> all filesystems will need that (eg. UBIFS). This is handled via a
>> flag on fscrypt_operations where a fs implementation can select in-place
>> encryption over using a bounce page (which is the default).
>> 
>> Signed-off-by: David Gstir <da...@sigma-star.at>
>> Signed-off-by: Richard Weinberger <rich...@nod.at>
> 
> The comment for fscrypt_encrypt_page() still says the following:
> 
> * Called on the page write path.  The caller must call
> * fscrypt_restore_control_page() on the returned ciphertext page to
> * release the bounce buffer and the encryption context.
> 
> It seems this isn't correct anymore.  

Yes, this is not true in all cases anymore. Will fix that.

> It also looks like the fscrypt_context
> never gets released in the case where the page is encrypted in-place.

You're right. I've already fixed that locally and will include it in the next 
patch set.

> Additionally, after this change the name of the flag FS_WRITE_PATH_FL is
> misleading, since it now really indicates the presence of a bounce buffer 
> rather
> than the "write path".

I can see no use case for FS_WRITE_PATH_FL other than to indicate that the 
bounce buffer has to be free'd. Is there any reason why we should not just 
remove it and check the presence of a bounce buffer by a simple "if 
(ctx->w.bounce_page)" ?

Thanks,
David

Re: [PATCH 01/29] fscrypt: Add in-place encryption mode

[David Gstir]

Eric,

> On 15.11.2016, at 19:14, Eric Biggers  wrote:
> 
> Hi,
> 
> On Sun, Nov 13, 2016 at 10:20:44PM +0100, Richard Weinberger wrote:
>> From: David Gstir 
>> 
>> ext4 and f2fs require a bounce page when encrypting pages. However, not
>> all filesystems will need that (eg. UBIFS). This is handled via a
>> flag on fscrypt_operations where a fs implementation can select in-place
>> encryption over using a bounce page (which is the default).
>> 
>> Signed-off-by: David Gstir 
>> Signed-off-by: Richard Weinberger 
> 
> The comment for fscrypt_encrypt_page() still says the following:
> 
> * Called on the page write path.  The caller must call
> * fscrypt_restore_control_page() on the returned ciphertext page to
> * release the bounce buffer and the encryption context.
> 
> It seems this isn't correct anymore.  

Yes, this is not true in all cases anymore. Will fix that.

> It also looks like the fscrypt_context
> never gets released in the case where the page is encrypted in-place.

You're right. I've already fixed that locally and will include it in the next 
patch set.

> Additionally, after this change the name of the flag FS_WRITE_PATH_FL is
> misleading, since it now really indicates the presence of a bounce buffer 
> rather
> than the "write path".

I can see no use case for FS_WRITE_PATH_FL other than to indicate that the 
bounce buffer has to be free'd. Is there any reason why we should not just 
remove it and check the presence of a bounce buffer by a simple "if 
(ctx->w.bounce_page)" ?

Thanks,
David

Re: [PATCH 02/29] fscrypt: Allow fscrypt_decrypt_page() to function with non-writeback pages

[David Gstir]

Eric,

> On 15.11.2016, at 19:19, Eric Biggers  wrote:
> 
> On Sun, Nov 13, 2016 at 10:20:45PM +0100, Richard Weinberger wrote:
>> /**
>>  * f2crypt_decrypt_page() - Decrypts a page in-place
>> - * @page: The page to decrypt. Must be locked.
>> + * @inode: The encrypted inode to decrypt.
>> + * @page:  The page to decrypt. Must be locked.
> 
> Strictly speaking, it's not the inode itself being decrypted, but rather the
> data associated with it.  Could this be better expressed as something like
> "The inode to which the page belongs"?

Yes, you're right. Will address that!

Thanks,
David

Re: [PATCH 02/29] fscrypt: Allow fscrypt_decrypt_page() to function with non-writeback pages

[David Gstir]

Eric,

> On 15.11.2016, at 19:19, Eric Biggers  wrote:
> 
> On Sun, Nov 13, 2016 at 10:20:45PM +0100, Richard Weinberger wrote:
>> /**
>>  * f2crypt_decrypt_page() - Decrypts a page in-place
>> - * @page: The page to decrypt. Must be locked.
>> + * @inode: The encrypted inode to decrypt.
>> + * @page:  The page to decrypt. Must be locked.
> 
> Strictly speaking, it's not the inode itself being decrypted, but rather the
> data associated with it.  Could this be better expressed as something like
> "The inode to which the page belongs"?

Yes, you're right. Will address that!

Thanks,
David

Re: [PATCH] UBIFS: Fix possible memory leak in ubifs_readdir()

[David Gstir]

> On 12.10.2015, at 23:35, Richard Weinberger  wrote:
> 
> If ubifs_tnc_next_ent() returns something else than -ENOENT
> we leak file->private_data.
> 
> Signed-off-by: Richard Weinberger 
> ---
> fs/ubifs/dir.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 5c27c66..cb88ea3 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -449,13 +449,14 @@ static int ubifs_readdir(struct file *file, struct 
> dir_context *ctx)
>   }
> 
> out:
> + kfree(file->private_data);
> + file->private_data = NULL;
> +
>   if (err != -ENOENT) {
>   ubifs_err(c, "cannot find next direntry, error %d", err);
>   return err;
>   }
> 
> - kfree(file->private_data);
> - file->private_data = NULL;
>   /* 2 is a special value indicating that there are no more direntries */
>   ctx->pos = 2;
>   return 0;
> -- 
> 2.5.0

Looks good to me.

Reviewed-by: David Gstir  

Thanks,
David--
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] UBIFS: Fix possible memory leak in ubifs_readdir()

[David Gstir]

> On 12.10.2015, at 23:35, Richard Weinberger <rich...@nod.at> wrote:
> 
> If ubifs_tnc_next_ent() returns something else than -ENOENT
> we leak file->private_data.
> 
> Signed-off-by: Richard Weinberger <rich...@nod.at>
> ---
> fs/ubifs/dir.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 5c27c66..cb88ea3 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -449,13 +449,14 @@ static int ubifs_readdir(struct file *file, struct 
> dir_context *ctx)
>   }
> 
> out:
> + kfree(file->private_data);
> + file->private_data = NULL;
> +
>   if (err != -ENOENT) {
>   ubifs_err(c, "cannot find next direntry, error %d", err);
>   return err;
>   }
> 
> - kfree(file->private_data);
> - file->private_data = NULL;
>   /* 2 is a special value indicating that there are no more direntries */
>   ctx->pos = 2;
>   return 0;
> -- 
> 2.5.0

Looks good to me.

Reviewed-by: David Gstir <da...@sigma-star.at> 

Thanks,
David--
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] UBI: Validate data_size

[David Gstir]

> On 22.09.2015, at 23:58, Richard Weinberger  wrote:
> 
> Make sure that data_size is less than LEB size.
> Otherwise a handcrafted UBI image is able to trigger
> an out of bounds memory access in ubi_compare_lebs().
> 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Richard Weinberger 
> ---
> drivers/mtd/ubi/io.c | 5 +
> 1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> index 5bbd1f0..1fc23e4 100644
> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -926,6 +926,11 @@ static int validate_vid_hdr(const struct ubi_device *ubi,
>   goto bad;
>   }
> 
> + if (data_size > ubi->leb_size) {
> + ubi_err(ubi, "bad data_size");
> + goto bad;
> + }
> +

Nice catch!

Reviewed-by: David Gstir --
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] UBI: Validate data_size

[David Gstir]

> On 22.09.2015, at 23:58, Richard Weinberger <rich...@nod.at> wrote:
> 
> Make sure that data_size is less than LEB size.
> Otherwise a handcrafted UBI image is able to trigger
> an out of bounds memory access in ubi_compare_lebs().
> 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Richard Weinberger <rich...@nod.at>
> ---
> drivers/mtd/ubi/io.c | 5 +
> 1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> index 5bbd1f0..1fc23e4 100644
> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -926,6 +926,11 @@ static int validate_vid_hdr(const struct ubi_device *ubi,
>   goto bad;
>   }
> 
> + if (data_size > ubi->leb_size) {
> + ubi_err(ubi, "bad data_size");
> + goto bad;
> + }
> +

Nice catch!

Reviewed-by: David Gstir <da...@sigma-star.at>--
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: DMAR faults from unrelated device when vfio is used

[David Gstir]

Am Montag, den 04.02.2013, 08:49 -0700 schrieb Alex Williamson:

> Can you clarify what you mean by assign?  Are you actually assigning the
> root ports to the qemu guest (1c.0 & 1c.6)?  vfio will require they be
> owned by vfio-pci to make use of 3:00.0, but assigning them to the guest
> is not recommended.  Can you provided your qemu command line?  

I did hand all of them to the guest OS. Removing 1c.0 & 1c.6 from the qemu 
command line seems to have done the trick. Thanks!

Here's my working qemu command line:
qemu-kvm -no-reboot -enable-kvm -cpu host -smp 4 -m 6G \
  -drive 
file=/home/test/qemu/images/win7_base_updated.qcow2,if=virtio,cache=none,media=disk,format=qcow2,index=0
 \
  -full-screen -no-quit -no-frame -display sdl -vnc :1 -k de -usbdevice tablet \
  -vga std -global VGA.vgamem_mb=256 \
  -netdev tap,id=guest0,ifname=tap0,script=no,downscript=no \
  -net nic,netdev=guest0,model=virtio,macaddr=00:16:35:BE:EF:12  \
  -rtc base=localtime \
  -device vfio-pci,host=00:1b.0,id=audio \
  -device vfio-pci,host=00:1a.0,id=ehci1 \
  -device vfio-pci,host=00:1d.0,id=ehci2 \
  -device vfio-pci,host=03:00.0,id=xhci1 \
  -monitor tcp::,server,nowait


> We need
> to re-visit how to handle pcieport devices with vfio-pci, perhaps
> white-listing it as a vfio "compatible" driver, but this still should
> not interfere with devices external to the group.
> 
> The DMAR fault address looks pretty bogus unless you happen to have
> 100GB+ of ram in the system.

Nope, definitely not. :)

> vfio makes use of the IOMMU API for programming DMA translations, so an
> reserved fields would have to be programmed by intel-iommu itself.  We
> could of course be passing some kind of bogus data that intel-iommu
> isn't catching.  If you're assigning the root ports to the guest, I'd
> start with that, don't do it.  Attach them to vfio, but don't give them
> to the guest.  Maybe that'll give us a hint.  I also notice that your
> USB 3 controller is dead:
> 
> 03:00.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host Controller 
> (rev ff) (prog-if ff)
>   !!! Unknown header type 7f
> 
> We only see unknown header type 7f when the read from the device returns
> -1.  This might have something to do with the root port above it (1c.6)
> being in state D3.  Windows likes to put unused devices in D3, which
> leads me to suspect you are giving it to the guest.  

There error does no longer occur. lspci now shows this:

-- snip --
03:00.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host Controller (rev 
04) (prog-if 30 [XHCI])
Subsystem: Intel Corporation Device 2008
Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR+ FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- SERR- http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: DMAR faults from unrelated device when vfio is used

[David Gstir]

Am Montag, den 04.02.2013, 08:49 -0700 schrieb Alex Williamson:

 Can you clarify what you mean by assign?  Are you actually assigning the
 root ports to the qemu guest (1c.0  1c.6)?  vfio will require they be
 owned by vfio-pci to make use of 3:00.0, but assigning them to the guest
 is not recommended.  Can you provided your qemu command line?  

I did hand all of them to the guest OS. Removing 1c.0  1c.6 from the qemu 
command line seems to have done the trick. Thanks!

Here's my working qemu command line:
qemu-kvm -no-reboot -enable-kvm -cpu host -smp 4 -m 6G \
  -drive 
file=/home/test/qemu/images/win7_base_updated.qcow2,if=virtio,cache=none,media=disk,format=qcow2,index=0
 \
  -full-screen -no-quit -no-frame -display sdl -vnc :1 -k de -usbdevice tablet \
  -vga std -global VGA.vgamem_mb=256 \
  -netdev tap,id=guest0,ifname=tap0,script=no,downscript=no \
  -net nic,netdev=guest0,model=virtio,macaddr=00:16:35:BE:EF:12  \
  -rtc base=localtime \
  -device vfio-pci,host=00:1b.0,id=audio \
  -device vfio-pci,host=00:1a.0,id=ehci1 \
  -device vfio-pci,host=00:1d.0,id=ehci2 \
  -device vfio-pci,host=03:00.0,id=xhci1 \
  -monitor tcp::,server,nowait


 We need
 to re-visit how to handle pcieport devices with vfio-pci, perhaps
 white-listing it as a vfio compatible driver, but this still should
 not interfere with devices external to the group.
 
 The DMAR fault address looks pretty bogus unless you happen to have
 100GB+ of ram in the system.

Nope, definitely not. :)

 vfio makes use of the IOMMU API for programming DMA translations, so an
 reserved fields would have to be programmed by intel-iommu itself.  We
 could of course be passing some kind of bogus data that intel-iommu
 isn't catching.  If you're assigning the root ports to the guest, I'd
 start with that, don't do it.  Attach them to vfio, but don't give them
 to the guest.  Maybe that'll give us a hint.  I also notice that your
 USB 3 controller is dead:
 
 03:00.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host Controller 
 (rev ff) (prog-if ff)
   !!! Unknown header type 7f
 
 We only see unknown header type 7f when the read from the device returns
 -1.  This might have something to do with the root port above it (1c.6)
 being in state D3.  Windows likes to put unused devices in D3, which
 leads me to suspect you are giving it to the guest.  

There error does no longer occur. lspci now shows this:

-- snip --
03:00.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host Controller (rev 
04) (prog-if 30 [XHCI])
Subsystem: Intel Corporation Device 2008
Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR+ FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast TAbort- TAbort- 
MAbort+ SERR- PERR- INTx-
Interrupt: pin A routed to IRQ 18
Region 0: Memory at fe50 (64-bit, non-prefetchable) [disabled] 
[size=8K]
Capabilities: [50] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA 
PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D3 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [70] MSI: Enable- Count=1/8 Maskable- 64bit+
Address:   Data: 
Capabilities: [90] MSI-X: Enable- Count=8 Masked-
Vector table: BAR=0 offset=1000
PBA: BAR=0 offset=1080
Capabilities: [a0] Express (v2) Endpoint, MSI 00
DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s 
unlimited, L1 unlimited
ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset-
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- 
Unsupported-
RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop+
MaxPayload 128 bytes, MaxReadReq 128 bytes
DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ 
TransPend-
LnkCap: Port #0, Speed 5GT/s, Width x1, ASPM L0s L1, Latency L0 
4us, L1 unlimited
ClockPM+ Surprise- LLActRep- BwNot-
LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk+
ExtSynch- ClockPM+ AutWidDis- BWInt- AutBWInt-
LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ 
DLActive- BWMgmt- ABWMgmt-
DevCap2: Completion Timeout: Not Supported, TimeoutDis+
DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-
LnkCtl2: Target Link Speed: 5GT/s, EnterCompliance- SpeedDis-, 
Selectable De-emphasis: -6dB
 Transmit Margin: Normal Operating Range, 
EnterModifiedCompliance- ComplianceSOS-
 Compliance De-emphasis: -6dB
LnkSta2: Current De-emphasis Level: -3.5dB, 
EqualizationComplete-, EqualizationPhase1-
 EqualizationPhase2-, EqualizationPhase3-, 
LinkEqualizationRequest-
Capabilities: [100 

DMAR faults from unrelated device when vfio is used

[David Gstir]

Hi!

I get the following error messages over and over again when using vfio in 
qemu-kvm:

[ 1692.021403] dmar: DMAR:[DMA Read] Request device [00:02.0] fault addr 
1a45aa9000 
[ 1692.021403] DMAR:[fault reason 12] non-zero reserved fields in PTE
[ 1692.021416] dmar: DRHD: handling fault status reg 2

This pci device is the graphics card, which I did not assign to qemu! I did 
assign the following devices:
00:1a.0, 00:1b.0, 00:1c.0, 00:1c.6, 00:1d.0, 03:00.0.

The error occurs at random and is not reproducible every time. It happens about 
every third reboot. 
I'm running qemu-kvm 1.3.0 (kvm-1.3.0-187.3), kernel 3.8.0-rc5 and windows 7 as 
guest OS. The hardware uses an Intel IOMMU. See attachments for output of 
lspci, and details on iommu groups

I'm not sure if this problem originates from qemu, kvm, vfio or the GPU driver.
Do you have any hints how to debug this further?


Thanks,
David

PS: please cc me, I'm not subscribed.
# /sbin/lspci -nn
00:00.0 Host bridge [0600]: Intel Corporation 2nd Generation Core Processor 
Family DRAM Controller [8086:0100] (rev 09)
00:01.0 PCI bridge [0604]: Intel Corporation Xeon E3-1200/2nd Generation Core 
Processor Family PCI Express Root Port [8086:0101] (rev 09)
00:02.0 VGA compatible controller [0300]: Intel Corporation 2nd Generation Core 
Processor Family Integrated Graphics Controller [8086:0102] (rev 09)
00:16.0 Communication controller [0780]: Intel Corporation 6 Series/C200 Series 
Chipset Family MEI Controller #1 [8086:1c3a] (rev 04)
00:16.2 IDE interface [0101]: Intel Corporation 6 Series/C200 Series Chipset 
Family IDE-r Controller [8086:1c3c] (rev 04)
00:16.3 Serial controller [0700]: Intel Corporation 6 Series/C200 Series 
Chipset Family KT Controller [8086:1c3d] (rev 04)
00:19.0 Ethernet controller [0200]: Intel Corporation 82579LM Gigabit Network 
Connection [8086:1502] (rev 04)
00:1a.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset 
Family USB Enhanced Host Controller #2 [8086:1c2d] (rev 04)
00:1b.0 Audio device [0403]: Intel Corporation 6 Series/C200 Series Chipset 
Family High Definition Audio Controller [8086:1c20] (rev 04)
00:1c.0 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset 
Family PCI Express Root Port 1 [8086:1c10] (rev b4)
00:1c.6 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset 
Family PCI Express Root Port 7 [8086:1c1c] (rev b4)
00:1d.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset 
Family USB Enhanced Host Controller #1 [8086:1c26] (rev 04)
00:1e.0 PCI bridge [0604]: Intel Corporation 82801 PCI Bridge [8086:244e] (rev 
a4)
00:1f.0 ISA bridge [0601]: Intel Corporation Q67 Express Chipset Family LPC 
Controller [8086:1c4e] (rev 04)
00:1f.2 SATA controller [0106]: Intel Corporation 6 Series/C200 Series Chipset 
Family SATA AHCI Controller [8086:1c02] (rev 04)
00:1f.3 SMBus [0c05]: Intel Corporation 6 Series/C200 Series Chipset Family 
SMBus Controller [8086:1c22] (rev 04)
03:00.0 USB controller [0c03]: NEC Corporation uPD720200 USB 3.0 Host 
Controller [1033:0194] (rev ff)




# lspci -vv
00:00.0 Host bridge: Intel Corporation 2nd Generation Core Processor Family 
DRAM Controller (rev 09)
Subsystem: Intel Corporation Device 2008
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- SERR- 

00:01.0 PCI bridge: Intel Corporation Xeon E3-1200/2nd Generation Core 
Processor Family PCI Express Root Port (rev 09) (prog-if 00 [Normal decode])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- SERR- TAbort- Reset- FastB2B-
PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
Capabilities: [88] Subsystem: Intel Corporation Device 2008
Capabilities: [80] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA 
PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [90] MSI: Enable+ Count=1/1 Maskable- 64bit-
Address: fee002d8  Data: 
Capabilities: [a0] Express (v2) Root Port (Slot+), MSI 00
DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s <64ns, 
L1 <1us
ExtTag- RBE+ FLReset-
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- 
Unsupported-
RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
MaxPayload 128 bytes, MaxReadReq 128 bytes
DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- 
TransPend-
LnkCap: Port #2, Speed 5GT/s, Width x16, ASPM L0s L1, Latency 
L0 <1us, L1 <4us
ClockPM- Surprise- LLActRep- BwNot+
LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk-
 

DMAR faults from unrelated device when vfio is used

[David Gstir]

Hi!

I get the following error messages over and over again when using vfio in 
qemu-kvm:

[ 1692.021403] dmar: DMAR:[DMA Read] Request device [00:02.0] fault addr 
1a45aa9000 
[ 1692.021403] DMAR:[fault reason 12] non-zero reserved fields in PTE
[ 1692.021416] dmar: DRHD: handling fault status reg 2

This pci device is the graphics card, which I did not assign to qemu! I did 
assign the following devices:
00:1a.0, 00:1b.0, 00:1c.0, 00:1c.6, 00:1d.0, 03:00.0.

The error occurs at random and is not reproducible every time. It happens about 
every third reboot. 
I'm running qemu-kvm 1.3.0 (kvm-1.3.0-187.3), kernel 3.8.0-rc5 and windows 7 as 
guest OS. The hardware uses an Intel IOMMU. See attachments for output of 
lspci, and details on iommu groups

I'm not sure if this problem originates from qemu, kvm, vfio or the GPU driver.
Do you have any hints how to debug this further?


Thanks,
David

PS: please cc me, I'm not subscribed.
# /sbin/lspci -nn
00:00.0 Host bridge [0600]: Intel Corporation 2nd Generation Core Processor 
Family DRAM Controller [8086:0100] (rev 09)
00:01.0 PCI bridge [0604]: Intel Corporation Xeon E3-1200/2nd Generation Core 
Processor Family PCI Express Root Port [8086:0101] (rev 09)
00:02.0 VGA compatible controller [0300]: Intel Corporation 2nd Generation Core 
Processor Family Integrated Graphics Controller [8086:0102] (rev 09)
00:16.0 Communication controller [0780]: Intel Corporation 6 Series/C200 Series 
Chipset Family MEI Controller #1 [8086:1c3a] (rev 04)
00:16.2 IDE interface [0101]: Intel Corporation 6 Series/C200 Series Chipset 
Family IDE-r Controller [8086:1c3c] (rev 04)
00:16.3 Serial controller [0700]: Intel Corporation 6 Series/C200 Series 
Chipset Family KT Controller [8086:1c3d] (rev 04)
00:19.0 Ethernet controller [0200]: Intel Corporation 82579LM Gigabit Network 
Connection [8086:1502] (rev 04)
00:1a.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset 
Family USB Enhanced Host Controller #2 [8086:1c2d] (rev 04)
00:1b.0 Audio device [0403]: Intel Corporation 6 Series/C200 Series Chipset 
Family High Definition Audio Controller [8086:1c20] (rev 04)
00:1c.0 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset 
Family PCI Express Root Port 1 [8086:1c10] (rev b4)
00:1c.6 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset 
Family PCI Express Root Port 7 [8086:1c1c] (rev b4)
00:1d.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset 
Family USB Enhanced Host Controller #1 [8086:1c26] (rev 04)
00:1e.0 PCI bridge [0604]: Intel Corporation 82801 PCI Bridge [8086:244e] (rev 
a4)
00:1f.0 ISA bridge [0601]: Intel Corporation Q67 Express Chipset Family LPC 
Controller [8086:1c4e] (rev 04)
00:1f.2 SATA controller [0106]: Intel Corporation 6 Series/C200 Series Chipset 
Family SATA AHCI Controller [8086:1c02] (rev 04)
00:1f.3 SMBus [0c05]: Intel Corporation 6 Series/C200 Series Chipset Family 
SMBus Controller [8086:1c22] (rev 04)
03:00.0 USB controller [0c03]: NEC Corporation uPD720200 USB 3.0 Host 
Controller [1033:0194] (rev ff)




# lspci -vv
00:00.0 Host bridge: Intel Corporation 2nd Generation Core Processor Family 
DRAM Controller (rev 09)
Subsystem: Intel Corporation Device 2008
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast TAbort- TAbort- 
MAbort+ SERR- PERR- INTx-
Latency: 0
Capabilities: [e0] Vendor Specific Information: Len=0c ?

00:01.0 PCI bridge: Intel Corporation Xeon E3-1200/2nd Generation Core 
Processor Family PCI Express Root Port (rev 09) (prog-if 00 [Normal decode])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast TAbort- TAbort- 
MAbort- SERR- PERR- INTx-
Latency: 0, Cache Line Size: 64 bytes
Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast TAbort- TAbort- 
MAbort- SERR- PERR-
BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- Reset- FastB2B-
PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
Capabilities: [88] Subsystem: Intel Corporation Device 2008
Capabilities: [80] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA 
PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [90] MSI: Enable+ Count=1/1 Maskable- 64bit-
Address: fee002d8  Data: 
Capabilities: [a0] Express (v2) Root Port (Slot+), MSI 00
DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s 64ns, 
L1 1us
ExtTag- RBE+ FLReset-
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- 
Unsupported-
RlxdOrd- ExtTag- PhantFunc- AuxPwr-