On Fri, Jun 21, 2024 at 11:52:01PM +0300, Dmitry Baryshkov wrote: > On Fri, Jun 21, 2024 at 08:14:41PM GMT, Eric Biggers wrote: > > On Fri, Jun 21, 2024 at 10:24:07PM +0300, Dmitry Baryshkov wrote: > > > > > > > > (fscrypt used to use the keyring service a bit more: it looked up a key whenever > > > > a file was opened, and it supported evicting per-file keys by revoking the > > > > corresponding keyring key. But this turned out to be totally broken. E.g., it > > > > didn't provide the correct semantics for filesystem encryption where the key > > > > should either be present or absent filesystem-wide.) > > > > > > > > We do need the ability to create HW-wrapped keys in long-term wrapped form, > > > > either via "generate" or "import", return those long-term wrapped keys to > > > > userspace so that they can be stored on-disk, and convert them into > > > > ephemerally-wrapped form so they can be used. It probably would be possible to > > > > support all of this through the keyrings service, but it would need a couple new > > > > key types: > > > > > > > > - One key type that can be instantiated with a raw key (or NULL to request > > > > generation of a key) and that automagically creates a long-term wrapped key > > > > and supports userspace reading it back. This would be vaguely similar to > > > > "trusted", but without any support for using the key directly. > > > > > > > > - One key type that can be instantiated using a long-term wrapped key which gets > > > > automagically converted to an ephemerally-wrapped key. This would be what is > > > > passed to other kernel subsystems. Functions specific to this key type would > > > > need to be provided for users to use. > > > > > > I think having one key type should be enough. The userspace loads / > > > generates&reads / wraps and reads back the 'exported' version wrapped > > > using the platform-specific key. In kernel the key is unsealed and > > > represented as binary key to be loaded to the hardware + a cookie for > > > the ephemeral key and device that have been used to wrap it. When > > > userspace asks the device to program the key, the cookie is verified > > > to match the device / ephemeral key and then the binary is programmed > > > to the hardware. Maybe it's enough to use the struct device as a > > > cookie. > > > > The long-term wrapped key has to be wiped from memory as soon as it's no longer > > needed. So it's hard to see how overloading a key type in this way can work, as > > the kernel can't know if userspace intends to read back the long-term wrapped > > key or not. > > Why? It should be user's decision. Pretty much in the same way as it's > done for all other keys. Sorry, I don't understand what your point is supposed to be here. It's certainly not okay to leave the long-term wrapped key in memory, since that destroys the security properties of hardware-wrapped keys. So we need to provide an API that makes it possible for the long-term wrapped key to be zeroized. The API you're proposing, as I understand it, wouldn't allow for that because the long-term wrapped key would remain in memory as long as the keyring service key exists, even when only the ephemerally-wrapped key is needed. > > > > > I think it would be possible, but it feels like a bit of a shoehorned API. The > > > > ioctls are a more straightforward solution. > > > > > > Are we going to have another set of IOCTLs for loading the encrypted > > > keys? keys sealed by TPM? > > > > Those features aren't compatible with hardware-wrapped inline encryption keys, > > so they're not really relevant here. BLKCRYPTOIMPORTKEY could support importing > > a keyring service key as an alternative to a raw key, of course. But this would > > just work similarly to fscrypt and dm-crypt where they just extract the payload, > > and the keyring service key plays no further role. > > Yes, extracting the payload is fine. As you wrote, dm-crypt and fscrypt > already do it in this way. But what I really don't like here is the idea > of having two different kinds of API having pretty close functionality. > In my opinion, all the keys should be handled via the existing keyrings > API and then imported via the BLKCRYPTOIMPORTKEY IOCTL. This way all > kinds of keys are handled in a similar way from user's point of view. But in that case all the proposed new BLKCRYPTO* ioctls are still needed. Your suggestion would just make them harder to use by requiring users to copy their key into a keyrings service key instead of just providing it directly in the ioctl. > > > > > > > Support for it will be added at some point, which will likely indeed take the > > > > > > form of an ioctl to set a key on a block device. But that would be the case > > > > > > even without HW-wrapped keys. And *requiring* the key to be given in a keyring > > > > > > (instead of just in a byte array passed to the ioctl) isn't very helpful, as it > > > > > > just makes the API harder to use. We've learned this from the fscrypt API > > > > > > already where we actually had to move away from the keyrings service in order to > > > > > > fix all the issues caused by it (see FS_IOC_ADD_ENCRYPTION_KEY). > > > > > > > > > > > > > > > > > > > > > > > Second part is the actual block interface. Gaurav wrote about > > > > > > > > > targeting fscrypt, but there should be no actual difference between > > > > > > > > > crypto targets. FDE or having a single partition encrypted should > > > > > > > > > probably work in the same way. Convert the key into blk_crypto_key > > > > > > > > > (including the cookie for the ephemeral key), program the key into the > > > > > > > > > slot, use the slot to en/decrypt hardware blocks. > > > > > > > > > > > > > > > > > > My main point is that the decision on the key type should be coming > > > > > > > > > from the user. > > > > > > > > > > > > > > > > That's exactly how it works. There is a block interface for specifying an > > > > > > > > inline encryption key along with each bio. The submitter of the bio can specify > > > > > > > > either a standard key or a HW-wrapped key. > > > > > > > > > > > > > > Not in this patchset. The ICE driver decides whether it can support > > > > > > > HW-wrapped keys or not and then fails to support other type of keys. > > > > > > > > > > > > > > > > > > > Sure, that's just a matter of hardware capabilities though, right? The block > > > > > > layer provides a way for drivers to declare which inline encryption capabilities > > > > > > they support. They can declare they support standard keys, HW-wrapped keys, > > > > > > both, or neither. If Qualcomm SoCs can't support both types of keys at the same > > > > > > time, that's unfortunate, but I'm not sure what your poitnt is. The user (e.g. > > > > > > fscrypt) still has control over whether they use the functionality that the > > > > > > hardware provides. > > > > > > > > > > It's a matter of policy. Harware / firmware doesn't support using both > > > > > kinds of keys concurrently, if I understood Gaurav's explanations > > > > > correctly. But the user should be able to make a judgement and use > > > > > non-hw-wrapped keys if it fits their requirements. The driver should > > > > > not make this kind of judgement. Note, this is not an issue of your > > > > > original patchset, but it's a driver flaw in this patchset. > > > > > > > > If the driver has to make a decision about which type of keys to support (due to > > > > the hardware and firmware supporting both but not at the same time), I think > > > > this will need to be done via a module parameter, e.g. > > > > qcom_ice.hw_wrapped_keys=1 to support HW-wrapped keys instead of standard keys. > > > > > > No, the user can not set modparams on e.g. Android device. In my > > > opinion it should be first-come-first-serve. If the user wants > > > hw-wrapped keys (and the platform is fine with that), then further > > > attempts to use raw keys should fail. If the user loads a raw key, > > > further attempts to set hw-wrapped key should fail (maybe until the > > > last raw key has been evicted from the hw, if such thing is actually > > > supported). > > > > That's not going to work. Upper layers need to know what the crypto > > capabilities are before they decide to use them. We can't randomly revoke > > capabilities based on who happened to get there first, as a user might have > > already checked the capabilities. Yes, the module parameter is a litle > > annoying, but it seems to be necessary here. > > Hmm. This is typical to have resource-limited capabilities. So yes, the > user checks the capabilities to identify whether the key type is > supported at all. But then _using_ the key might fail. For example > because all the hardware resources that are used by this key type are > already taken. That mustn't happen here, since finding out in the middle of an I/O request that inline encryption isn't supported is too late. That's what the crypto capabilities in struct blk_crypto_profile are for -- to allow users to check what is supported before trying to use it. > > > It is not a problem for Android > > because the type of encryption an Android device uses is set by the build > > anyway, which makes it no easier to change than module parameters. > > If AOSP misbehaves, it doesn't mean that we should follow the pattern. It's not "misbehaving" -- it's just an example of a system that configures the encryption centrally, which is common. (And the reason I brought up that the module parameter works for Android is because you claimed it wouldn't.) Again, needing a module parameter is unfortunate but I don't see any realistic way around it for these Qualcomm SoCs. - Eric
