[v2] sed-opal: allow using IOC_OPAL_SAVE for locking too

Commit Message

Luca Boccassi Dec. 3, 2022, 12:12 a.m. UTC

From: Luca Boccassi <[e-mail]>

Usually when closing a crypto device (eg: dm-crypt with LUKS) the
volume key is not required, as it requires root privileges anyway, and
root can deny access to a disk in many ways regardless. Requiring the
volume key to lock the device is a peculiarity of the OPAL
specification.

Given we might already have saved the key if the user requested it via
the 'IOC_OPAL_SAVE' ioctl, we can use that key to lock the device if no
key was provided here and the locking range matches, and the user sets
the appropriate flag with 'IOC_OPAL_SAVE'. This allows integrating OPAL
with tools and libraries that are used to the common behaviour and do
not ask for the volume key when closing a device.

Callers can always pass a non-zero key and it will be used regardless,
as before.

Suggested-by: Štěpán Horáček <[e-mail]>
Signed-off-by: Luca Boccassi <[e-mail]>
---
v2: break on 80chr for optimal rendering on 1970s green monochrome monitors
    make the new functionality dependent on a new flag that has to be
    passed to IOC_OPAL_SAVE, using reserved bits in its ioctl struct

 block/sed-opal.c              | 32 ++++++++++++++++++++++++++++++++
 include/uapi/linux/sed-opal.h |  3 ++-
 2 files changed, 34 insertions(+), 1 deletion(-)

Comments

Christoph Hellwig Dec. 5, 2022, 7:09 a.m. UTC | #1

On Sat, Dec 03, 2022 at 12:12:43AM +0000, [e-mail] wrote:
> +	 * Usually when closing a crypto device (eg: dm-crypt with LUKS) the volume

As said last time, please correctly format your block comments so that
they don't spill out of 80 characters for every single line and become
completely unreadable.

> +	if (lk_unlk->l_state == OPAL_LK &&
> +			lk_unlk->session.opal_key.key_len == 0) {
> +		struct opal_suspend_data *iter;
> +
> +		setup_opal_dev(dev);
> +		list_for_each_entry(iter, &dev->unlk_lst, node) {
> +			if (iter->unlk.save_for_lock &&
> +					iter->lr == lk_unlk->session.opal_key.lr &&
> +					iter->unlk.session.opal_key.key_len > 0) {
> +				lk_unlk->session.opal_key.key_len =
> +					iter->unlk.session.opal_key.key_len;
> +				memcpy(lk_unlk->session.opal_key.key,
> +					iter->unlk.session.opal_key.key,
> +					iter->unlk.session.opal_key.key_len);
> +				break;
> +			}
> +		}

And please split this logic into a helper.

>  	__u32 l_state;
> -	__u8 __align[4];
> +	__u8 save_for_lock:1; /* if in IOC_OPAL_SAVE will also use key to lock */

Please never use bitfields in ABIs, the psABI rules for them are just
a mess to start with, and not filling all of them leads to leaks of
stack contents as well.  Just turn the entire 4 bytes into a flags
field and then just bitmasks.

Patch

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 9bdb833e5817..3a81754a0fdf 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -2470,6 +2470,38 @@  static int opal_lock_unlock(struct opal_dev *dev,
 		return -EINVAL;
 
 	mutex_lock(&dev->dev_lock);
+
+	/*
+	 * Usually when closing a crypto device (eg: dm-crypt with LUKS) the volume
+	 * key is not required, as it requires root privileges anyway, and root can
+	 * deny access to a disk in many ways regardless. Requiring the volume key
+	 * to lock the device is a peculiarity of the OPAL specification.
+	 * Given we might already have saved the key if the user requested it via
+	 * the 'IOC_OPAL_SAVE' ioctl, we can use that key to lock the device if no
+	 * key was provided here, the locking range matches and the appropriate
+	 * flag was passed with 'IOC_OPAL_SAVE'. This allows integrating OPAL with
+	 * tools and libraries that are used to the common behaviour and do not ask
+	 * for the volume key when closing a device.
+	 */
+	if (lk_unlk->l_state == OPAL_LK &&
+			lk_unlk->session.opal_key.key_len == 0) {
+		struct opal_suspend_data *iter;
+
+		setup_opal_dev(dev);
+		list_for_each_entry(iter, &dev->unlk_lst, node) {
+			if (iter->unlk.save_for_lock &&
+					iter->lr == lk_unlk->session.opal_key.lr &&
+					iter->unlk.session.opal_key.key_len > 0) {
+				lk_unlk->session.opal_key.key_len =
+					iter->unlk.session.opal_key.key_len;
+				memcpy(lk_unlk->session.opal_key.key,
+					iter->unlk.session.opal_key.key,
+					iter->unlk.session.opal_key.key_len);
+				break;
+			}
+		}
+	}
+
 	ret = __opal_lock_unlock(dev, lk_unlk);
 	mutex_unlock(&dev->dev_lock);
 
diff --git a/include/uapi/linux/sed-opal.h b/include/uapi/linux/sed-opal.h
index 2573772e2fb3..fa604fb07f50 100644
--- a/include/uapi/linux/sed-opal.h
+++ b/include/uapi/linux/sed-opal.h
@@ -76,7 +76,8 @@  struct opal_user_lr_setup {
 struct opal_lock_unlock {
 	struct opal_session_info session;
 	__u32 l_state;
-	__u8 __align[4];
+	__u8 save_for_lock:1; /* if in IOC_OPAL_SAVE will also use key to lock */
+	__u8 __align[3];
 };
 
 struct opal_new_pw {