Re: [PATCH v3 3/3] block: sed-opal: keyring support for SED keys

[Greg Joyce <gjoyce@linux.vnet.ibm.com>]

On Wed, 2022-11-30 at 08:00 +0100, Hannes Reinecke wrote:
> On 11/30/22 00:25, gjoyce@linux.vnet.ibm.com wrote:
> > From: Greg Joyce <gjoyce@linux.vnet.ibm.com>
> > 
> > Extend the SED block driver so it can alternatively
> > obtain a key from a sed-opal kernel keyring. The SED
> > ioctls will indicate the source of the key, either
> > directly in the ioctl data or from the keyring.
> > 
> > This allows the use of SED commands in scripts such as
> > udev scripts so that drives may be automatically unlocked
> > as they become available.
> > 
> > Signed-off-by: Greg Joyce <gjoyce@linux.vnet.ibm.com>
> > Reviewed-by: Jonathan Derrick <jonathan.derrick@linux.dev>
> > ---
> >   block/Kconfig                 |   1 +
> >   block/sed-opal.c              | 174
> > +++++++++++++++++++++++++++++++++-
> >   include/linux/sed-opal.h      |   3 +
> >   include/uapi/linux/sed-opal.h |   8 +-
> >   4 files changed, 183 insertions(+), 3 deletions(-)
> >  
> > +	ret = opal_get_key(dev, &opal_lrs->session.opal_key);
> > +	if (ret)
> > +		return ret;
> >   	mutex_lock(&dev->dev_lock);
> >   	setup_opal_dev(dev);
> >   	ret = execute_steps(dev, lr_steps, ARRAY_SIZE(lr_steps));
> > @@ -2622,6 +2759,14 @@ static int opal_set_new_pw(struct opal_dev
> > *dev, struct opal_new_pw *opal_pw)
> >   	ret = execute_steps(dev, pw_steps, ARRAY_SIZE(pw_steps));
> >   	mutex_unlock(&dev->dev_lock);
> >   
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* update keyring with new password */
> > +	ret = update_sed_opal_key(OPAL_AUTH_KEY,
> > +				  opal_pw->new_user_pw.opal_key.key,
> > +				  opal_pw-
> > >new_user_pw.opal_key.key_len);
> > +
> >   	return ret;
> >   }
> >   
> What about key revocation?
> You only allow to set a new key, but what happens with the old ones?

My understanding was that key_create_or_update() would not allow
duplicates so there shouldn't be old ones. Is that incorrect?

> 
> > +static int __init sed_opal_init(void)
> > +{
> > +	struct key *kr;
> > +
> > +	kr = keyring_alloc(".sed_opal",
> > +			   GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
> > current_cred(),
> > +			   (KEY_POS_ALL & ~KEY_POS_SETATTR) |
> > KEY_USR_VIEW |
> > +			   KEY_USR_READ | KEY_USR_SEARCH |
> > KEY_USR_WRITE,
> > +			   KEY_ALLOC_NOT_IN_QUOTA,
> > +			   NULL, NULL);
> > +	if (IS_ERR(kr))
> > +		return PTR_ERR(kr);
> > +
> > +	sed_opal_keyring = kr;
> > +
> > +	return 0;
> > +}
> > +late_initcall(sed_opal_init);
> 
> Shouldn't you free the keyring on exit?

The SED Opal driver is part of the block driver and does not build as a
module so it will not exit. I had looked at "blacklist" as an example
and saw that it allocated but did not free its keyring.

> Cheers,
> 
> Hannes

Thanks for the comments on the keyring. I'm not very familiar with the
keyring code, so I'd appreciate suggestions on code changes if any are
needed for your two comments.

-Greg