On Fri, 21 Jun 2024 at 21:36, Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > On Fri, Jun 21, 2024 at 08:49:56PM +0300, Dmitry Baryshkov wrote: > > On Fri, 21 Jun 2024 at 19:31, Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > > > > > On Fri, Jun 21, 2024 at 07:06:25PM +0300, Dmitry Baryshkov wrote: > > > > On Fri, 21 Jun 2024 at 18:39, Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > > > > > > > > > On Fri, Jun 21, 2024 at 06:16:37PM +0300, Dmitry Baryshkov wrote: > > > > > > On Fri, 21 Jun 2024 at 07:47, Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > On Thu, Jun 20, 2024 at 02:57:40PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > > > > > > > > > > > > Is it possible to use both kind of keys when working on standard mode? > > > > > > > > > > > > If not, it should be the user who selects what type of keys to be used. > > > > > > > > > > > > Enforcing this via DT is not a way to go. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Unfortunately, that support is not there yet. When you say user, do > > > > > > > > > > > you mean to have it as a filesystem mount option? > > > > > > > > > > > > > > > > > > > > During cryptsetup time. When running e.g. cryptsetup I, as a user, would like > > > > > > > > > > to be able to use either a hardware-wrapped key or a standard key. > > > > > > > > > > > > > > > > > > > > > > > > > > > > What we are looking for with these patches is for per-file/folder encryption using fscrypt policies. > > > > > > > > > Cryptsetup to my understanding supports only full-disk , and does not support FBE (File-Based) > > > > > > > > > > > > > > > > I must admit, I mostly used dm-crypt beforehand, so I had to look at > > > > > > > > fscrypt now. Some of my previous comments might not be fully > > > > > > > > applicable. > > > > > > > > > > > > > > > > > Hence the idea here is that we mount an unencrypted device (with the inlinecrypt option that indicates inline encryption is supported) > > > > > > > > > And specify policies (links to keys) for different folders. > > > > > > > > > > > > > > > > > > > > The way the UFS/EMMC crypto layer is designed currently is that, this > > > > > > > > > > > information is needed when the modules are loaded. > > > > > > > > > > > > > > > > > > > > > > https://lore.kernel.org/all/20231104211259.17448-2-ebiggers@xxxxxxxxxx > > > > > > > > > > > /#Z31drivers:ufs:core:ufshcd-crypto.c > > > > > > > > > > > > > > > > > > > > I see that the driver lists capabilities here. E.g. that it supports HW-wrapped > > > > > > > > > > keys. But the line doesn't specify that standard keys are not supported. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Those are capabilities that are read from the storage controller. However, wrapped keys > > > > > > > > > Are not a standard in the ICE JEDEC specification, and in most cases, is a value add coming > > > > > > > > > from the SoC. > > > > > > > > > > > > > > > > > > QCOM SOC and firmware currently does not support both kinds of keys in the HWKM mode. > > > > > > > > > That is something we are internally working on, but not available yet. > > > > > > > > > > > > > > > > I'd say this is a significant obstacle, at least from my point of > > > > > > > > view. I understand that the default might be to use hw-wrapped keys, > > > > > > > > but it should be possible for the user to select non-HW keys if the > > > > > > > > ability to recover the data is considered to be important. Note, I'm > > > > > > > > really pointing to the user here, not to the system integrator. So > > > > > > > > using DT property or specifying kernel arguments to switch between > > > > > > > > these modes is not really an option. > > > > > > > > > > > > > > > > But I'd really love to hear some feedback from linux-security and/or > > > > > > > > linux-fscrypt here. > > > > > > > > > > > > > > > > In my humble opinion the user should be able to specify that the key > > > > > > > > is wrapped using the hardware KMK. Then if the hardware has already > > > > > > > > started using the other kind of keys, it should be able to respond > > > > > > > > with -EINVAL / whatever else. Then the user can evict previously > > > > > > > > programmed key and program a desired one. > > > > > > > > > > > > > > > > > > Also, I'd have expected that hw-wrapped keys are handled using trusted > > > > > > > > > > keys mechanism (see security/keys/trusted-keys/). Could you please point > > > > > > > > > > out why that's not the case? > > > > > > > > > > > > > > > > > > > > > > > > > > > > I will evaluate this. > > > > > > > > > But my initial response is that we currently cannot communicate to our TPM directly from HLOS, but > > > > > > > > > goes through QTEE, and I don't think our qtee currently interfaces with the open source tee > > > > > > > > > driver. The interface is through QCOM SCM driver. > > > > > > > > > > > > > > > > Note, this is just an API interface, see how it is implemented for the > > > > > > > > CAAM hardware. > > > > > > > > > > > > > > > > > > > > > > The problem is that this patchset was sent out without the patches that add the > > > > > > > block and filesystem-level framework for hardware-wrapped inline encryption > > > > > > > keys, which it depends on. So it's lacking context. The proposed framework can > > > > > > > be found at > > > > > > > https://lore.kernel.org/linux-block/20231104211259.17448-1-ebiggers@xxxxxxxxxx/T/#u > > > > > > > > > > > > Thank you. I have quickly skimmed through the patches, but I didn't > > > > > > review them thoroughly. Maybe the patchset already implements the > > > > > > interfaces that I'm thinking about. In such a case please excuse me. I > > > > > > will give it a more thorough look later today. > > > > > > > > > > > > > As for why "trusted keys" aren't used, they just aren't helpful here. "Trusted > > > > > > > keys" are based around a model where the kernel can request that keys be sealed > > > > > > > and unsealed using a trust source, and the kernel gets access to the raw > > > > > > > unsealed keys. Hardware-wrapped inline encryption keys use a different model > > > > > > > where the kernel never gets access to the raw keys. They also have the concept > > > > > > > of ephemeral wrapping which does not exist in "trusted keys". And they need to > > > > > > > be properly integrated with the inline encryption framework in the block layer. > > > > > > > > > > > > Then what exactly does qcom_scm_derive_sw_secret() do? Does it rewrap > > > > > > the key under some other key? > > > > > > > > > > It derives a secret for functionality such as filenames encryption that can't > > > > > use inline encryption. > > > > > > > > > > > I had the feeling that there are two separate pieces of functionality > > > > > > being stuffed into a single patchset and into a single solution. > > > > > > > > > > > > First one is handling the keys. I keep on thinking that there should > > > > > > be a separate software interface to unseal the key and rewrap it under > > > > > > an ephemeral key. > > > > > > > > > > There is. That's what the BLKCRYPTOPREPAREKEY ioctl is for. > > > > > > > > > > > Some hardware might permit importing raw keys. > > > > > > > > > > That's what BLKCRYPTOIMPORTKEY is for. > > > > > > > > > > > Other hardware might insist on generating the keys on-chip so that raw keys > > > > > > can never be used. > > > > > > > > > > And that's what BLKCRYPTOGENERATEKEY is for. > > > > > > > > Again, this might be answered somewhere, but why can't we use keyctl > > > > for handling the keys and then use a single IOCTL to point the block > > > > device to the key in the keyring? > > > > > > All the same functionality would need to be supported, and I think that > > > shoehorning it into the keyrings service instead of just adding new ioctls would > > > be more difficult. The keyrings service was not designed for this use case. > > > We've already had a lot of problems trying to take advantage of the keyrings > > > service in fscrypt previously. The keyrings service is something that sounds > > > useful but really isn't all that useful. > > > > I would be really interested in reading or listening to any kind of > > summary or parts of the issues. > > I'm slightly pushy towards keyctl / keyrings, because it already > > provides support for different kinds of key wrapping and key > > management. Encrypted keys, trusted keys - those are all kinds of key > > management, which either will be missing or will have to be > > reimplemented for block layers. > > > > I know that keyrings are clumsy and not that logical, but then their > > API needs to be improved. Just ignoring the existing mechanisms sounds > > like a bad idea. > > One thing to keep in mind is that keyring service keys can't be used directly > for storage encryption. So if a component that manages storage encryption (such > as fscrypt or dm-crypt) is given a keyring service key, in practice it has to > extract the payload from the keyring service key, and not use the keyring > service key any further. So the keyring service can, at most, serve as a way to > create and prepare the key, and after that it doesn't serve a purpose. Yes, this sounds good enough. > > (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. > 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? > > > By "a single IOCTL to point the block device to the key in the keyring", you > > > seem to be referring to configuring full block device encryption with a single > > > key. That's not something that's supported by the upstream kernel yet, and it's > > > not related to this patchset; currently only fscrypt supports inline encryption. > > > > I see that dm has at least some provisioning and hooks for > > CONFIG_BLK_INLINE_ENCRYPTION. Thus I thought that it's possible to use > > inline encryption through DM. > > device-mapper supports passing through the inline encryption support of > underlying devices in certain cases, but it doesn't yet support using it itself. I see. I was confused by the dm code then. Please excuse me. > > > > > > 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). -- With best wishes Dmitry
