diff mbox series

block: Fix regression in sed-opal for a saved key.

Message ID 20231003100209.380037-1-gmazyland@gmail.com
State New
Headers
Series block: Fix regression in sed-opal for a saved key. |

Commit Message

Milan Broz Oct. 3, 2023, 10:02 a.m. UTC 
  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(-)

Comments

Milan Broz Oct. 4, 2023, 2:07 p.m. UTC | #1 
On 10/3/23 12:02, 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.

Just forgot to mention - this is regression in 6.6-rc,
breaking our OPAL cryptsetup support, so  I think this
should go into next 6.6 rc.

Stable versions are ok, only 6.6-rc is affected currently.

Milan


> 
> 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);
>   	mutex_unlock(&dev->dev_lock);
>   
>   	return ret;
Jens Axboe Oct. 4, 2023, 8:28 p.m. UTC | #2 
On 10/3/23 4:02 AM, 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.

Greg, please review this.
Greg Joyce Oct. 4, 2023, 8:54 p.m. UTC | #3 
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;
Milan Broz Oct. 5, 2023, 6:58 a.m. UTC | #4 
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@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.

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 :-)

Milan

> 
>>   	mutex_unlock(&dev->dev_lock);
>>
>>   	return ret;
>
Greg Joyce Oct. 5, 2023, 5:58 p.m. UTC | #5 
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@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.
> 
> 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;
Milan Broz Oct. 11, 2023, 8:30 a.m. UTC | #6 
On 10/5/23 19:58, Greg Joyce wrote:
> 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@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.
>>
>> 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.


Jens, what's the status of this patch?

It is clear regression in 6.6 (I forgot to add regression list, fixed now.)

For reference, the original report and patch is here
#regzbot link: https://lore.kernel.org/linux-block/20231003100209.380037-1-gmazyland@gmail.com/
#regzbot ^introduced 3bfeb6125664

Thanks,
Milan
Ondrej Kozina Oct. 13, 2023, 9:39 a.m. UTC | #7 
Hi Jens,

On 03/10/2023 12:02, Milan Broz wrote:
> Fixes: 3bfeb6125664 ("block: sed-opal: keyring support for SED keys")

could we please get this in 6.6 in time for final release? This 
regression blocks cryptsetup from correctly locking sed-opal locaking 
range when device needs to be deactivated.

Is there anything that needs to be done for it to get it in 6.6?

Thank you
O.
Jens Axboe Oct. 13, 2023, 2:17 p.m. UTC | #8 
On 10/11/23 2:30 AM, Milan Broz wrote:
> On 10/5/23 19:58, Greg Joyce wrote:
>> 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@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.
>>>
>>> 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.
> 
> 
> Jens, what's the status of this patch?
> 
> It is clear regression in 6.6 (I forgot to add regression list, fixed now.)
> 
> For reference, the original report and patch is here
> #regzbot link: https://lore.kernel.org/linux-block/20231003100209.380037-1-gmazyland@gmail.com/
> #regzbot ^introduced 3bfeb6125664

Was waiting on Greg to ack/review it, which it looks like he kind of
has. But would've been nice with a formal ack on it. I've queued it up
now.
Jens Axboe Oct. 13, 2023, 2:17 p.m. UTC | #9 
On Tue, 03 Oct 2023 12:02:09 +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.
> 
> [...]

Applied, thanks!

[1/1] block: Fix regression in sed-opal for a saved key.
      commit: 4eaf0932c69bdc56d2c2af30404f9c918b1f6295

Best regards,
Linux regression tracking (Thorsten Leemhuis) Oct. 13, 2023, 2:38 p.m. UTC | #10 
[TLDR: This mail in primarily relevant for Linux kernel regression
tracking. See link in footer if these mails annoy you.]

On 13.10.23 16:17, Jens Axboe wrote:

> Was waiting on Greg to ack/review it, which it looks like he kind of
> has. But would've been nice with a formal ack on it. I've queued it up
> now.

#regzbot fix: block: Fix regression in sed-opal for a saved key.
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.
diff mbox series

Patch

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);
 	mutex_unlock(&dev->dev_lock);
 
 	return ret;