Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <5c4fbafb1daa45f2faf60c7d587cd23c53d9393c.camel@linux.vnet.ibm.com>
Subject: Re: [PATCH] block: Fix regression in sed-opal for a saved key.
From:   Greg Joyce <gjoyce@linux.vnet.ibm.com>
Reply-To: gjoyce@linux.vnet.ibm.com
To:     Milan Broz <gmazyland@gmail.com>, linux-block@vger.kernel.org
Cc:     jonathan.derrick@linux.dev, axboe@kernel.dk,
        linux-kernel@vger.kernel.org, Ondrej Kozina <okozina@redhat.com>
Date:   Wed, 04 Oct 2023 15:54:09 -0500
In-Reply-To: <20231003100209.380037-1-gmazyland@gmail.com>
References: <20231003100209.380037-1-gmazyland@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@redhat.com>
> Signed-off-by: Milan Broz <gmazyland@gmail.com>
> ---
>  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.

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