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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <59535b4b-9f07-44c5-a7da-e6b2fc1c67bb@gmail.com>
Date:   Thu, 5 Oct 2023 08:58:30 +0200
From:   Milan Broz <gmazyland@...il.com>
To:     gjoyce@...ux.vnet.ibm.com, linux-block@...r.kernel.org
Cc:     jonathan.derrick@...ux.dev, axboe@...nel.dk,
        linux-kernel@...r.kernel.org, Ondrej Kozina <okozina@...hat.com>
Subject: Re: [PATCH] block: Fix regression in sed-opal for a saved key.

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@...hat.com>
>> Signed-off-by: Milan Broz <gmazyland@...il.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;
>