Re: [PATCH] block: Fix regression in sed-opal for a saved key.

[Greg Joyce]

On Thu, 2023-10-05 at 08:58 +0200, Milan Broz wrote:
> On 10/4/23 22:54, Greg Joyce wrote:
> > On Tue, 2023-10-03 at 12:02 +0200, Milan Broz wrote:
> > > The commit 3bfeb61256643281ac4be5b8a57e9d9da3db4335
> > > introduced the use of keyring for sed-opal.
> > > 
> > > Unfortunately, there is also a possibility to save
> > > the Opal key used in opal_lock_unlock().
> > > 
> > > This patch switches the order of operation, so the cached
> > > key is used instead of failure for opal_get_key.
> > > 
> > > The problem was found by the cryptsetup Opal test recently
> > > added to the cryptsetup tree.
> > > 
> > > Fixes: 3bfeb6125664 ("block: sed-opal: keyring support for SED
> > > keys")
> > > Tested-by: Ondrej Kozina <okozina@xxxxxxxxxx>
> > > Signed-off-by: Milan Broz <gmazyland@xxxxxxxxx>
> > > ---
> > >   block/sed-opal.c | 7 +++----
> > >   1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/block/sed-opal.c b/block/sed-opal.c
> > > index 6d7f25d1711b..04f38a3f5d95 100644
> > > --- a/block/sed-opal.c
> > > +++ b/block/sed-opal.c
> > > @@ -2888,12 +2888,11 @@ static int opal_lock_unlock(struct
> > > opal_dev
> > > *dev,
> > >   	if (lk_unlk->session.who > OPAL_USER9)
> > >   		return -EINVAL;
> > > 
> > > -	ret = opal_get_key(dev, &lk_unlk->session.opal_key);
> > > -	if (ret)
> > > -		return ret;
> > >   	mutex_lock(&dev->dev_lock);
> > >   	opal_lock_check_for_saved_key(dev, lk_unlk);
> > > -	ret = __opal_lock_unlock(dev, lk_unlk);
> > > +	ret = opal_get_key(dev, &lk_unlk->session.opal_key);
> > > +	if (!ret)
> > > +		ret = __opal_lock_unlock(dev, lk_unlk);
> > 
> > This is relying on opal_get_key() returning 0 to decide if
> > __opal_lock_unlock() is called. Is this really what you want? It
> > seems
> > that you would want to unlock if the key is a LUKS key, even if
> > opal_get_key() returns non-zero.
> 
> I think it is ok. That was logic introduced in your keyring patch
> anyway.
> 
> I just fixed that if key is cached (stored in OPAL struct), that key
> is used
> and subsequent opal_get_key() does nothing, returning 0.
> 
> The story is here that both OPAL lock and unlock need key, while LUKS
> logic never required key for lock (deactivation), so we rely on the
> cached
> OPAL key here. We do not need any key stored for unlocking (that is
> always
> decrypted from a keyslot)
> (I think requiring key for locking range is a design mistake in OPAL
> but
> that's not relevant for now :-)

Okay, if the key is such that opal_get_key() always returns 0, then I
agree there isn't an issue.

Greg

> 
> Milan
> 
> > >   	mutex_unlock(&dev->dev_lock);
> > > 
> > >   	return ret;