On Tue, 2022-12-20 at 09:23 -0500, James Bottomley wrote: > On Tue, 2022-12-20 at 09:13 -0500, Mimi Zohar wrote: > > On Tue, 2022-12-20 at 09:03 -0500, James Bottomley wrote: > > > On Tue, 2022-12-20 at 08:54 -0500, Mimi Zohar wrote: > > > > On Tue, 2022-12-20 at 07:50 -0500, James Bottomley wrote: > > > > > On Tue, 2022-12-20 at 12:03 +0530, Sughosh Ganu wrote: > > > [...] > > > > > > I was able to load the key after clearing the keyring. Thanks > > > > > > James and Mimi for your pointers. > > > > > > > > > > Actually, I think this is a bug in trusted keys. Add on > > > > > existing key is supposed to go through the update path. If the > > > > > path doesn't exist it returns -EEXIST. Trusted keys have an > > > > > update path but they return - EINVAL if the trusted key command > > > > > is anything but update (which is used to reseal a key). > > > > > Obviously this is incorrect and the code should be returning - > > > > > EEXIST for a key we refuse to update to match every other key > > > > > type. > > > > > > > > Re-loading an existing key was previously permitted. Obviously > > > > this changed at some point. Any "fixes" should point out when > > > > it changed. > > > > > > Git history doesn't think so. It thinks when you added trusted > > > keys with d00a1c72f7f4661212299e6cb132dfa58030bcdb the update path > > > already had the -EINVAL return, so reload has always failed this > > > way unless we were doing a reseal update. We could certainly > > > permit overwriting an existing key with load, but that would be a > > > more extensive change. > > > > Replacing existing keys/keyrings was part of the infrastructure. I > > don't think this change has anything to do with trusted type keys. > > The ability of replacing keys/keyrings was one of the main reasons > > for trusted keyrings (dot prefixed keyrings). > > Keys can only be replaced by calling the ->update() helper for the key > type. If that doesn't exist keyctl add will return -EEXIST (behaviour > for at least the last 12 years). Most key type update routines do > unconditionally update, so the error they return is the same error they > would have returned for an add of a non existent key (EINVAL if the > payload is too large, for instance). The trusted keys ->update() > helper (trusted_update()) only allows update if the trusted operation > is update, so they've always failed a load with EINVAL going back to > the original commit I quoted. At no time that I can find has there > ever been a modification to this supporting updating trusted keys with > anything other than an update trusted operation. So they've supported > changing the sealing parameters (PCR values) but not changing the > payload. This contrasts with user keys where add of a new payload on > an existing key changes the payload. Yes, my mistake. With your change, it's now returning "add_key: File exists". Reviewed-by: Mimi Zohar <zohar@xxxxxxxxxxxxx> -- thanks, Mimi
