On Thu, Sep 24 2020 at 3:17am -0400,
Satya Tangirala <[email protected]> wrote:
> On Wed, Sep 23, 2020 at 09:14:39PM -0400, Mike Snitzer wrote:
> > On Mon, Sep 21 2020 at 8:32pm -0400,
> > Eric Biggers <[email protected]> wrote:
> >
> > > On Wed, Sep 09, 2020 at 11:44:21PM +0000, Satya Tangirala wrote:
> > > > From: Eric Biggers <[email protected]>
> > > >
> > > > Update the device-mapper core to support exposing the inline crypto
> > > > support of the underlying device(s) through the device-mapper device.
> > > >
> > > > This works by creating a "passthrough keyslot manager" for the dm
> > > > device, which declares support for encryption settings which all
> > > > underlying devices support. When a supported setting is used, the bio
> > > > cloning code handles cloning the crypto context to the bios for all the
> > > > underlying devices. When an unsupported setting is used, the blk-crypto
> > > > fallback is used as usual.
> > > >
> > > > Crypto support on each underlying device is ignored unless the
> > > > corresponding dm target opts into exposing it. This is needed because
> > > > for inline crypto to semantically operate on the original bio, the data
> > > > must not be transformed by the dm target. Thus, targets like dm-linear
> > > > can expose crypto support of the underlying device, but targets like
> > > > dm-crypt can't. (dm-crypt could use inline crypto itself, though.)
> > > >
> > > > When a key is evicted from the dm device, it is evicted from all
> > > > underlying devices.
> > > >
> > > > Signed-off-by: Eric Biggers <[email protected]>
> > > > Co-developed-by: Satya Tangirala <[email protected]>
> > > > Signed-off-by: Satya Tangirala <[email protected]>
> > >
> > > Looks good as far as Satya's changes from my original patch are concerned.
> > >
> > > Can the device-mapper maintainers take a look at this?
> >
> > In general it looks like these changes were implemented very carefully
> > and are reasonable if we _really_ want to enable passing through inline
> > crypto.
> >
> > I do have concerns about the inability to handle changes at runtime (due
> > to a table reload that introduces new devices without the encryption
> > settings the existing devices in the table are using). But the fallback
> > mechanism saves it from being a complete non-starter.
>
> Unfortunately, the fallback doesn't completely handle that situation
> right now. The DM device could be suspended while an upper layer like
> fscrypt is doing something like "checking if encryption algorithm 'A'
> is supported by the DM device". It's possible that fscrypt thinks
> the DM device supports 'A' even though the DM device is suspended, and
> the table is about to be reloaded to introduce a new device that doesn't
> support 'A'. Before the DM device is resumed with the new table, fscrypt
> might send a bio that uses encryption algorithm 'A' without initializing
> the blk-crypto-fallback ciphers for 'A', because it believes that the DM
> device supports 'A'. When the bio gets processed by the DM (or when
> blk-crypto does its checks to decide whether to use the fallback on that
> bio), the bio will fail because the fallback ciphers aren't initialized.
>
> Off the top of my head, one thing we could do is to always allocate the
> fallback ciphers when the device mapper is the target device for the bio
> (by maybe adding a "encryption_capabilities_may_change_at_runtime" flag
> to struct blk_keyslot_manager that the DM will set to true, and that
> the block layer will check for and decide to appropriately allocate
> the fallback ciphers), although this does waste memory on systems
> where we know the DM device tables will never change....
Yeah, I agree that'd be too wasteful.
> This patch also doesn't handle the case when the encryption capabilities
> of the new table are a superset of the old capabilities. Currently, a
> DM device's capabilities can only shrink after the device is initially
> created. They can never "expand" to make use of capabilities that might
> be added due to introduction of new devices via table reloads. I might
> be forgetting something I thought of before, but looking at it again
> now, I don't immediately see anything wrong with expanding the
> advertised capabilities on table reload....I'll look carefully into that
> again.
OK, that'd be good (expanding capabilities on reload).
And conversely, you _could_ also fail a reload if the new device(s)
don't have capabilities that are in use by the active table.
> > Can you help me better understand the expected consumer of this code?
> > If you have something _real_ please be explicit. It makes justifying
> > supporting niche code like this more tolerable.
>
> So the motivation for this code was that Android currently uses a device
> mapper target on top of a phone's disk for user data. On many phones,
> that disk has inline encryption support, and it'd be great to be able to
> make use of that. The DM device configuration isn't changed at runtime.
OK, which device mapper target is used?
Thanks,
Mike
On Thu, Sep 24, 2020 at 09:46:49AM -0400, Mike Snitzer wrote:
> > > Can you help me better understand the expected consumer of this code?
> > > If you have something _real_ please be explicit. It makes justifying
> > > supporting niche code like this more tolerable.
> >
> > So the motivation for this code was that Android currently uses a device
> > mapper target on top of a phone's disk for user data. On many phones,
> > that disk has inline encryption support, and it'd be great to be able to
> > make use of that. The DM device configuration isn't changed at runtime.
>
> OK, which device mapper target is used?
There are several device-mapper targets that Android can require for the
"userdata" partition -- potentially in a stack of more than one:
dm-linear: required for Dynamic System Updates
(https://developer.android.com/topic/dsu)
dm-bow: required for User Data Checkpoints on ext4
(https://source.android.com/devices/tech/ota/user-data-checkpoint)
(https://patchwork.kernel.org/patch/10838743/)
dm-default-key: required for metadata encryption
(https://source.android.com/security/encryption/metadata)
We're already carrying this patchset in the Android common kernels since late
last year, as it's required for inline encryption to work when any of the above
is used. So this is something that is needed and is already being used.
Now, you don't have to "count" dm-bow and dm-default-key since they aren't
upstream; that leaves dm-linear. But hopefully the others at least show that
architecturally, dm-linear is just the initial use case, and this patchset also
makes it easy to pass through inline crypto on any other target that can support
it (basically, anything that doesn't change the data itself as it goes through).
- Eric
On Thu, Sep 24 2020 at 11:45am -0400,
Eric Biggers <[email protected]> wrote:
> On Thu, Sep 24, 2020 at 09:46:49AM -0400, Mike Snitzer wrote:
> > > > Can you help me better understand the expected consumer of this code?
> > > > If you have something _real_ please be explicit. It makes justifying
> > > > supporting niche code like this more tolerable.
> > >
> > > So the motivation for this code was that Android currently uses a device
> > > mapper target on top of a phone's disk for user data. On many phones,
> > > that disk has inline encryption support, and it'd be great to be able to
> > > make use of that. The DM device configuration isn't changed at runtime.
> >
> > OK, which device mapper target is used?
>
> There are several device-mapper targets that Android can require for the
> "userdata" partition -- potentially in a stack of more than one:
>
> dm-linear: required for Dynamic System Updates
> (https://developer.android.com/topic/dsu)
>
> dm-bow: required for User Data Checkpoints on ext4
> (https://source.android.com/devices/tech/ota/user-data-checkpoint)
> (https://patchwork.kernel.org/patch/10838743/)
>
> dm-default-key: required for metadata encryption
> (https://source.android.com/security/encryption/metadata)
Please work with all google stakeholders to post the latest code for the
dm-bow and dm-default-key targets and I'll work through them.
I think the more code we have to inform DM core's implementation the
better off we'll be in the long run. Could also help improve these
targets as a side-effect of additional review.
I know I largely ignored dm-bow before but that was more to do with
competing tasks, etc. I'll try my best...
> We're already carrying this patchset in the Android common kernels since late
> last year, as it's required for inline encryption to work when any of the above
> is used. So this is something that is needed and is already being used.
>
> Now, you don't have to "count" dm-bow and dm-default-key since they aren't
> upstream; that leaves dm-linear. But hopefully the others at least show that
> architecturally, dm-linear is just the initial use case, and this patchset also
> makes it easy to pass through inline crypto on any other target that can support
> it (basically, anything that doesn't change the data itself as it goes through).
Sure, that context really helps.
About "basically, anything that doesn't change the data itself as it
goes through": could you be a bit more precise? Very few DM targets
actually change the data as associated bios are remapped.
I'm just wondering if your definition of "doesn't change the data"
includes things more subtle than the data itself?
Thanks,
Mike
On Thu, Sep 24, 2020 at 12:16:24PM -0400, Mike Snitzer wrote:
> On Thu, Sep 24 2020 at 11:45am -0400,
> Eric Biggers <[email protected]> wrote:
>
> > On Thu, Sep 24, 2020 at 09:46:49AM -0400, Mike Snitzer wrote:
> > > > > Can you help me better understand the expected consumer of this code?
> > > > > If you have something _real_ please be explicit. It makes justifying
> > > > > supporting niche code like this more tolerable.
> > > >
> > > > So the motivation for this code was that Android currently uses a device
> > > > mapper target on top of a phone's disk for user data. On many phones,
> > > > that disk has inline encryption support, and it'd be great to be able to
> > > > make use of that. The DM device configuration isn't changed at runtime.
> > >
> > > OK, which device mapper target is used?
> >
> > There are several device-mapper targets that Android can require for the
> > "userdata" partition -- potentially in a stack of more than one:
> >
> > dm-linear: required for Dynamic System Updates
> > (https://developer.android.com/topic/dsu)
> >
> > dm-bow: required for User Data Checkpoints on ext4
> > (https://source.android.com/devices/tech/ota/user-data-checkpoint)
> > (https://patchwork.kernel.org/patch/10838743/)
> >
> > dm-default-key: required for metadata encryption
> > (https://source.android.com/security/encryption/metadata)
>
> Please work with all google stakeholders to post the latest code for the
> dm-bow and dm-default-key targets and I'll work through them.
>
> I think the more code we have to inform DM core's implementation the
> better off we'll be in the long run. Could also help improve these
> targets as a side-effect of additional review.
>
> I know I largely ignored dm-bow before but that was more to do with
> competing tasks, etc. I'll try my best...
I'm not sure what happened with dm-bow; I'll check with the person maintaining
it.
We expect that dm-default-key would be controversial, since it's sort of a
layering violation; metadata encryption really should be a filesystem-level
thing. Satya has been investigating implementing it in filesystems instead.
I think we need to see how that turns out first.
> > We're already carrying this patchset in the Android common kernels since late
> > last year, as it's required for inline encryption to work when any of the above
> > is used. So this is something that is needed and is already being used.
> >
> > Now, you don't have to "count" dm-bow and dm-default-key since they aren't
> > upstream; that leaves dm-linear. But hopefully the others at least show that
> > architecturally, dm-linear is just the initial use case, and this patchset also
> > makes it easy to pass through inline crypto on any other target that can support
> > it (basically, anything that doesn't change the data itself as it goes through).
>
> Sure, that context really helps.
>
> About "basically, anything that doesn't change the data itself as it
> goes through": could you be a bit more precise? Very few DM targets
> actually change the data as associated bios are remapped.
>
> I'm just wondering if your definition of "doesn't change the data"
> includes things more subtle than the data itself?
The semantics expected by upper layers (e.g. filesystems) are that a "write" bio
that has an encryption context is equivalent to a "write" bio with no encryption
context that contains the data already encrypted. Similarly, a "read" bio with
an encryption context is equivalent to submitting a "read" bio with no
encryption context, then decrypting the resulting data.
blk-crypto-fallback obviously works in that way. However, when actual inline
encryption hardware is used, the encryption/decryption actually happens at the
lowest level in the stack.
To maintain the semantics in that case, the data can't be modified anywhere in
the stack. For example, if the data also passes through a dm-crypt target that
encrypted/decrypted the data (again) in software, that would break things.
You're right that it's a bit more than that, though. The targets also have to
behave the same way regardless of whether the data is already encrypted or not.
So if e.g. a target hashes the data, then it can't set
may_passthrough_inline_crypto, even if it doesn't change the data. It can't
sometimes be hashing the plaintext data and sometimes the ciphertext data.
(And also, storing hashes of the plaintext on-disk would be insecure, as it
would leak information that encryption is meant to protect.)
- Eric