Linux-mtd Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] mtd: ubi: UBI Encryption
@ 2015-08-10 19:56 Andrew Murray
  2015-08-11  5:38 ` Timo Ketola
                   ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Andrew Murray @ 2015-08-10 19:56 UTC (permalink / raw)
  To: linux-mtd

We've recently implemented support for encryption within UBI for one of our
customers and now wish to use this experience to provide a suitable solution
for the community.

Our current implementation works on real hardware and the latest linux-mtd
kernel - however there are many issues that in my opinion make it unsuitable
for the wider community. I'm keen to address these issues with feedback from
linux-mtd such that we can get this in good shape. I'm happy to share the
current implementation if it helps form the basis of a discussion that will
address the general issues of adding encryption to UBI. (The diffstat for the
current implementation is about 407 insertions).

In summary:

 - The approach I've taken is to intercept data between UBI and MTD (e.g.
   mtd_read, mtd_write etc) and encrypt/decrypt it with the kernel crypto
   framework (e.g. crypto_*). This is good because it de-couples encryption
   from the rest of UBI, reduces/isolates complexity and ensures that everything
   is indiscriminately encrypted. Though there may be a more obvious place to do
   this.

 - This approach is also bad because it breaks an assumption that UBI and UBIFS
   make (as well as any other UBI users) that data returned from the MTD layer
   containing 'all bits set' is erased flash. The same is also true for writing
   data - for example when filling space with 0xFFs. Where we intercept and
   encrypt/decrypt the 'all bits set' data we break the assumption because
   we've turned it to garbage.

 - My work around for this erased flash issue was to conditionally
   encrypt/decrypt only when the input data is not 'all bits set'. This had
   minimal impact on UBI/UBIFS/etc but it is possible (though very unlikely)
   that the output of an encryption algorithm is 'all bits set' - Thus when you
   later attempt to decrypt the 'all bits set' cipher text we incorrectly treat
   it as erased flash so return it verbatim and thus cause corruption. I've not
   seen this issue occur despite reading and writing more than 50GB of data.

 - A better solution may be to correctly fix up the callers to the
   'interception' layer such that they can choose to read raw or with
   encryption. An example of where this would be needed is in 'torture_peb' -
   after an erase the fuction reads back the flash to see if its 'all
bits set'. This
   seems like the right approach to me.

 - I have implemented the 'better solution' and it appears to work - however
   modifications are then needed to UBIFS in order for that to work. For example
   when mounting UBIFS on empty flash it will scan and fail to find
   UBIFS_NODE_MAGIC headers (as expected) - it will then determine that the
   start scan wasn't empty space (as the 0xFFs have been decrypted into garbage)
   and return an error. I believe this is the only issue I found with UBIFS.
   (Of course another way to solve this would be to encrypt empty space - but
   this would increase wear as the empty space (decrypted to 0xFFs) wouldn't
   actually be erased flash thus requiring an additional erase prior to writing
   data.).

 - The current implementation  encrypts with cbc(aes) across fixed sized
   units of hdrs_min_io_size - it also uses an IV based on the physcial offset
   of the block and user provided IV. The key/IV is read from a fixed location
   in flash. I'm not sure of the best way to manage/locate keys - this is
   clearly a hack.

 - Encryption in UBI was preferred as it removed the complexity from userspace,
   though I suppose there is no reason why this can't be done within the MTD
   layer rather than in UBI and thus benefit all MTD users.

I'm keen to hear any feedback regarding the approaches I've mentioned and to
hear if anyone else working in this area. I'm also keen to hear if
this is wanted
or if this is a bad idea. I accept this isn't trivial undertaking.

Thanks,

Andrew Murray

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-10 19:56 [RFC] mtd: ubi: UBI Encryption Andrew Murray
@ 2015-08-11  5:38 ` Timo Ketola
  2015-08-11  6:22   ` Richard Weinberger
  2015-08-11  6:30 ` Richard Weinberger
  2015-08-12 17:19 ` David Gstir
  2 siblings, 1 reply; 19+ messages in thread
From: Timo Ketola @ 2015-08-11  5:38 UTC (permalink / raw)
  To: Andrew Murray, linux-mtd@lists.infradead.org

Hi,

I have been lurking in this list for a long time and this is my first
post here. I decided to write because I think I have yet another idea
for this one:

On 10.08.2015 22:56, Andrew Murray wrote:
> ...
>  - My work around for this erased flash issue was to conditionally
>    encrypt/decrypt only when the input data is not 'all bits set'. This had
>    minimal impact on UBI/UBIFS/etc but it is possible (though very unlikely)
>    that the output of an encryption algorithm is 'all bits set' - Thus when you
>    later attempt to decrypt the 'all bits set' cipher text we incorrectly treat
>    it as erased flash so return it verbatim and thus cause corruption. I've not
>    seen this issue occur despite reading and writing more than 50GB of data.
> ...

Why not postprocess the data so that the encrypted FF becomes FF again
like this:

Lets say clear text data is I, encrypted data is O, encryption function
is e() and decryption function is d(). Then, what is normally done, is
of course:

Write: O = e(I)
Read: I = d(O)

Calculate F = ~e(FF), where F is encrypted and inverted version of 'all
bits set' (FF) data, and modify writing and reading:

Write: O = e(I) ^ F
Read: I = d(O ^ F)

Now encrypting FF input results in FF output and vice versa.

Just wanted to introduce an idea.

--

Timo

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-11  5:38 ` Timo Ketola
@ 2015-08-11  6:22   ` Richard Weinberger
  2015-08-11  6:35     ` Timo Ketola
  0 siblings, 1 reply; 19+ messages in thread
From: Richard Weinberger @ 2015-08-11  6:22 UTC (permalink / raw)
  To: Timo Ketola; +Cc: Andrew Murray, linux-mtd@lists.infradead.org

On Tue, Aug 11, 2015 at 7:38 AM, Timo Ketola <Timo.Ketola@exertus.fi> wrote:
> Hi,
>
> I have been lurking in this list for a long time and this is my first
> post here. I decided to write because I think I have yet another idea
> for this one:
>
> On 10.08.2015 22:56, Andrew Murray wrote:
>> ...
>>  - My work around for this erased flash issue was to conditionally
>>    encrypt/decrypt only when the input data is not 'all bits set'. This had
>>    minimal impact on UBI/UBIFS/etc but it is possible (though very unlikely)
>>    that the output of an encryption algorithm is 'all bits set' - Thus when you
>>    later attempt to decrypt the 'all bits set' cipher text we incorrectly treat
>>    it as erased flash so return it verbatim and thus cause corruption. I've not
>>    seen this issue occur despite reading and writing more than 50GB of data.
>> ...
>
> Why not postprocess the data so that the encrypted FF becomes FF again
> like this:
>
> Lets say clear text data is I, encrypted data is O, encryption function
> is e() and decryption function is d(). Then, what is normally done, is
> of course:
>
> Write: O = e(I)
> Read: I = d(O)
>
> Calculate F = ~e(FF), where F is encrypted and inverted version of 'all
> bits set' (FF) data, and modify writing and reading:
>
> Write: O = e(I) ^ F
> Read: I = d(O ^ F)
>
> Now encrypting FF input results in FF output and vice versa.
>
> Just wanted to introduce an idea.

But wouldn't that only work for ECB cipher mode?

-- 
Thanks,
//richard

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-10 19:56 [RFC] mtd: ubi: UBI Encryption Andrew Murray
  2015-08-11  5:38 ` Timo Ketola
@ 2015-08-11  6:30 ` Richard Weinberger
  2015-08-11  9:47   ` Andrew Murray
  2015-08-12 17:19 ` David Gstir
  2 siblings, 1 reply; 19+ messages in thread
From: Richard Weinberger @ 2015-08-11  6:30 UTC (permalink / raw)
  To: Andrew Murray; +Cc: linux-mtd@lists.infradead.org

On Mon, Aug 10, 2015 at 9:56 PM, Andrew Murray
<amurray@embedded-bits.co.uk> wrote:
> We've recently implemented support for encryption within UBI for one of our
> customers and now wish to use this experience to provide a suitable solution
> for the community.
>
> Our current implementation works on real hardware and the latest linux-mtd
> kernel - however there are many issues that in my opinion make it unsuitable
> for the wider community. I'm keen to address these issues with feedback from
> linux-mtd such that we can get this in good shape. I'm happy to share the
> current implementation if it helps form the basis of a discussion that will
> address the general issues of adding encryption to UBI. (The diffstat for the
> current implementation is about 407 insertions).
>
> In summary:
>
>  - The approach I've taken is to intercept data between UBI and MTD (e.g.
>    mtd_read, mtd_write etc) and encrypt/decrypt it with the kernel crypto
>    framework (e.g. crypto_*). This is good because it de-couples encryption
>    from the rest of UBI, reduces/isolates complexity and ensures that everything
>    is indiscriminately encrypted. Though there may be a more obvious place to do
>    this.
>
>  - This approach is also bad because it breaks an assumption that UBI and UBIFS
>    make (as well as any other UBI users) that data returned from the MTD layer
>    containing 'all bits set' is erased flash. The same is also true for writing
>    data - for example when filling space with 0xFFs. Where we intercept and
>    encrypt/decrypt the 'all bits set' data we break the assumption because
>    we've turned it to garbage.
>
>  - My work around for this erased flash issue was to conditionally
>    encrypt/decrypt only when the input data is not 'all bits set'. This had
>    minimal impact on UBI/UBIFS/etc but it is possible (though very unlikely)
>    that the output of an encryption algorithm is 'all bits set' - Thus when you
>    later attempt to decrypt the 'all bits set' cipher text we incorrectly treat
>    it as erased flash so return it verbatim and thus cause corruption. I've not
>    seen this issue occur despite reading and writing more than 50GB of data.
>
>  - A better solution may be to correctly fix up the callers to the
>    'interception' layer such that they can choose to read raw or with
>    encryption. An example of where this would be needed is in 'torture_peb' -
>    after an erase the fuction reads back the flash to see if its 'all
> bits set'. This
>    seems like the right approach to me.
>
>  - I have implemented the 'better solution' and it appears to work - however
>    modifications are then needed to UBIFS in order for that to work. For example
>    when mounting UBIFS on empty flash it will scan and fail to find
>    UBIFS_NODE_MAGIC headers (as expected) - it will then determine that the
>    start scan wasn't empty space (as the 0xFFs have been decrypted into garbage)
>    and return an error. I believe this is the only issue I found with UBIFS.
>    (Of course another way to solve this would be to encrypt empty space - but
>    this would increase wear as the empty space (decrypted to 0xFFs) wouldn't
>    actually be erased flash thus requiring an additional erase prior to writing
>    data.).
>
>  - The current implementation  encrypts with cbc(aes) across fixed sized
>    units of hdrs_min_io_size - it also uses an IV based on the physcial offset
>    of the block and user provided IV. The key/IV is read from a fixed location
>    in flash. I'm not sure of the best way to manage/locate keys - this is
>    clearly a hack.

Hm, what do you mean by "read from a fixed location in flash"?
Did you change the UBI on-flash format to store new meta data?

How do you chain the encrypted blocks? You have to deal with bad blocks.
IOW if you lose a block it should only affect encrypted data with in
the same block.

>  - Encryption in UBI was preferred as it removed the complexity from userspace,
>    though I suppose there is no reason why this can't be done within the MTD
>    layer rather than in UBI and thus benefit all MTD users.

I'm not sure if UBI is the right layer for that. I'd do it in MTD to
have a dm_crypt like
MTD driver. At best it will be compatible with dm_crypt's userspace tools.

-- 
Thanks,
//richard

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-11  6:22   ` Richard Weinberger
@ 2015-08-11  6:35     ` Timo Ketola
  0 siblings, 0 replies; 19+ messages in thread
From: Timo Ketola @ 2015-08-11  6:35 UTC (permalink / raw)
  To: Richard Weinberger; +Cc: Andrew Murray, linux-mtd@lists.infradead.org

On 11.08.2015 09:22, Richard Weinberger wrote:
> But wouldn't that only work for ECB cipher mode?

You are right. Obviously I know too little about encryption methods...

--

Timo

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-11  6:30 ` Richard Weinberger
@ 2015-08-11  9:47   ` Andrew Murray
  2015-08-11 10:23     ` Michal Suchanek
  0 siblings, 1 reply; 19+ messages in thread
From: Andrew Murray @ 2015-08-11  9:47 UTC (permalink / raw)
  To: Richard Weinberger; +Cc: linux-mtd@lists.infradead.org

On 11 August 2015 at 07:30, Richard Weinberger
<richard.weinberger@gmail.com> wrote:
> On Mon, Aug 10, 2015 at 9:56 PM, Andrew Murray
> <amurray@embedded-bits.co.uk> wrote:
>> We've recently implemented support for encryption within UBI for one of our
>> customers and now wish to use this experience to provide a suitable solution
>> for the community.
>>
>> Our current implementation works on real hardware and the latest linux-mtd
>> kernel - however there are many issues that in my opinion make it unsuitable
>> for the wider community. I'm keen to address these issues with feedback from
>> linux-mtd such that we can get this in good shape. I'm happy to share the
>> current implementation if it helps form the basis of a discussion that will
>> address the general issues of adding encryption to UBI. (The diffstat for the
>> current implementation is about 407 insertions).
>>
>> In summary:
>>
>>  - The approach I've taken is to intercept data between UBI and MTD (e.g.
>>    mtd_read, mtd_write etc) and encrypt/decrypt it with the kernel crypto
>>    framework (e.g. crypto_*). This is good because it de-couples encryption
>>    from the rest of UBI, reduces/isolates complexity and ensures that everything
>>    is indiscriminately encrypted. Though there may be a more obvious place to do
>>    this.
>>
>>  - This approach is also bad because it breaks an assumption that UBI and UBIFS
>>    make (as well as any other UBI users) that data returned from the MTD layer
>>    containing 'all bits set' is erased flash. The same is also true for writing
>>    data - for example when filling space with 0xFFs. Where we intercept and
>>    encrypt/decrypt the 'all bits set' data we break the assumption because
>>    we've turned it to garbage.
>>
>>  - My work around for this erased flash issue was to conditionally
>>    encrypt/decrypt only when the input data is not 'all bits set'. This had
>>    minimal impact on UBI/UBIFS/etc but it is possible (though very unlikely)
>>    that the output of an encryption algorithm is 'all bits set' - Thus when you
>>    later attempt to decrypt the 'all bits set' cipher text we incorrectly treat
>>    it as erased flash so return it verbatim and thus cause corruption. I've not
>>    seen this issue occur despite reading and writing more than 50GB of data.
>>
>>  - A better solution may be to correctly fix up the callers to the
>>    'interception' layer such that they can choose to read raw or with
>>    encryption. An example of where this would be needed is in 'torture_peb' -
>>    after an erase the fuction reads back the flash to see if its 'all
>> bits set'. This
>>    seems like the right approach to me.
>>
>>  - I have implemented the 'better solution' and it appears to work - however
>>    modifications are then needed to UBIFS in order for that to work. For example
>>    when mounting UBIFS on empty flash it will scan and fail to find
>>    UBIFS_NODE_MAGIC headers (as expected) - it will then determine that the
>>    start scan wasn't empty space (as the 0xFFs have been decrypted into garbage)
>>    and return an error. I believe this is the only issue I found with UBIFS.
>>    (Of course another way to solve this would be to encrypt empty space - but
>>    this would increase wear as the empty space (decrypted to 0xFFs) wouldn't
>>    actually be erased flash thus requiring an additional erase prior to writing
>>    data.).
>>
>>  - The current implementation  encrypts with cbc(aes) across fixed sized
>>    units of hdrs_min_io_size - it also uses an IV based on the physcial offset
>>    of the block and user provided IV. The key/IV is read from a fixed location
>>    in flash. I'm not sure of the best way to manage/locate keys - this is
>>    clearly a hack.
>
> Hm, what do you mean by "read from a fixed location in flash"?
> Did you change the UBI on-flash format to store new meta data?

No this was read from an offset within another MTD partition defined
in the kernel .config. In this case it was the internal flash of a
SoC. This allowed us to keep the key in the SoC and boot an encrypted
rootfs such that the external NAND held only encrypted data. This
seemed less complex than needing an intermediary filesystem (e.g.
initramfs) that would need to find keys prior to attaching UBI.

I'm not sure how best to apply this to the general use case. Perhaps
userspace should be required to provide keys thus requiring some
intermediary filesystem (and UBI API)? Or perhaps the current Kconfig
approach provides enough flexibility for most users.

>
> How do you chain the encrypted blocks? You have to deal with bad blocks.

The encryption is performed within the minimum units of
hdrs_min_io_size. In our case we used cbc(aes) (though that is
configurable), the key size was smaller than hdrs_min_io_size and thus
I believe some benefit is gained. Blocks aren't chained together - but
we thought we'd start with something simple that worked. Therefore cbc
works across hdrs_min_io_size but not between them.

> IOW if you lose a block it should only affect encrypted data with in
> the same block.

Yes, but on the granularity of hdrs_min_io_size.

>
>>  - Encryption in UBI was preferred as it removed the complexity from userspace,
>>    though I suppose there is no reason why this can't be done within the MTD
>>    layer rather than in UBI and thus benefit all MTD users.
>
> I'm not sure if UBI is the right layer for that. I'd do it in MTD to
> have a dm_crypt like
> MTD driver. At best it will be compatible with dm_crypt's userspace tools.

I've not investigated this - but it seems plausible. My 'interception'
layer works immediately above the MTD layer (i.e. the patchset looks a
bit like this):

-       err = mtd_write(ubi->mtd, addr, len, &written, buf);
+       err = mtd_crypt_write(ubi, addr, len, &written, buf);

Thus this code could just as easily exist on the other size of mtd_*.

However this still leaves the issue of assumptions all over the place
that 0xFF == erased flash. Is it best to risk the collision that
crypto output could generate all 0xFFs - or is it best to overcome the
assumptions by fixing them up as an initial stage? I suspect the later
- besides helping the encryption problem it also means you aren't
relying on the content of the flash to determine its state (I wonder
if this could also be used to solve other issues where its difficult
to tell if flash is erased - e.g. inverted ECCs?).

Andrew Murray

>
> --
> Thanks,
> //richard

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-11  9:47   ` Andrew Murray
@ 2015-08-11 10:23     ` Michal Suchanek
  2015-08-11 11:03       ` Andrew Murray
  0 siblings, 1 reply; 19+ messages in thread
From: Michal Suchanek @ 2015-08-11 10:23 UTC (permalink / raw)
  To: Andrew Murray; +Cc: Richard Weinberger, linux-mtd@lists.infradead.org

On 11 August 2015 at 11:47, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
> On 11 August 2015 at 07:30, Richard Weinberger
> <richard.weinberger@gmail.com> wrote:
>> On Mon, Aug 10, 2015 at 9:56 PM, Andrew Murray
>> <amurray@embedded-bits.co.uk> wrote:
>>> We've recently implemented support for encryption within UBI for one of our
>>> customers and now wish to use this experience to provide a suitable solution
>>> for the community.
>>>
>>> Our current implementation works on real hardware and the latest linux-mtd
>>> kernel - however there are many issues that in my opinion make it unsuitable
>>> for the wider community. I'm keen to address these issues with feedback from
>>> linux-mtd such that we can get this in good shape. I'm happy to share the
>>> current implementation if it helps form the basis of a discussion that will
>>> address the general issues of adding encryption to UBI. (The diffstat for the
>>> current implementation is about 407 insertions).
>>>
>>> In summary:
>>>
>>>  - The approach I've taken is to intercept data between UBI and MTD (e.g.
>>>    mtd_read, mtd_write etc) and encrypt/decrypt it with the kernel crypto
>>>    framework (e.g. crypto_*). This is good because it de-couples encryption
>>>    from the rest of UBI, reduces/isolates complexity and ensures that everything
>>>    is indiscriminately encrypted. Though there may be a more obvious place to do
>>>    this.
>>>
>>>  - This approach is also bad because it breaks an assumption that UBI and UBIFS
>>>    make (as well as any other UBI users) that data returned from the MTD layer
>>>    containing 'all bits set' is erased flash. The same is also true for writing
>>>    data - for example when filling space with 0xFFs. Where we intercept and
>>>    encrypt/decrypt the 'all bits set' data we break the assumption because
>>>    we've turned it to garbage.
>>>
>>>  - My work around for this erased flash issue was to conditionally
>>>    encrypt/decrypt only when the input data is not 'all bits set'. This had
>>>    minimal impact on UBI/UBIFS/etc but it is possible (though very unlikely)
>>>    that the output of an encryption algorithm is 'all bits set' - Thus when you
>>>    later attempt to decrypt the 'all bits set' cipher text we incorrectly treat
>>>    it as erased flash so return it verbatim and thus cause corruption. I've not
>>>    seen this issue occur despite reading and writing more than 50GB of data.
>>>
>>>  - A better solution may be to correctly fix up the callers to the
>>>    'interception' layer such that they can choose to read raw or with
>>>    encryption. An example of where this would be needed is in 'torture_peb' -
>>>    after an erase the fuction reads back the flash to see if its 'all
>>> bits set'. This
>>>    seems like the right approach to me.
>>>
>>>  - I have implemented the 'better solution' and it appears to work - however
>>>    modifications are then needed to UBIFS in order for that to work. For example
>>>    when mounting UBIFS on empty flash it will scan and fail to find
>>>    UBIFS_NODE_MAGIC headers (as expected) - it will then determine that the
>>>    start scan wasn't empty space (as the 0xFFs have been decrypted into garbage)
>>>    and return an error. I believe this is the only issue I found with UBIFS.
>>>    (Of course another way to solve this would be to encrypt empty space - but
>>>    this would increase wear as the empty space (decrypted to 0xFFs) wouldn't
>>>    actually be erased flash thus requiring an additional erase prior to writing
>>>    data.).
>>>
>>>  - The current implementation  encrypts with cbc(aes) across fixed sized
>>>    units of hdrs_min_io_size - it also uses an IV based on the physcial offset
>>>    of the block and user provided IV. The key/IV is read from a fixed location
>>>    in flash. I'm not sure of the best way to manage/locate keys - this is
>>>    clearly a hack.
>>
>> Hm, what do you mean by "read from a fixed location in flash"?
>> Did you change the UBI on-flash format to store new meta data?
>
> No this was read from an offset within another MTD partition defined
> in the kernel .config. In this case it was the internal flash of a
> SoC. This allowed us to keep the key in the SoC and boot an encrypted
> rootfs such that the external NAND held only encrypted data. This
> seemed less complex than needing an intermediary filesystem (e.g.
> initramfs) that would need to find keys prior to attaching UBI.
>
> I'm not sure how best to apply this to the general use case. Perhaps
> userspace should be required to provide keys thus requiring some
> intermediary filesystem (and UBI API)? Or perhaps the current Kconfig
> approach provides enough flexibility for most users.
>
>>
>> How do you chain the encrypted blocks? You have to deal with bad blocks.
>
> The encryption is performed within the minimum units of
> hdrs_min_io_size. In our case we used cbc(aes) (though that is
> configurable), the key size was smaller than hdrs_min_io_size and thus
> I believe some benefit is gained. Blocks aren't chained together - but
> we thought we'd start with something simple that worked. Therefore cbc
> works across hdrs_min_io_size but not between them.
>
>> IOW if you lose a block it should only affect encrypted data with in
>> the same block.
>
> Yes, but on the granularity of hdrs_min_io_size.
>
>>
>>>  - Encryption in UBI was preferred as it removed the complexity from userspace,
>>>    though I suppose there is no reason why this can't be done within the MTD
>>>    layer rather than in UBI and thus benefit all MTD users.
>>
>> I'm not sure if UBI is the right layer for that. I'd do it in MTD to
>> have a dm_crypt like
>> MTD driver. At best it will be compatible with dm_crypt's userspace tools.
>
> I've not investigated this - but it seems plausible. My 'interception'
> layer works immediately above the MTD layer (i.e. the patchset looks a
> bit like this):
>
> -       err = mtd_write(ubi->mtd, addr, len, &written, buf);
> +       err = mtd_crypt_write(ubi, addr, len, &written, buf);
>
> Thus this code could just as easily exist on the other size of mtd_*.
>
> However this still leaves the issue of assumptions all over the place
> that 0xFF == erased flash. Is it best to risk the collision that
> crypto output could generate all 0xFFs - or is it best to overcome the
> assumptions by fixing them up as an initial stage? I suspect the later
> - besides helping the encryption problem it also means you aren't
> relying on the content of the flash to determine its state (I wonder
> if this could also be used to solve other issues where its difficult
> to tell if flash is erased - e.g. inverted ECCs?).

It's probably good idea to treat empty blocks as truly empty and only
encrypt data blocks. It is also possible to write all 1s to
unencrypted ubifs block and the filesystem should handle it. The block
will technically remain empty but it can be interpreted as data. If
this does not work properly for non-encrypted flash it should be fixed
I guess.

In other words you have two layers here - the layer of physical blocks
which should be the same regardless of encryption and the layer of
data blocks which is transformed by encryption.

It's good idea to weight options when dealing with encryption and erasing blocks
 - one is to erase unused blocks in the hope that reading them back
and getting stale data will then be more difficult
 - other is to keep unused blocks intact as much as possible, avoid
half-full blocks and only erase blocks that are about to be written so
usage patterns are not as obvious - this only applies if the block
usage tables and other FTL data is also encrypted.

Encrypting the FTL data may also pose a problem since the structure of
the tables may be fixed to some extent and may be used as additional
leverage if weakness is found in a cipher algorithm.

On 11 August 2015 at 07:38, Timo Ketola <Timo.Ketola@exertus.fi> wrote:
> Hi,
>
> I have been lurking in this list for a long time and this is my first
> post here. I decided to write because I think I have yet another idea
> for this one:
>
> On 10.08.2015 22:56, Andrew Murray wrote:
>> ...
>>  - My work around for this erased flash issue was to conditionally
>>    encrypt/decrypt only when the input data is not 'all bits set'. This had
>>    minimal impact on UBI/UBIFS/etc but it is possible (though very unlikely)
>>    that the output of an encryption algorithm is 'all bits set' - Thus when you
>>    later attempt to decrypt the 'all bits set' cipher text we incorrectly treat
>>    it as erased flash so return it verbatim and thus cause corruption. I've not
>>    seen this issue occur despite reading and writing more than 50GB of data.
>> ...
>
> Why not postprocess the data so that the encrypted FF becomes FF again
> like this:
>
> Lets say clear text data is I, encrypted data is O, encryption function
> is e() and decryption function is d(). Then, what is normally done, is
> of course:
>
> Write: O = e(I)
> Read: I = d(O)
>
> Calculate F = ~e(FF), where F is encrypted and inverted version of 'all
> bits set' (FF) data, and modify writing and reading:
>
> Write: O = e(I) ^ F
> Read: I = d(O ^ F)
>
> Now encrypting FF input results in FF output and vice versa.
>
> Just wanted to introduce an idea.

This post-processing function is in general hard to obtain. Since the
encryption is seeded with block offset you would have to encrypt empty
block with the block-specific iv to obtain the transform function and
then apply it to the encrypted block to obtain flash data when writing
and apply inversion on the flash data to obtain encrypted block when
reading. So you would basically encrypt/decrypt each block twice.

Without some serious research you cannot tell how this damages the
security of the encryption.

Thanks

Michal

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-11 10:23     ` Michal Suchanek
@ 2015-08-11 11:03       ` Andrew Murray
  2015-08-11 11:39         ` Michal Suchanek
  0 siblings, 1 reply; 19+ messages in thread
From: Andrew Murray @ 2015-08-11 11:03 UTC (permalink / raw)
  To: Michal Suchanek; +Cc: Richard Weinberger, linux-mtd@lists.infradead.org

On 11 August 2015 at 11:23, Michal Suchanek <hramrach@gmail.com> wrote:
>
> On 11 August 2015 at 11:47, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
> > On 11 August 2015 at 07:30, Richard Weinberger
> > <richard.weinberger@gmail.com> wrote:
> >> On Mon, Aug 10, 2015 at 9:56 PM, Andrew Murray
> >> <amurray@embedded-bits.co.uk> wrote:
>
> >>
> >>>  - Encryption in UBI was preferred as it removed the complexity from userspace,
> >>>    though I suppose there is no reason why this can't be done within the MTD
> >>>    layer rather than in UBI and thus benefit all MTD users.
> >>
> >> I'm not sure if UBI is the right layer for that. I'd do it in MTD to
> >> have a dm_crypt like
> >> MTD driver. At best it will be compatible with dm_crypt's userspace tools.
> >
> > I've not investigated this - but it seems plausible. My 'interception'
> > layer works immediately above the MTD layer (i.e. the patchset looks a
> > bit like this):
> >
> > -       err = mtd_write(ubi->mtd, addr, len, &written, buf);
> > +       err = mtd_crypt_write(ubi, addr, len, &written, buf);
> >
> > Thus this code could just as easily exist on the other size of mtd_*.
> >
> > However this still leaves the issue of assumptions all over the place
> > that 0xFF == erased flash. Is it best to risk the collision that
> > crypto output could generate all 0xFFs - or is it best to overcome the
> > assumptions by fixing them up as an initial stage? I suspect the later
> > - besides helping the encryption problem it also means you aren't
> > relying on the content of the flash to determine its state (I wonder
> > if this could also be used to solve other issues where its difficult
> > to tell if flash is erased - e.g. inverted ECCs?).
>
> It's probably good idea to treat empty blocks as truly empty and only
> encrypt data blocks.


In some sense this is what I already have. My encryption hooks exist
just above the MTD layer and only get used if UBI attempts to write
data to MTD. One of my implementations doesn't encrypt data from UBI
if it contains all 0xFFs. (Of course the problem is reading those
0xFFs back from flash and determining if its empty data or encrypted
data).

>
> It is also possible to write all 1s to
> unencrypted ubifs block and the filesystem should handle it. The block
> will technically remain empty but it can be interpreted as data. If
> this does not work properly for non-encrypted flash it should be fixed
> I guess.


Yes this is what I do, if data is provided to UBI containing all 1s, I
write it to flash as all 1s. Upon reading it back I also return it to
UBI without modification. UBIFS expects this - and attempting to mount
UBIFS on a empty UBI volume will otherwise fail unless I do this (as
UBIFS makes the assumption that all 1s is empty flash).

I'm slightly uncomfortable that UBIFS makes this assumption, it seems
to be present as a very sensible sanity check - but it ties back in
the characteristics of flash back into a layer that should be
abstracted away from it.

>
>
> In other words you have two layers here - the layer of physical blocks
> which should be the same regardless of encryption and the layer of
> data blocks which is transformed by encryption.


This is my understanding if I s/phyiscal blocks/logical erase blocks/g
and s/data blocks/physical erase blocks/g

>
>
> It's good idea to weight options when dealing with encryption and erasing blocks
>  - one is to erase unused blocks in the hope that reading them back
> and getting stale data will then be more difficult
>  - other is to keep unused blocks intact as much as possible, avoid
> half-full blocks and only erase blocks that are about to be written so
> usage patterns are not as obvious - this only applies if the block
> usage tables and other FTL data is also encrypted.


In trying to keep encryption decoupled from UBI, my encryption layer
is rather transparent, and thus oblivious to the structures of the
data being encrypted and the significance of erasing blocks. Thus here
the FTL data (assuming you mean the filesystem meta data?) is all
encrypted. The downside here is that I don't know when the filesystem
has finished using blocks.

>
>
> Encrypting the FTL data may also pose a problem since the structure of
> the tables may be fixed to some extent and may be used as additional
> leverage if weakness is found in a cipher algorithm.


Is encrypting the FTL undesirable?

>
>
> On 11 August 2015 at 07:38, Timo Ketola <Timo.Ketola@exertus.fi> wrote:
> > Hi,
> >
> > I have been lurking in this list for a long time and this is my first
> > post here. I decided to write because I think I have yet another idea
> > for this one:
> >
> > On 10.08.2015 22:56, Andrew Murray wrote:
> >> ...
> >>  - My work around for this erased flash issue was to conditionally
> >>    encrypt/decrypt only when the input data is not 'all bits set'. This had
> >>    minimal impact on UBI/UBIFS/etc but it is possible (though very unlikely)
> >>    that the output of an encryption algorithm is 'all bits set' - Thus when you
> >>    later attempt to decrypt the 'all bits set' cipher text we incorrectly treat
> >>    it as erased flash so return it verbatim and thus cause corruption. I've not
> >>    seen this issue occur despite reading and writing more than 50GB of data.
> >> ...
> >
> > Why not postprocess the data so that the encrypted FF becomes FF again
> > like this:
> >
> > Lets say clear text data is I, encrypted data is O, encryption function
> > is e() and decryption function is d(). Then, what is normally done, is
> > of course:
> >
> > Write: O = e(I)
> > Read: I = d(O)
> >
> > Calculate F = ~e(FF), where F is encrypted and inverted version of 'all
> > bits set' (FF) data, and modify writing and reading:
> >
> > Write: O = e(I) ^ F
> > Read: I = d(O ^ F)
> >
> > Now encrypting FF input results in FF output and vice versa.
> >
> > Just wanted to introduce an idea.
>
> This post-processing function is in general hard to obtain. Since the
> encryption is seeded with block offset you would have to encrypt empty
> block with the block-specific iv to obtain the transform function and
> then apply it to the encrypted block to obtain flash data when writing
> and apply inversion on the flash data to obtain encrypted block when
> reading. So you would basically encrypt/decrypt each block twice.
>
> Without some serious research you cannot tell how this damages the
> security of the encryption.


I shy'd away from taking this approach as I'm not a crypto expert and
didn't want to tie UBI encryption to a specific crypto algorithm.

Andrew Murray

>
>
> Thanks
>
> Michal

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-11 11:03       ` Andrew Murray
@ 2015-08-11 11:39         ` Michal Suchanek
  2015-08-11 12:40           ` Andrew Murray
  0 siblings, 1 reply; 19+ messages in thread
From: Michal Suchanek @ 2015-08-11 11:39 UTC (permalink / raw)
  To: Andrew Murray; +Cc: Richard Weinberger, linux-mtd@lists.infradead.org

On 11 August 2015 at 13:03, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
> On 11 August 2015 at 11:23, Michal Suchanek <hramrach@gmail.com> wrote:
>>
>> On 11 August 2015 at 11:47, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
>> > On 11 August 2015 at 07:30, Richard Weinberger
>> > <richard.weinberger@gmail.com> wrote:
>> >> On Mon, Aug 10, 2015 at 9:56 PM, Andrew Murray
>> >> <amurray@embedded-bits.co.uk> wrote:
>>
>> >>
>> >>>  - Encryption in UBI was preferred as it removed the complexity from userspace,
>> >>>    though I suppose there is no reason why this can't be done within the MTD
>> >>>    layer rather than in UBI and thus benefit all MTD users.
>> >>
>> >> I'm not sure if UBI is the right layer for that. I'd do it in MTD to
>> >> have a dm_crypt like
>> >> MTD driver. At best it will be compatible with dm_crypt's userspace tools.
>> >
>> > I've not investigated this - but it seems plausible. My 'interception'
>> > layer works immediately above the MTD layer (i.e. the patchset looks a
>> > bit like this):
>> >
>> > -       err = mtd_write(ubi->mtd, addr, len, &written, buf);
>> > +       err = mtd_crypt_write(ubi, addr, len, &written, buf);
>> >
>> > Thus this code could just as easily exist on the other size of mtd_*.
>> >
>> > However this still leaves the issue of assumptions all over the place
>> > that 0xFF == erased flash. Is it best to risk the collision that
>> > crypto output could generate all 0xFFs - or is it best to overcome the
>> > assumptions by fixing them up as an initial stage? I suspect the later
>> > - besides helping the encryption problem it also means you aren't
>> > relying on the content of the flash to determine its state (I wonder
>> > if this could also be used to solve other issues where its difficult
>> > to tell if flash is erased - e.g. inverted ECCs?).
>>
>> It's probably good idea to treat empty blocks as truly empty and only
>> encrypt data blocks.
>
>
> In some sense this is what I already have. My encryption hooks exist
> just above the MTD layer and only get used if UBI attempts to write
> data to MTD. One of my implementations doesn't encrypt data from UBI
> if it contains all 0xFFs. (Of course the problem is reading those
> 0xFFs back from flash and determining if its empty data or encrypted
> data).

Then that's wrong. The data is all 1 and the encrypted data should be
not. If the output of encryption is all 1 then the block stays empty
although writing all 1 should generate an ECC on flash chips that have
one. Some handle all 1 specially and don't generate one ..

If the FTL says it's a datablock it's a datablock.

If all blocks are empty you should format that flash.

Flash with FTL structure has non-empty blocks that contain the FTL data.

>
>>
>> It is also possible to write all 1s to
>> unencrypted ubifs block and the filesystem should handle it. The block
>> will technically remain empty but it can be interpreted as data. If
>> this does not work properly for non-encrypted flash it should be fixed
>> I guess.
>
>
> Yes this is what I do, if data is provided to UBI containing all 1s, I
> write it to flash as all 1s. Upon reading it back I also return it to
> UBI without modification. UBIFS expects this - and attempting to mount
> UBIFS on a empty UBI volume will otherwise fail unless I do this (as
> UBIFS makes the assumption that all 1s is empty flash).
>
> I'm slightly uncomfortable that UBIFS makes this assumption, it seems
> to be present as a very sensible sanity check - but it ties back in
> the characteristics of flash back into a layer that should be
> abstracted away from it.

Ubifs deals directly with physical flash erase blocks so it cannot
possibly be abstracted from it. Technically the pattern for empty
block could be different on some devices so it might be nice to have
some sort of is_empty_block somewhere in MTD. So far all 1 is what
everything uses otherwise ubifs would fail ;-)

>>
>>
>> It's good idea to weight options when dealing with encryption and erasing blocks
>>  - one is to erase unused blocks in the hope that reading them back
>> and getting stale data will then be more difficult
>>  - other is to keep unused blocks intact as much as possible, avoid
>> half-full blocks and only erase blocks that are about to be written so
>> usage patterns are not as obvious - this only applies if the block
>> usage tables and other FTL data is also encrypted.
>
>
> In trying to keep encryption decoupled from UBI, my encryption layer
> is rather transparent, and thus oblivious to the structures of the
> data being encrypted and the significance of erasing blocks. Thus here
> the FTL data (assuming you mean the filesystem meta data?) is all
> encrypted. The downside here is that I don't know when the filesystem
> has finished using blocks.

Erasing happens in UBIFS presumably by invoking a block erase on the
MTD that turns the block in all 1 (except for stuck bits). That is
outside of write().

So the encryption would not deal with erasing blocks but you might
want to tune ubifs parameters that deal with erasing blocks. That is
independent of actually encrypting the data but might affect the
overall strength of your encryption scheme.

>
>>
>>
>> Encrypting the FTL data may also pose a problem since the structure of
>> the tables may be fixed to some extent and may be used as additional
>> leverage if weakness is found in a cipher algorithm.
>
>
> Is encrypting the FTL undesirable?

It is probably desirable. It's just something to be aware of and
something to consult with an encryption expert if you want to make
sure your encryption scheme is really sound.

>
>>
>>
>> On 11 August 2015 at 07:38, Timo Ketola <Timo.Ketola@exertus.fi> wrote:

>> > Why not postprocess the data so that the encrypted FF becomes FF again
>> > like this:
>> >
>> > Lets say clear text data is I, encrypted data is O, encryption function
>> > is e() and decryption function is d(). Then, what is normally done, is
>> > of course:
>> >
>> > Write: O = e(I)
>> > Read: I = d(O)
>> >
>> > Calculate F = ~e(FF), where F is encrypted and inverted version of 'all
>> > bits set' (FF) data, and modify writing and reading:
>> >
>> > Write: O = e(I) ^ F
>> > Read: I = d(O ^ F)
>> >
>> > Now encrypting FF input results in FF output and vice versa.
>> >
>> > Just wanted to introduce an idea.
>>
>> This post-processing function is in general hard to obtain. Since the
>> encryption is seeded with block offset you would have to encrypt empty
>> block with the block-specific iv to obtain the transform function and
>> then apply it to the encrypted block to obtain flash data when writing
>> and apply inversion on the flash data to obtain encrypted block when
>> reading. So you would basically encrypt/decrypt each block twice.
>>
>> Without some serious research you cannot tell how this damages the
>> security of the encryption.
>
>
> I shy'd away from taking this approach as I'm not a crypto expert and
> didn't want to tie UBI encryption to a specific crypto algorithm.
>

You could do this with any encryption algorithm provided you can get a
reversible transform (such as XOR negation) that converts arbitrary
block in all 1. The result may, however, be dodgy unless you are
really sure what you are doing.

Thanks

Michal

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-11 11:39         ` Michal Suchanek
@ 2015-08-11 12:40           ` Andrew Murray
  2015-08-11 13:24             ` Michal Suchanek
  0 siblings, 1 reply; 19+ messages in thread
From: Andrew Murray @ 2015-08-11 12:40 UTC (permalink / raw)
  To: Michal Suchanek; +Cc: Richard Weinberger, linux-mtd@lists.infradead.org

On 11 August 2015 at 12:39, Michal Suchanek <hramrach@gmail.com> wrote:
> On 11 August 2015 at 13:03, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
>> On 11 August 2015 at 11:23, Michal Suchanek <hramrach@gmail.com> wrote:
>>>
>>> On 11 August 2015 at 11:47, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
>>> > On 11 August 2015 at 07:30, Richard Weinberger
>>> > <richard.weinberger@gmail.com> wrote:
>>> >> On Mon, Aug 10, 2015 at 9:56 PM, Andrew Murray
>>> >> <amurray@embedded-bits.co.uk> wrote:
>>>
>>> >>
>>> >>>  - Encryption in UBI was preferred as it removed the complexity from userspace,
>>> >>>    though I suppose there is no reason why this can't be done within the MTD
>>> >>>    layer rather than in UBI and thus benefit all MTD users.
>>> >>
>>> >> I'm not sure if UBI is the right layer for that. I'd do it in MTD to
>>> >> have a dm_crypt like
>>> >> MTD driver. At best it will be compatible with dm_crypt's userspace tools.
>>> >
>>> > I've not investigated this - but it seems plausible. My 'interception'
>>> > layer works immediately above the MTD layer (i.e. the patchset looks a
>>> > bit like this):
>>> >
>>> > -       err = mtd_write(ubi->mtd, addr, len, &written, buf);
>>> > +       err = mtd_crypt_write(ubi, addr, len, &written, buf);
>>> >
>>> > Thus this code could just as easily exist on the other size of mtd_*.
>>> >
>>> > However this still leaves the issue of assumptions all over the place
>>> > that 0xFF == erased flash. Is it best to risk the collision that
>>> > crypto output could generate all 0xFFs - or is it best to overcome the
>>> > assumptions by fixing them up as an initial stage? I suspect the later
>>> > - besides helping the encryption problem it also means you aren't
>>> > relying on the content of the flash to determine its state (I wonder
>>> > if this could also be used to solve other issues where its difficult
>>> > to tell if flash is erased - e.g. inverted ECCs?).
>>>
>>> It's probably good idea to treat empty blocks as truly empty and only
>>> encrypt data blocks.
>>
>>
>> In some sense this is what I already have. My encryption hooks exist
>> just above the MTD layer and only get used if UBI attempts to write
>> data to MTD. One of my implementations doesn't encrypt data from UBI
>> if it contains all 0xFFs. (Of course the problem is reading those
>> 0xFFs back from flash and determining if its empty data or encrypted
>> data).
>
> Then that's wrong. The data is all 1 and the encrypted data should be
> not.

For clarify I've tried two approaches:

1) An implementation where encryption is provided unconditionally,
thus all 1's become something other than all 1's

2) An implementation where encryption is conditional based on the
data. If the plain text is all 1's then I do not encrypt.

I stuck with implementation 2) as it was less invasive. In order to
support implementation 2) (which also works), I needed to modify much
more code including UBIFS. This was needed as approach 2) breaks the
assumption that all 1's is empty flash - this is the case because when
reading and decrypting, all 1's becomes garbage - this breaks lots.
For example torture_peb erases flash and reads it back to ensure it
was erased - with encryption this test fails as the erased flash is
decrypted and becomes garbage.

I feel that the second approach is the correct one, I'm sensing you also agree?

 If the output of encryption is all 1 then the block stays empty
> although writing all 1 should generate an ECC on flash chips that have
> one. Some handle all 1 specially and don't generate one ..

I feel that a solution to overcome the issues with approach 2) could
also address the issues with ECC that is not all 0's where the data is
all 1's.

>
> If the FTL says it's a datablock it's a datablock.
>
> If all blocks are empty you should format that flash.
>
> Flash with FTL structure has non-empty blocks that contain the FTL data.

Can you clarify what FTL is? Are you referring to a data structure
devised by the software layers that is stored on flash, a flash
translation layer or something in hardware?

>
>>
>>>
>>> It is also possible to write all 1s to
>>> unencrypted ubifs block and the filesystem should handle it. The block
>>> will technically remain empty but it can be interpreted as data. If
>>> this does not work properly for non-encrypted flash it should be fixed
>>> I guess.
>>
>>
>> Yes this is what I do, if data is provided to UBI containing all 1s, I
>> write it to flash as all 1s. Upon reading it back I also return it to
>> UBI without modification. UBIFS expects this - and attempting to mount
>> UBIFS on a empty UBI volume will otherwise fail unless I do this (as
>> UBIFS makes the assumption that all 1s is empty flash).
>>
>> I'm slightly uncomfortable that UBIFS makes this assumption, it seems
>> to be present as a very sensible sanity check - but it ties back in
>> the characteristics of flash back into a layer that should be
>> abstracted away from it.
>
> Ubifs deals directly with physical flash erase blocks so it cannot
> possibly be abstracted from it.

I thought the purpose of UBI was to abstract the horribleness of flash
from its users. Such that filesystems could care about filesystem
related things rather than working around bad blocks and the other
nasties of flash.

UBIFS deals with logical erase blocks doesn't it? It doesn't need to
worry about erasing flash before using it, etc. So why should UBIFS
need to understand that all 1's is empty flash?

> Technically the pattern for empty
> block could be different on some devices so it might be nice to have
> some sort of is_empty_block somewhere in MTD. So far all 1 is what
> everything uses otherwise ubifs would fail ;-)

Yes - agreed. An is_empty_block interface would overcome my encryption
issues, perhaps assist with ECC issues, and support flash devices that
aren't all 1's when erased (if they existed).

>
>>>
>>>
>>> It's good idea to weight options when dealing with encryption and erasing blocks
>>>  - one is to erase unused blocks in the hope that reading them back
>>> and getting stale data will then be more difficult
>>>  - other is to keep unused blocks intact as much as possible, avoid
>>> half-full blocks and only erase blocks that are about to be written so
>>> usage patterns are not as obvious - this only applies if the block
>>> usage tables and other FTL data is also encrypted.
>>
>>
>> In trying to keep encryption decoupled from UBI, my encryption layer
>> is rather transparent, and thus oblivious to the structures of the
>> data being encrypted and the significance of erasing blocks. Thus here
>> the FTL data (assuming you mean the filesystem meta data?) is all
>> encrypted. The downside here is that I don't know when the filesystem
>> has finished using blocks.
>
> Erasing happens in UBIFS presumably by invoking a block erase on the
> MTD that turns the block in all 1 (except for stuck bits). That is
> outside of write().

No - UBIFS doesn't ever tell MTD or UBI to erase. It uses the
ubi_leb_map|unmap interfaces and UBI manages erasing when needed.

>
> So the encryption would not deal with erasing blocks but you might
> want to tune ubifs parameters that deal with erasing blocks. That is
> independent of actually encrypting the data but might affect the
> overall strength of your encryption scheme.

Yes.

>
>>
>>>
>>>
>>> Encrypting the FTL data may also pose a problem since the structure of
>>> the tables may be fixed to some extent and may be used as additional
>>> leverage if weakness is found in a cipher algorithm.
>>
>>
>> Is encrypting the FTL undesirable?
>
> It is probably desirable. It's just something to be aware of and
> something to consult with an encryption expert if you want to make
> sure your encryption scheme is really sound.

Thanks,

Andrew Murray

>
>>
>>>
>>>
>>> On 11 August 2015 at 07:38, Timo Ketola <Timo.Ketola@exertus.fi> wrote:
>
>>> > Why not postprocess the data so that the encrypted FF becomes FF again
>>> > like this:
>>> >
>>> > Lets say clear text data is I, encrypted data is O, encryption function
>>> > is e() and decryption function is d(). Then, what is normally done, is
>>> > of course:
>>> >
>>> > Write: O = e(I)
>>> > Read: I = d(O)
>>> >
>>> > Calculate F = ~e(FF), where F is encrypted and inverted version of 'all
>>> > bits set' (FF) data, and modify writing and reading:
>>> >
>>> > Write: O = e(I) ^ F
>>> > Read: I = d(O ^ F)
>>> >
>>> > Now encrypting FF input results in FF output and vice versa.
>>> >
>>> > Just wanted to introduce an idea.
>>>
>>> This post-processing function is in general hard to obtain. Since the
>>> encryption is seeded with block offset you would have to encrypt empty
>>> block with the block-specific iv to obtain the transform function and
>>> then apply it to the encrypted block to obtain flash data when writing
>>> and apply inversion on the flash data to obtain encrypted block when
>>> reading. So you would basically encrypt/decrypt each block twice.
>>>
>>> Without some serious research you cannot tell how this damages the
>>> security of the encryption.
>>
>>
>> I shy'd away from taking this approach as I'm not a crypto expert and
>> didn't want to tie UBI encryption to a specific crypto algorithm.
>>
>
> You could do this with any encryption algorithm provided you can get a
> reversible transform (such as XOR negation) that converts arbitrary
> block in all 1. The result may, however, be dodgy unless you are
> really sure what you are doing.
>
> Thanks
>
> Michal

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-11 12:40           ` Andrew Murray
@ 2015-08-11 13:24             ` Michal Suchanek
  2015-08-12  9:39               ` Andrew Murray
  0 siblings, 1 reply; 19+ messages in thread
From: Michal Suchanek @ 2015-08-11 13:24 UTC (permalink / raw)
  To: Andrew Murray; +Cc: Richard Weinberger, linux-mtd@lists.infradead.org

On 11 August 2015 at 14:40, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
> On 11 August 2015 at 12:39, Michal Suchanek <hramrach@gmail.com> wrote:
>> On 11 August 2015 at 13:03, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
>>> On 11 August 2015 at 11:23, Michal Suchanek <hramrach@gmail.com> wrote:
>>>>
>>>> On 11 August 2015 at 11:47, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
>>>> > On 11 August 2015 at 07:30, Richard Weinberger
>>>> > <richard.weinberger@gmail.com> wrote:
>>>> >> On Mon, Aug 10, 2015 at 9:56 PM, Andrew Murray
>>>> >> <amurray@embedded-bits.co.uk> wrote:
>>>>
>>>> >>
>>>> >>>  - Encryption in UBI was preferred as it removed the complexity from userspace,
>>>> >>>    though I suppose there is no reason why this can't be done within the MTD
>>>> >>>    layer rather than in UBI and thus benefit all MTD users.
>>>> >>
>>>> >> I'm not sure if UBI is the right layer for that. I'd do it in MTD to
>>>> >> have a dm_crypt like
>>>> >> MTD driver. At best it will be compatible with dm_crypt's userspace tools.
>>>> >
>>>> > I've not investigated this - but it seems plausible. My 'interception'
>>>> > layer works immediately above the MTD layer (i.e. the patchset looks a
>>>> > bit like this):
>>>> >
>>>> > -       err = mtd_write(ubi->mtd, addr, len, &written, buf);
>>>> > +       err = mtd_crypt_write(ubi, addr, len, &written, buf);
>>>> >
>>>> > Thus this code could just as easily exist on the other size of mtd_*.
>>>> >
>>>> > However this still leaves the issue of assumptions all over the place
>>>> > that 0xFF == erased flash. Is it best to risk the collision that
>>>> > crypto output could generate all 0xFFs - or is it best to overcome the
>>>> > assumptions by fixing them up as an initial stage? I suspect the later
>>>> > - besides helping the encryption problem it also means you aren't
>>>> > relying on the content of the flash to determine its state (I wonder
>>>> > if this could also be used to solve other issues where its difficult
>>>> > to tell if flash is erased - e.g. inverted ECCs?).
>>>>
>>>> It's probably good idea to treat empty blocks as truly empty and only
>>>> encrypt data blocks.
>>>
>>>
>>> In some sense this is what I already have. My encryption hooks exist
>>> just above the MTD layer and only get used if UBI attempts to write
>>> data to MTD. One of my implementations doesn't encrypt data from UBI
>>> if it contains all 0xFFs. (Of course the problem is reading those
>>> 0xFFs back from flash and determining if its empty data or encrypted
>>> data).
>>
>> Then that's wrong. The data is all 1 and the encrypted data should be
>> not.
>
> For clarify I've tried two approaches:
>
> 1) An implementation where encryption is provided unconditionally,
> thus all 1's become something other than all 1's
>
> 2) An implementation where encryption is conditional based on the
> data. If the plain text is all 1's then I do not encrypt.
>
> I stuck with implementation 2) as it was less invasive. In order to
> support implementation 2) (which also works), I needed to modify much
> more code including UBIFS. This was needed as approach 2) breaks the
> assumption that all 1's is empty flash - this is the case because when
> reading and decrypting, all 1's becomes garbage - this breaks lots.
> For example torture_peb erases flash and reads it back to ensure it
> was erased - with encryption this test fails as the erased flash is
> decrypted and becomes garbage.
>
> I feel that the second approach is the correct one, I'm sensing you also agree?

Yes, it sounds correct. Unfortunately, it seems it needs to straighten
a wart of UBIFS.

>
>>
>> If the FTL says it's a datablock it's a datablock.
>>
>> If all blocks are empty you should format that flash.
>>
>> Flash with FTL structure has non-empty blocks that contain the FTL data.
>
> Can you clarify what FTL is? Are you referring to a data structure
> devised by the software layers that is stored on flash, a flash
> translation layer or something in hardware?

Flash translation layer - something that deals accounting of used and
unused blocks, erase counts, wear levelling, etc. It can be in
software like with MTD in kernel or in hardware like on SD cards. It
needs some bookkeeping on the raw flash medium to know which blocks
are used and keep the erase counters, etc.

>
>>
>>>
>>>>
>>>> It is also possible to write all 1s to
>>>> unencrypted ubifs block and the filesystem should handle it. The block
>>>> will technically remain empty but it can be interpreted as data. If
>>>> this does not work properly for non-encrypted flash it should be fixed
>>>> I guess.
>>>
>>>
>>> Yes this is what I do, if data is provided to UBI containing all 1s, I
>>> write it to flash as all 1s. Upon reading it back I also return it to
>>> UBI without modification. UBIFS expects this - and attempting to mount
>>> UBIFS on a empty UBI volume will otherwise fail unless I do this (as
>>> UBIFS makes the assumption that all 1s is empty flash).
>>>
>>> I'm slightly uncomfortable that UBIFS makes this assumption, it seems
>>> to be present as a very sensible sanity check - but it ties back in
>>> the characteristics of flash back into a layer that should be
>>> abstracted away from it.
>>
>> Ubifs deals directly with physical flash erase blocks so it cannot
>> possibly be abstracted from it.
>
> I thought the purpose of UBI was to abstract the horribleness of flash
> from its users. Such that filesystems could care about filesystem
> related things rather than working around bad blocks and the other
> nasties of flash.
>
> UBIFS deals with logical erase blocks doesn't it? It doesn't need to
> worry about erasing flash before using it, etc. So why should UBIFS
> need to understand that all 1's is empty flash?

If it's supposed to be abstracetd then the abstraction leaks here.

If it dealt with logical blocks it could not enumerate the physical
blocks of the volume and ascertain all of them are all 1s

Actually it seems that this is indeed supposed to be tracked by UBI
and the UBIFS driver is just tied way too tightly to it.

Thanks

Michal

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-11 13:24             ` Michal Suchanek
@ 2015-08-12  9:39               ` Andrew Murray
  0 siblings, 0 replies; 19+ messages in thread
From: Andrew Murray @ 2015-08-12  9:39 UTC (permalink / raw)
  To: Michal Suchanek; +Cc: Richard Weinberger, linux-mtd@lists.infradead.org

On 11 August 2015 at 14:24, Michal Suchanek <hramrach@gmail.com> wrote:
> On 11 August 2015 at 14:40, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
>> On 11 August 2015 at 12:39, Michal Suchanek <hramrach@gmail.com> wrote:
>>> On 11 August 2015 at 13:03, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
>>>> On 11 August 2015 at 11:23, Michal Suchanek <hramrach@gmail.com> wrote:
>>>>>
>>>>> On 11 August 2015 at 11:47, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
>>>>> > On 11 August 2015 at 07:30, Richard Weinberger
>>>>> > <richard.weinberger@gmail.com> wrote:
>>>>> >> On Mon, Aug 10, 2015 at 9:56 PM, Andrew Murray
>>>>> >> <amurray@embedded-bits.co.uk> wrote:
>>>>>
>>>>> >>
>>>>> >>>  - Encryption in UBI was preferred as it removed the complexity from userspace,
>>>>> >>>    though I suppose there is no reason why this can't be done within the MTD
>>>>> >>>    layer rather than in UBI and thus benefit all MTD users.
>>>>> >>
>>>>> >> I'm not sure if UBI is the right layer for that. I'd do it in MTD to
>>>>> >> have a dm_crypt like
>>>>> >> MTD driver. At best it will be compatible with dm_crypt's userspace tools.
>>>>> >
>>>>> > I've not investigated this - but it seems plausible. My 'interception'
>>>>> > layer works immediately above the MTD layer (i.e. the patchset looks a
>>>>> > bit like this):
>>>>> >
>>>>> > -       err = mtd_write(ubi->mtd, addr, len, &written, buf);
>>>>> > +       err = mtd_crypt_write(ubi, addr, len, &written, buf);
>>>>> >
>>>>> > Thus this code could just as easily exist on the other size of mtd_*.
>>>>> >
>>>>> > However this still leaves the issue of assumptions all over the place
>>>>> > that 0xFF == erased flash. Is it best to risk the collision that
>>>>> > crypto output could generate all 0xFFs - or is it best to overcome the
>>>>> > assumptions by fixing them up as an initial stage? I suspect the later
>>>>> > - besides helping the encryption problem it also means you aren't
>>>>> > relying on the content of the flash to determine its state (I wonder
>>>>> > if this could also be used to solve other issues where its difficult
>>>>> > to tell if flash is erased - e.g. inverted ECCs?).
>>>>>
>>>>> It's probably good idea to treat empty blocks as truly empty and only
>>>>> encrypt data blocks.
>>>>
>>>>
>>>> In some sense this is what I already have. My encryption hooks exist
>>>> just above the MTD layer and only get used if UBI attempts to write
>>>> data to MTD. One of my implementations doesn't encrypt data from UBI
>>>> if it contains all 0xFFs. (Of course the problem is reading those
>>>> 0xFFs back from flash and determining if its empty data or encrypted
>>>> data).
>>>
>>> Then that's wrong. The data is all 1 and the encrypted data should be
>>> not.
>>
>> For clarify I've tried two approaches:
>>
>> 1) An implementation where encryption is provided unconditionally,
>> thus all 1's become something other than all 1's
>>
>> 2) An implementation where encryption is conditional based on the
>> data. If the plain text is all 1's then I do not encrypt.
>>
>> I stuck with implementation 2) as it was less invasive. In order to
>> support implementation 2) (which also works), I needed to modify much
>> more code including UBIFS. This was needed as approach 2) breaks the
>> assumption that all 1's is empty flash - this is the case because when
>> reading and decrypting, all 1's becomes garbage - this breaks lots.
>> For example torture_peb erases flash and reads it back to ensure it
>> was erased - with encryption this test fails as the erased flash is
>> decrypted and becomes garbage.
>>
>> I feel that the second approach is the correct one, I'm sensing you also agree?
>
> Yes, it sounds correct. Unfortunately, it seems it needs to straighten
> a wart of UBIFS.

This seems the general consensus.

>
>>
>>>
>>> If the FTL says it's a datablock it's a datablock.
>>>
>>> If all blocks are empty you should format that flash.
>>>
>>> Flash with FTL structure has non-empty blocks that contain the FTL data.
>>
>> Can you clarify what FTL is? Are you referring to a data structure
>> devised by the software layers that is stored on flash, a flash
>> translation layer or something in hardware?
>
> Flash translation layer - something that deals accounting of used and
> unused blocks, erase counts, wear levelling, etc. It can be in
> software like with MTD in kernel or in hardware like on SD cards. It
> needs some bookkeeping on the raw flash medium to know which blocks
> are used and keep the erase counters, etc.

OK I'm on the same page as you.

>
>>
>>>
>>>>
>>>>>
>>>>> It is also possible to write all 1s to
>>>>> unencrypted ubifs block and the filesystem should handle it. The block
>>>>> will technically remain empty but it can be interpreted as data. If
>>>>> this does not work properly for non-encrypted flash it should be fixed
>>>>> I guess.
>>>>
>>>>
>>>> Yes this is what I do, if data is provided to UBI containing all 1s, I
>>>> write it to flash as all 1s. Upon reading it back I also return it to
>>>> UBI without modification. UBIFS expects this - and attempting to mount
>>>> UBIFS on a empty UBI volume will otherwise fail unless I do this (as
>>>> UBIFS makes the assumption that all 1s is empty flash).
>>>>
>>>> I'm slightly uncomfortable that UBIFS makes this assumption, it seems
>>>> to be present as a very sensible sanity check - but it ties back in
>>>> the characteristics of flash back into a layer that should be
>>>> abstracted away from it.
>>>
>>> Ubifs deals directly with physical flash erase blocks so it cannot
>>> possibly be abstracted from it.
>>
>> I thought the purpose of UBI was to abstract the horribleness of flash
>> from its users. Such that filesystems could care about filesystem
>> related things rather than working around bad blocks and the other
>> nasties of flash.
>>
>> UBIFS deals with logical erase blocks doesn't it? It doesn't need to
>> worry about erasing flash before using it, etc. So why should UBIFS
>> need to understand that all 1's is empty flash?
>
> If it's supposed to be abstracetd then the abstraction leaks here.
>
> If it dealt with logical blocks it could not enumerate the physical
> blocks of the volume and ascertain all of them are all 1s
>
> Actually it seems that this is indeed supposed to be tracked by UBI
> and the UBIFS driver is just tied way too tightly to it.

The feedback that I've gained from this discussion is:

- Encryption shouldn't be conditional - this causes issues with 0xFF
assumptions - but that should be addressed. This seems like a first
step to prepare the way for encryption
- Encryption could be provided in the MTD layer rather than the UBI
layer - this will likely expose more 0xFF assumptions in other MTD
users besides UBI (that wish to use encryption).
- I've not heard anything that suggests this shouldn't be done in the
kernel (rather than userspace) - or that the mechanism of encrypting
through intercepting reads/writes is terrible. (Though interested to
hear further thoughts on this.)

I will investigate the occurrences of 0xFF assumptions with a view to
consolidate them - such that the logic for determining empty flash is
in one place that can consider the presence of encryption (or other
factors).

Thanks,

Andrew Murray

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-10 19:56 [RFC] mtd: ubi: UBI Encryption Andrew Murray
  2015-08-11  5:38 ` Timo Ketola
  2015-08-11  6:30 ` Richard Weinberger
@ 2015-08-12 17:19 ` David Gstir
  2015-08-14  7:25   ` Michal Suchanek
  2015-08-14  7:48   ` Andrew Murray
  2 siblings, 2 replies; 19+ messages in thread
From: David Gstir @ 2015-08-12 17:19 UTC (permalink / raw)
  To: Andrew Murray; +Cc: linux-mtd

Hi!

I'm fairly new to MTD/UBI/UBIFS, but I have a bit of knowledge of cryptography, so I'll focus on that. :)

> On 10.08.2015, at 21:56, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
> 
> - The current implementation  encrypts with cbc(aes) across fixed sized
>   units of hdrs_min_io_size - it also uses an IV based on the physcial offset
>   of the block and user provided IV. The key/IV is read from a fixed location
>   in flash. I'm not sure of the best way to manage/locate keys - this is
>   clearly a hack.

So does this mean the IV for each encryption-unit is chosen from a predictable sequence?
If this is the case, using CBC makes the encryption vulnerable to certain types of attack.
In general CBC requires random IVs to be secure. A common solution for disk encryption is to use ESSIV [1].

Also, how do you handle cases where hdrs_min_io_size is not a multiple of the cipher's block size?
I'm not a 100% sure how often this will happen, but in theory it could happen and will cause problems when using modes like CBC (len(ciphertext) >= len(plaintext)).

[1] https://en.wikipedia.org/wiki/Disk_encryption_theory#Encrypted_salt-sector_initialization_vector_.28ESSIV.29

> - Encryption in UBI was preferred as it removed the complexity from userspace,
>   though I suppose there is no reason why this can't be done within the MTD
>   layer rather than in UBI and thus benefit all MTD users.

Generally speaking, I'd argue for moving encryption to the highest layer possible. So, if you exclusively use UBIFS and need encryption, add it to UBIFS or even your userspace application.
The main reason for this is that disk encryption on lower layers (e.g block-level) has fewer security guarantees, like no authentication of encrypted data for example. So it cannot prevent "evil maid" attacks.
There is a nice writeup on this topic here: [2], which focuses on the commonly used XTS mode, but makes valid points for disk encryption in general.

So, when you do encryption in UBI or MTD, be aware of the security implications and know your thread model.

[2] http://sockpuppet.org/blog/2014/04/30/you-dont-want-xts/


Thanks,
David

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-12 17:19 ` David Gstir
@ 2015-08-14  7:25   ` Michal Suchanek
  2015-08-14  8:08     ` Andrew Murray
  2015-08-14  7:48   ` Andrew Murray
  1 sibling, 1 reply; 19+ messages in thread
From: Michal Suchanek @ 2015-08-14  7:25 UTC (permalink / raw)
  To: David Gstir; +Cc: Andrew Murray, MTD Maling List

Hello,

On 12 August 2015 at 19:19, David Gstir <david@sigma-star.at> wrote:
> Hi!
>

>> - Encryption in UBI was preferred as it removed the complexity from userspace,
>>   though I suppose there is no reason why this can't be done within the MTD
>>   layer rather than in UBI and thus benefit all MTD users.
>
> Generally speaking, I'd argue for moving encryption to the highest layer possible. So, if you exclusively use UBIFS and need encryption, add it to UBIFS or even your userspace application.
> The main reason for this is that disk encryption on lower layers (e.g block-level) has fewer security guarantees, like no authentication of encrypted data for example. So it cannot prevent "evil maid" attacks.
> There is a nice writeup on this topic here: [2], which focuses on the commonly used XTS mode, but makes valid points for disk encryption in general.
>
> So, when you do encryption in UBI or MTD, be aware of the security implications and know your thread model.
>
> [2] http://sockpuppet.org/blog/2014/04/30/you-dont-want-xts/

Thanks for the pointers.

Obviously, full disk encryption is not ideal but it's also last resort
in the sense that you can cut & paste state of the art solution and it
works on any kind of disk with minimum requirements on the implementor
and reviewer.

>From the notes about internal and external flash it seems that this is
upposed to protect against removing an external flash memory which is
attached to a device. This is probably a decent solution for the
problem. Knowing more details would help here.

Adding encryption to UBIFS itself is much more difficult.

Adding encryption to every application is not really feasible unless
you have a single-purpose device with one application.

Thanks

Michal

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-12 17:19 ` David Gstir
  2015-08-14  7:25   ` Michal Suchanek
@ 2015-08-14  7:48   ` Andrew Murray
  2015-08-15 11:43     ` David Gstir
  1 sibling, 1 reply; 19+ messages in thread
From: Andrew Murray @ 2015-08-14  7:48 UTC (permalink / raw)
  To: David Gstir; +Cc: linux-mtd

On 12 August 2015 at 18:19, David Gstir <david@sigma-star.at> wrote:
> Hi!
>
> I'm fairly new to MTD/UBI/UBIFS, but I have a bit of knowledge of cryptography, so I'll focus on that. :)

Thanks!

>
>> On 10.08.2015, at 21:56, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
>>
>> - The current implementation  encrypts with cbc(aes) across fixed sized
>>   units of hdrs_min_io_size - it also uses an IV based on the physcial offset
>>   of the block and user provided IV. The key/IV is read from a fixed location
>>   in flash. I'm not sure of the best way to manage/locate keys - this is
>>   clearly a hack.
>
> So does this mean the IV for each encryption-unit is chosen from a predictable sequence?
> If this is the case, using CBC makes the encryption vulnerable to certain types of attack.
> In general CBC requires random IVs to be secure. A common solution for disk encryption is to use ESSIV [1].

At present the IV is a fixed IV salt combined (^) with a block offset.
Thus if you can deduce the salt you could work out the IV.

ESSIV is apparently supported in kernel, I'll take a look and see if I
can also use this.

>
> Also, how do you handle cases where hdrs_min_io_size is not a multiple of the cipher's block size?
> I'm not a 100% sure how often this will happen, but in theory it could happen and will cause problems when using modes like CBC (len(ciphertext) >= len(plaintext)).

At present I don't - I don't even have any assertions to guard this. I
guess this is something I need to further consider - thanks.

>
> [1] https://en.wikipedia.org/wiki/Disk_encryption_theory#Encrypted_salt-sector_initialization_vector_.28ESSIV.29
>
>> - Encryption in UBI was preferred as it removed the complexity from userspace,
>>   though I suppose there is no reason why this can't be done within the MTD
>>   layer rather than in UBI and thus benefit all MTD users.
>
> Generally speaking, I'd argue for moving encryption to the highest layer possible. So, if you exclusively use UBIFS and need encryption, add it to UBIFS or even your userspace application.
> The main reason for this is that disk encryption on lower layers (e.g block-level) has fewer security guarantees, like no authentication of encrypted data for example. So it cannot prevent "evil maid" attacks.
> There is a nice writeup on this topic here: [2], which focuses on the commonly used XTS mode, but makes valid points for disk encryption in general.
>
> So, when you do encryption in UBI or MTD, be aware of the security implications and know your thread model.

Thanks for the links. Ultimately it will be up to the
user/system-designer to determine the best scheme (taking into account
above considerations) - I'm hoping to provide a kernel based option
that will be useful to some. There are already good userspace
facilities for encryption I don't see this deprecating those.

Thanks,

Andrew Murray

>
> [2] http://sockpuppet.org/blog/2014/04/30/you-dont-want-xts/
>
>
> Thanks,
> David
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-14  7:25   ` Michal Suchanek
@ 2015-08-14  8:08     ` Andrew Murray
  2015-08-14 14:11       ` Murali Karicheri
  0 siblings, 1 reply; 19+ messages in thread
From: Andrew Murray @ 2015-08-14  8:08 UTC (permalink / raw)
  To: Michal Suchanek; +Cc: David Gstir, MTD Maling List

On 14 August 2015 at 08:25, Michal Suchanek <hramrach@gmail.com> wrote:
> Hello,
>
> On 12 August 2015 at 19:19, David Gstir <david@sigma-star.at> wrote:
>> Hi!
>>
>
>>> - Encryption in UBI was preferred as it removed the complexity from userspace,
>>>   though I suppose there is no reason why this can't be done within the MTD
>>>   layer rather than in UBI and thus benefit all MTD users.
>>
>> Generally speaking, I'd argue for moving encryption to the highest layer possible. So, if you exclusively use UBIFS and need encryption, add it to UBIFS or even your userspace application.
>> The main reason for this is that disk encryption on lower layers (e.g block-level) has fewer security guarantees, like no authentication of encrypted data for example. So it cannot prevent "evil maid" attacks.
>> There is a nice writeup on this topic here: [2], which focuses on the commonly used XTS mode, but makes valid points for disk encryption in general.
>>
>> So, when you do encryption in UBI or MTD, be aware of the security implications and know your thread model.
>>
>> [2] http://sockpuppet.org/blog/2014/04/30/you-dont-want-xts/
>
> Thanks for the pointers.
>
> Obviously, full disk encryption is not ideal but it's also last resort
> in the sense that you can cut & paste state of the art solution and it
> works on any kind of disk with minimum requirements on the implementor
> and reviewer.
>
> From the notes about internal and external flash it seems that this is
> upposed to protect against removing an external flash memory which is
> attached to a device. This is probably a decent solution for the
> problem. Knowing more details would help here.

Yes that was the intent of this work - to provide moderate protection
from someone removing an external flash chip and retrieving its data.

Many SOMs provide internal flash and a means of protection such as
security fuses that prevent JTAG access etc. Thus to retrieve the
internal key you would need an increased level of skill (i.e. rather
than removing an external chip and wiring it to another board (perhaps
a reference board) - you'd now have to skim the SOM or similar).
Combined with application/system security you can also make it much
harder for the user to access the external flash via software hacking
such as accessing a console prompt, exploiting vulnerabilities etc.

This implementation provides a balance between
implementation/integration complexity and protection. If other users
can benefit from this then it's something they can just switch on -
rather than having to add a variety of userspace components to their
distribution, etc.

At present my implementation makes an assumption that the key is
stored in another MTD partition, I took this approach because it was
easy. However I'm not sure if this is useful to the general case - or
if the general case is in fact users on SOMs protecting external flash
with keys on internal flash. It would be possible to extend the
UBI/MTD API to add ioctl's (or similar) such that a user can provide a
key during mount/attach time. This makes it slightly more complex for
a user to use - as rather than updating a .config they now have to add
an initramfs that reads a key from one MTD partition and provides it
to the kernel.

>
> Adding encryption to UBIFS itself is much more difficult.

Whilst experimenting with this stuff, I actually was successful in
adding encryption to UBIFS.

To support compression, UBIFS provides functions thats get called when
data needs to be compressed. This calls use a crypto framework, e.g.
crypto_comp_compress. I extended this to actually use encryption. This
worked - though it only encrypted the data and not file names etc, I
also recall that compression can be turned off or not always applied.

Much like the UBI encryption - I could have also tried to provided
UBIFS encryption by intercepting the ubi_leb_write and ubi_write
calls.

>
> Adding encryption to every application is not really feasible unless
> you have a single-purpose device with one application.


Thanks,

Andrew Murray

>
> Thanks
>
> Michal

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-14  8:08     ` Andrew Murray
@ 2015-08-14 14:11       ` Murali Karicheri
  2015-08-14 14:28         ` Richard Weinberger
  0 siblings, 1 reply; 19+ messages in thread
From: Murali Karicheri @ 2015-08-14 14:11 UTC (permalink / raw)
  To: Andrew Murray, Michal Suchanek; +Cc: David Gstir, MTD Maling List

Andrew,

> This implementation provides a balance between
> implementation/integration complexity and protection. If other users
> can benefit from this then it's something they can just switch on -
> rather than having to add a variety of userspace components to their
> distribution, etc.
>
> At present my implementation makes an assumption that the key is
> stored in another MTD partition, I took this approach because it was
Did you looked into how to use the encryption key from secure storage in 
SoC itself such as one from OTP memory?  In such case, is there an API 
retrieve the key from such storage?

Murali

> easy. However I'm not sure if this is useful to the general case - or
> if the general case is in fact users on SOMs protecting external flash
> with keys on internal flash. It would be possible to extend the
> UBI/MTD API to add ioctl's (or similar) such that a user can provide a
> key during mount/attach time. This makes it slightly more complex for
> a user to use - as rather than updating a .config they now have to add
> an initramfs that reads a key from one MTD partition and provides it
> to the kernel.
>
>>
>> Adding encryption to UBIFS itself is much more difficult.
>
> Whilst experimenting with this stuff, I actually was successful in
> adding encryption to UBIFS.
>
> To support compression, UBIFS provides functions thats get called when
> data needs to be compressed. This calls use a crypto framework, e.g.
> crypto_comp_compress. I extended this to actually use encryption. This
> worked - though it only encrypted the data and not file names etc, I
> also recall that compression can be turned off or not always applied.
>
> Much like the UBI encryption - I could have also tried to provided
> UBIFS encryption by intercepting the ubi_leb_write and ubi_write
> calls.
>
>>
>> Adding encryption to every application is not really feasible unless
>> you have a single-purpose device with one application.
>
>
> Thanks,
>
> Andrew Murray
>
>>
>> Thanks
>>
>> Michal
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>


-- 
Murali Karicheri
Linux Kernel, Keystone

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-14 14:11       ` Murali Karicheri
@ 2015-08-14 14:28         ` Richard Weinberger
  0 siblings, 0 replies; 19+ messages in thread
From: Richard Weinberger @ 2015-08-14 14:28 UTC (permalink / raw)
  To: Murali Karicheri
  Cc: Andrew Murray, Michal Suchanek, David Gstir, MTD Maling List

On Fri, Aug 14, 2015 at 4:11 PM, Murali Karicheri <m-karicheri2@ti.com> wrote:
> Did you looked into how to use the encryption key from secure storage in SoC
> itself such as one from OTP memory?  In such case, is there an API retrieve
> the key from such storage?

The key setup should really be done in userspace.
Exactly how dm_crypt does.
Then your key can be stored anywhere.

-- 
Thanks,
//richard

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC] mtd: ubi: UBI Encryption
  2015-08-14  7:48   ` Andrew Murray
@ 2015-08-15 11:43     ` David Gstir
  0 siblings, 0 replies; 19+ messages in thread
From: David Gstir @ 2015-08-15 11:43 UTC (permalink / raw)
  To: Andrew Murray; +Cc: linux-mtd

Andrew,

> On 14.08.2015, at 09:48, Andrew Murray <amurray@embedded-bits.co.uk> wrote:

> ESSIV is apparently supported in kernel, I'll take a look and see if I
> can also use this.

Yep. ESSIV is also supported by dm_crypt for instance.

>> Also, how do you handle cases where hdrs_min_io_size is not a multiple of the cipher's block size?
>> I'm not a 100% sure how often this will happen, but in theory it could happen and will cause problems when using modes like CBC (len(ciphertext) >= len(plaintext)).
> 
> At present I don't - I don't even have any assertions to guard this. I
> guess this is something I need to further consider - thanks.

The XTS mode is often used for disk encryption, since it is able to handle such cases.
XTS is also supported by the kernel crypto framework.

>>> - Encryption in UBI was preferred as it removed the complexity from userspace,
>>>  though I suppose there is no reason why this can't be done within the MTD
>>>  layer rather than in UBI and thus benefit all MTD users.
>> 
>> Generally speaking, I'd argue for moving encryption to the highest layer possible. So, if you exclusively use UBIFS and need encryption, add it to UBIFS or even your userspace application.
>> The main reason for this is that disk encryption on lower layers (e.g block-level) has fewer security guarantees, like no authentication of encrypted data for example. So it cannot prevent "evil maid" attacks.
>> There is a nice writeup on this topic here: [2], which focuses on the commonly used XTS mode, but makes valid points for disk encryption in general.
>> 
>> So, when you do encryption in UBI or MTD, be aware of the security implications and know your thread model.
> 
> Thanks for the links. Ultimately it will be up to the
> user/system-designer to determine the best scheme (taking into account
> above considerations) - I'm hoping to provide a kernel based option
> that will be useful to some. There are already good userspace
> facilities for encryption I don't see this deprecating those.

Sure, it depends on the use case.
As Michal noted, FDE is a last resort and always better than no encryption at all. :)

Thanks,
David

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2015-08-15 11:44 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-10 19:56 [RFC] mtd: ubi: UBI Encryption Andrew Murray
2015-08-11  5:38 ` Timo Ketola
2015-08-11  6:22   ` Richard Weinberger
2015-08-11  6:35     ` Timo Ketola
2015-08-11  6:30 ` Richard Weinberger
2015-08-11  9:47   ` Andrew Murray
2015-08-11 10:23     ` Michal Suchanek
2015-08-11 11:03       ` Andrew Murray
2015-08-11 11:39         ` Michal Suchanek
2015-08-11 12:40           ` Andrew Murray
2015-08-11 13:24             ` Michal Suchanek
2015-08-12  9:39               ` Andrew Murray
2015-08-12 17:19 ` David Gstir
2015-08-14  7:25   ` Michal Suchanek
2015-08-14  8:08     ` Andrew Murray
2015-08-14 14:11       ` Murali Karicheri
2015-08-14 14:28         ` Richard Weinberger
2015-08-14  7:48   ` Andrew Murray
2015-08-15 11:43     ` David Gstir

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox