[PATCH] block: sed-opal: reduce stack size of ioctl handler

[PATCH] block: sed-opal: reduce stack size of ioctl handler

[Arnd Bergmann]

When CONFIG_KASAN is in use, the sed_ioctl function uses unusually large stack,
as each possible ioctl argument gets its own stack area plus redzone:

block/sed-opal.c: In function 'sed_ioctl':
block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]

Moving the copy_from_user() calls into the individual functions has little
effect on readablility, but significantly reduces the stack size, with the
largest individual function (opal_enable_disable_shadow_mbr) now at
reasonable 456 bytes.

Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 block/sed-opal.c | 256 +++++++++++++++++++++++++++----------------------------
 1 file changed, 128 insertions(+), 128 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index bf1406e5159b..6b33d970ce5e 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -1957,7 +1957,7 @@ void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv)
 EXPORT_SYMBOL(init_opal_dev);
 
 static int opal_secure_erase_locking_range(struct opal_dev *dev,
-					   struct opal_session_info *opal_session)
+					   struct opal_session_info __user *arg)
 {
 	void *data[3] = { NULL };
 	static const opal_step erase_funcs[] = {
@@ -1968,14 +1968,18 @@ static int opal_secure_erase_locking_range(struct opal_dev *dev,
 		end_opal_session,
 		NULL,
 	};
+	struct opal_session_info opal_session;
 	int ret;
 
+	if (copy_from_user(&opal_session, arg, sizeof(opal_session)))
+		return -EFAULT;
+
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev, erase_funcs);
 
 	dev->func_data = data;
-	dev->func_data[1] = opal_session;
-	dev->func_data[2] = &opal_session->opal_key.lr;
+	dev->func_data[1] = &opal_session;
+	dev->func_data[2] = &opal_session.opal_key.lr;
 
 	ret = next(dev);
 	mutex_unlock(&dev->dev_lock);
@@ -1983,7 +1987,7 @@ static int opal_secure_erase_locking_range(struct opal_dev *dev,
 }
 
 static int opal_erase_locking_range(struct opal_dev *dev,
-				    struct opal_session_info *opal_session)
+				    struct opal_session_info __user *arg)
 {
 	void *data[3] = { NULL };
 	static const opal_step erase_funcs[] = {
@@ -1993,14 +1997,18 @@ static int opal_erase_locking_range(struct opal_dev *dev,
 		end_opal_session,
 		NULL,
 	};
+	struct opal_session_info opal_session;
 	int ret;
 
+	if (copy_from_user(&opal_session, arg, sizeof(opal_session)))
+		return -EFAULT;
+
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev, erase_funcs);
 
 	dev->func_data = data;
-	dev->func_data[1] = opal_session;
-	dev->func_data[2] = opal_session;
+	dev->func_data[1] = &opal_session;
+	dev->func_data[2] = &opal_session;
 
 	ret = next(dev);
 	mutex_unlock(&dev->dev_lock);
@@ -2008,7 +2016,7 @@ static int opal_erase_locking_range(struct opal_dev *dev,
 }
 
 static int opal_enable_disable_shadow_mbr(struct opal_dev *dev,
-					  struct opal_mbr_data *opal_mbr)
+					  struct opal_mbr_data __user *arg)
 {
 	void *func_data[6] = { NULL };
 	static const opal_step mbr_funcs[] = {
@@ -2021,34 +2029,42 @@ static int opal_enable_disable_shadow_mbr(struct opal_dev *dev,
 		end_opal_session,
 		NULL,
 	};
+	struct opal_mbr_data opal_mbr;
 	int ret;
 
-	if (opal_mbr->enable_disable != OPAL_MBR_ENABLE &&
-	    opal_mbr->enable_disable != OPAL_MBR_DISABLE)
+	if (copy_from_user(&opal_mbr, arg, sizeof(opal_mbr)))
+		return -EFAULT;
+
+	if (opal_mbr.enable_disable != OPAL_MBR_ENABLE &&
+	    opal_mbr.enable_disable != OPAL_MBR_DISABLE)
 		return -EINVAL;
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev, mbr_funcs);
 	dev->func_data = func_data;
-	dev->func_data[1] = &opal_mbr->key;
-	dev->func_data[2] = &opal_mbr->enable_disable;
-	dev->func_data[4] = &opal_mbr->key;
-	dev->func_data[5] = &opal_mbr->enable_disable;
+	dev->func_data[1] = &opal_mbr.key;
+	dev->func_data[2] = &opal_mbr.enable_disable;
+	dev->func_data[4] = &opal_mbr.key;
+	dev->func_data[5] = &opal_mbr.enable_disable;
 	ret = next(dev);
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
 
-static int opal_save(struct opal_dev *dev, struct opal_lock_unlock *lk_unlk)
+static int opal_save(struct opal_dev *dev, struct opal_lock_unlock __user *arg)
 {
 	struct opal_suspend_data *suspend;
+	struct opal_lock_unlock lk_unlk;
+
+	if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
+		return -EFAULT;
 
 	suspend = kzalloc(sizeof(*suspend), GFP_KERNEL);
 	if (!suspend)
 		return -ENOMEM;
 
-	suspend->unlk = *lk_unlk;
-	suspend->lr = lk_unlk->session.opal_key.lr;
+	suspend->unlk = lk_unlk;
+	suspend->lr = lk_unlk.session.opal_key.lr;
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev, NULL);
@@ -2058,7 +2074,7 @@ static int opal_save(struct opal_dev *dev, struct opal_lock_unlock *lk_unlk)
 }
 
 static int opal_add_user_to_lr(struct opal_dev *dev,
-			       struct opal_lock_unlock *lk_unlk)
+			       struct opal_lock_unlock *arg)
 {
 	void *func_data[3] = { NULL };
 	static const opal_step funcs[] = {
@@ -2068,20 +2084,24 @@ static int opal_add_user_to_lr(struct opal_dev *dev,
 		end_opal_session,
 		NULL
 	};
+	struct opal_lock_unlock lk_unlk;
 	int ret;
 
-	if (lk_unlk->l_state != OPAL_RO &&
-	    lk_unlk->l_state != OPAL_RW) {
+	if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
+		return -EFAULT;
+
+	if (lk_unlk.l_state != OPAL_RO &&
+	    lk_unlk.l_state != OPAL_RW) {
 		pr_err("Locking state was not RO or RW\n");
 		return -EINVAL;
 	}
-	if (lk_unlk->session.who < OPAL_USER1 &&
-	    lk_unlk->session.who > OPAL_USER9) {
+	if (lk_unlk.session.who < OPAL_USER1 &&
+	    lk_unlk.session.who > OPAL_USER9) {
 		pr_err("Authority was not within the range of users: %d\n",
-		       lk_unlk->session.who);
+		       lk_unlk.session.who);
 		return -EINVAL;
 	}
-	if (lk_unlk->session.sum) {
+	if (lk_unlk.session.sum) {
 		pr_err("%s not supported in sum. Use setup locking range\n",
 		       __func__);
 		return -EINVAL;
@@ -2090,14 +2110,14 @@ static int opal_add_user_to_lr(struct opal_dev *dev,
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev, funcs);
 	dev->func_data = func_data;
-	dev->func_data[1] = &lk_unlk->session.opal_key;
-	dev->func_data[2] = lk_unlk;
+	dev->func_data[1] = &lk_unlk.session.opal_key;
+	dev->func_data[2] = &lk_unlk;
 	ret = next(dev);
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
 
-static int opal_reverttper(struct opal_dev *dev, struct opal_key *opal)
+static int opal_reverttper(struct opal_dev *dev, struct opal_key __user *arg)
 {
 	void *data[2] = { NULL };
 	static const opal_step revert_funcs[] = {
@@ -2106,12 +2126,16 @@ static int opal_reverttper(struct opal_dev *dev, struct opal_key *opal)
 		revert_tper, /* controller will terminate session */
 		NULL,
 	};
+	struct opal_key opal_key;
 	int ret;
 
+	if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
+		return -EFAULT;
+
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev, revert_funcs);
 	dev->func_data = data;
-	dev->func_data[1] = opal;
+	dev->func_data[1] = &opal_key;
 	ret = next(dev);
 	mutex_unlock(&dev->dev_lock);
 	return ret;
@@ -2145,22 +2169,26 @@ static int __opal_lock_unlock(struct opal_dev *dev)
 	return next(dev);
 }
 
-static int opal_lock_unlock(struct opal_dev *dev, struct opal_lock_unlock *lk_unlk)
+static int opal_lock_unlock(struct opal_dev *dev, struct opal_lock_unlock __user *arg)
 {
+	struct opal_lock_unlock lk_unlk;
 	void *func_data[3] = { NULL };
 	int ret;
 
-	if (lk_unlk->session.who < OPAL_ADMIN1 ||
-	    lk_unlk->session.who > OPAL_USER9)
+	if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
+		return -EFAULT;
+
+	if (lk_unlk.session.who < OPAL_ADMIN1 ||
+	    lk_unlk.session.who > OPAL_USER9)
 		return -EINVAL;
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev, NULL);
 	dev->func_data = func_data;
-	dev->func_data[1] = &lk_unlk->session;
-	dev->func_data[2] = lk_unlk;
+	dev->func_data[1] = &lk_unlk.session;
+	dev->func_data[2] = &lk_unlk;
 
-	if (lk_unlk->session.sum)
+	if (lk_unlk.session.sum)
 		ret = __opal_lock_unlock_sum(dev);
 	else
 		ret = __opal_lock_unlock(dev);
@@ -2169,7 +2197,7 @@ static int opal_lock_unlock(struct opal_dev *dev, struct opal_lock_unlock *lk_un
 	return ret;
 }
 
-static int opal_take_ownership(struct opal_dev *dev, struct opal_key *opal)
+static int opal_take_ownership(struct opal_dev *dev, struct opal_key __user *arg)
 {
 	static const opal_step owner_funcs[] = {
 		opal_discovery0,
@@ -2182,22 +2210,26 @@ static int opal_take_ownership(struct opal_dev *dev, struct opal_key *opal)
 		NULL
 	};
 	void *data[6] = { NULL };
+	struct opal_key opal;
 	int ret;
 
+	if (copy_from_user(&opal, arg, sizeof(opal)))
+		return -EFAULT;
+
 	if (!dev)
 		return -ENODEV;
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev, owner_funcs);
 	dev->func_data = data;
-	dev->func_data[4] = opal;
-	dev->func_data[5] = opal;
+	dev->func_data[4] = &opal;
+	dev->func_data[5] = &opal;
 	ret = next(dev);
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
 
-static int opal_activate_lsp(struct opal_dev *dev, struct opal_lr_act *opal_lr_act)
+static int opal_activate_lsp(struct opal_dev *dev, struct opal_lr_act __user *arg)
 {
 	void *data[4] = { NULL };
 	static const opal_step active_funcs[] = {
@@ -2208,23 +2240,27 @@ static int opal_activate_lsp(struct opal_dev *dev, struct opal_lr_act *opal_lr_a
 		end_opal_session,
 		NULL
 	};
+	struct opal_lr_act opal_lr_act;
 	int ret;
 
-	if (!opal_lr_act->num_lrs || opal_lr_act->num_lrs > OPAL_MAX_LRS)
+	if (copy_from_user(&opal_lr_act, arg, sizeof(opal_lr_act)))
+		return -EFAULT;
+
+	if (!opal_lr_act.num_lrs || opal_lr_act.num_lrs > OPAL_MAX_LRS)
 		return -EINVAL;
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev, active_funcs);
 	dev->func_data = data;
-	dev->func_data[1] = &opal_lr_act->key;
-	dev->func_data[3] = opal_lr_act;
+	dev->func_data[1] = &opal_lr_act.key;
+	dev->func_data[3] = &opal_lr_act;
 	ret = next(dev);
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
 
 static int opal_setup_locking_range(struct opal_dev *dev,
-				    struct opal_user_lr_setup *opal_lrs)
+				    struct opal_user_lr_setup __user *arg)
 {
 	void *data[3] = { NULL };
 	static const opal_step lr_funcs[] = {
@@ -2234,19 +2270,23 @@ static int opal_setup_locking_range(struct opal_dev *dev,
 		end_opal_session,
 		NULL,
 	};
+	struct opal_user_lr_setup opal_lrs;
 	int ret;
 
+	if (copy_from_user(&opal_lrs, arg, sizeof(opal_lrs)))
+		return -EFAULT;
+
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev, lr_funcs);
 	dev->func_data = data;
-	dev->func_data[1] = &opal_lrs->session;
-	dev->func_data[2] = opal_lrs;
+	dev->func_data[1] = &opal_lrs.session;
+	dev->func_data[2] = &opal_lrs;
 	ret = next(dev);
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
 
-static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw *opal_pw)
+static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw __user *arg)
 {
 	static const opal_step pw_funcs[] = {
 		opal_discovery0,
@@ -2256,19 +2296,23 @@ static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw *opal_pw)
 		NULL
 	};
 	void *data[3] = { NULL };
+	struct opal_new_pw opal_pw;
 	int ret;
 
-	if (opal_pw->session.who < OPAL_ADMIN1 ||
-	    opal_pw->session.who > OPAL_USER9  ||
-	    opal_pw->new_user_pw.who < OPAL_ADMIN1 ||
-	    opal_pw->new_user_pw.who > OPAL_USER9)
+	if (copy_from_user(&opal_pw, arg, sizeof(opal_pw)))
+		return -EFAULT;
+
+	if (opal_pw.session.who < OPAL_ADMIN1 ||
+	    opal_pw.session.who > OPAL_USER9  ||
+	    opal_pw.new_user_pw.who < OPAL_ADMIN1 ||
+	    opal_pw.new_user_pw.who > OPAL_USER9)
 		return -EINVAL;
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev, pw_funcs);
 	dev->func_data = data;
-	dev->func_data[1] = (void *) &opal_pw->session;
-	dev->func_data[2] = (void *) &opal_pw->new_user_pw;
+	dev->func_data[1] = (void *) &opal_pw.session;
+	dev->func_data[2] = (void *) &opal_pw.new_user_pw;
 
 	ret = next(dev);
 	mutex_unlock(&dev->dev_lock);
@@ -2276,7 +2320,7 @@ static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw *opal_pw)
 }
 
 static int opal_activate_user(struct opal_dev *dev,
-			      struct opal_session_info *opal_session)
+			      struct opal_session_info __user *arg)
 {
 	static const opal_step act_funcs[] = {
 		opal_discovery0,
@@ -2286,20 +2330,24 @@ static int opal_activate_user(struct opal_dev *dev,
 		NULL
 	};
 	void *data[3] = { NULL };
+	struct opal_session_info opal_session;
 	int ret;
 
+	if (copy_from_user(&opal_session, arg, sizeof(opal_session)))
+		return -EFAULT;
+
 	/* We can't activate Admin1 it's active as manufactured */
-	if (opal_session->who < OPAL_USER1 &&
-	    opal_session->who > OPAL_USER9) {
-		pr_err("Who was not a valid user: %d\n", opal_session->who);
+	if (opal_session.who < OPAL_USER1 &&
+	    opal_session.who > OPAL_USER9) {
+		pr_err("Who was not a valid user: %d\n", opal_session.who);
 		return -EINVAL;
 	}
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev, act_funcs);
 	dev->func_data = data;
-	dev->func_data[1] = &opal_session->opal_key;
-	dev->func_data[2] = opal_session;
+	dev->func_data[1] = &opal_session.opal_key;
+	dev->func_data[2] = &opal_session;
 	ret = next(dev);
 	mutex_unlock(&dev->dev_lock);
 	return ret;
@@ -2356,90 +2404,42 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
 	}
 
 	switch (cmd) {
-	case IOC_OPAL_SAVE: {
-		struct opal_lock_unlock lk_unlk;
+	case IOC_OPAL_SAVE:
+		return opal_save(dev, arg);
 
-		if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
-			return -EFAULT;
-		return opal_save(dev, &lk_unlk);
-	}
-	case IOC_OPAL_LOCK_UNLOCK: {
-		struct opal_lock_unlock lk_unlk;
+	case IOC_OPAL_LOCK_UNLOCK:
+		return opal_lock_unlock(dev, arg);
 
-		if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
-			return -EFAULT;
-		return opal_lock_unlock(dev, &lk_unlk);
-	}
-	case IOC_OPAL_TAKE_OWNERSHIP: {
-		struct opal_key opal_key;
+	case IOC_OPAL_TAKE_OWNERSHIP: 
+		return opal_take_ownership(dev, arg);
 
-		if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
-			return -EFAULT;
-		return opal_take_ownership(dev, &opal_key);
-	}
-	case IOC_OPAL_ACTIVATE_LSP: {
-		struct opal_lr_act opal_lr_act;
+	case IOC_OPAL_ACTIVATE_LSP:
+		return opal_activate_lsp(dev, arg);
 
-		if (copy_from_user(&opal_lr_act, arg, sizeof(opal_lr_act)))
-			return -EFAULT;
-		return opal_activate_lsp(dev, &opal_lr_act);
-	}
-	case IOC_OPAL_SET_PW: {
-		struct opal_new_pw opal_pw;
+	case IOC_OPAL_SET_PW:
+		return opal_set_new_pw(dev, arg);
 
-		if (copy_from_user(&opal_pw, arg, sizeof(opal_pw)))
-			return -EFAULT;
-		return opal_set_new_pw(dev, &opal_pw);
-	}
-	case IOC_OPAL_ACTIVATE_USR: {
-		struct opal_session_info session;
+	case IOC_OPAL_ACTIVATE_USR:
+		return opal_activate_user(dev, arg);
 
-		if (copy_from_user(&session, arg, sizeof(session)))
-			return -EFAULT;
-		return opal_activate_user(dev, &session);
-	}
-	case IOC_OPAL_REVERT_TPR: {
-		struct opal_key opal_key;
+	case IOC_OPAL_REVERT_TPR:
+		return opal_reverttper(dev, arg);
 
-		if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
-			return -EFAULT;
-		return opal_reverttper(dev, &opal_key);
-	}
-	case IOC_OPAL_LR_SETUP: {
-		struct opal_user_lr_setup lrs;
+	case IOC_OPAL_LR_SETUP:
+		return opal_setup_locking_range(dev, arg);
 
-		if (copy_from_user(&lrs, arg, sizeof(lrs)))
-			return -EFAULT;
-		return opal_setup_locking_range(dev, &lrs);
-	}
-	case IOC_OPAL_ADD_USR_TO_LR: {
-		struct opal_lock_unlock lk_unlk;
+	case IOC_OPAL_ADD_USR_TO_LR:
+		return opal_add_user_to_lr(dev, arg);
 
-		if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
-			return -EFAULT;
-		return opal_add_user_to_lr(dev, &lk_unlk);
-	}
-	case IOC_OPAL_ENABLE_DISABLE_MBR: {
-		struct opal_mbr_data mbr;
+	case IOC_OPAL_ENABLE_DISABLE_MBR:
+		return opal_enable_disable_shadow_mbr(dev, arg);
 
-		if (copy_from_user(&mbr, arg, sizeof(mbr)))
-			return -EFAULT;
-		return opal_enable_disable_shadow_mbr(dev, &mbr);
-	}
-	case IOC_OPAL_ERASE_LR: {
-		struct opal_session_info session;
+	case IOC_OPAL_ERASE_LR:
+		return opal_erase_locking_range(dev, arg);
 
-		if (copy_from_user(&session, arg, sizeof(session)))
-			return -EFAULT;
-		return opal_erase_locking_range(dev, &session);
-	}
-	case IOC_OPAL_SECURE_ERASE_LR: {
-		struct opal_session_info session;
+	case IOC_OPAL_SECURE_ERASE_LR:
+		return opal_secure_erase_locking_range(dev, arg);
 
-		if (copy_from_user(&session, arg, sizeof(session)))
-			return -EFAULT;
-		return opal_secure_erase_locking_range(dev, &session);
-	}
 	default:
 		pr_warn("No such Opal Ioctl %u\n", cmd);
 	}
-- 
2.9.0

Re: [PATCH] block: sed-opal: reduce stack size of ioctl handler

[Scott Bauer]

On Wed, Feb 08, 2017 at 10:15:28PM +0100, Arnd Bergmann wrote:
> When CONFIG_KASAN is in use, the sed_ioctl function uses unusually large stack,
> as each possible ioctl argument gets its own stack area plus redzone:
> 
> block/sed-opal.c: In function 'sed_ioctl':
> block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> 
> Moving the copy_from_user() calls into the individual functions has little
> effect on readablility, but significantly reduces the stack size, with the
> largest individual function (opal_enable_disable_shadow_mbr) now at
> reasonable 456 bytes.
> 
> Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>


Hi Arnd,

Thank you for the report. We want to keep the function calls agnostic to userland.
In the future we will have in-kernel callers and I don't want to have to do any
get_fs(KERNEL_DS) wizardry.

Instead I think we can use a union to lessen the stack burden. I tested this patch just now
with config_ksasan and was able to build.


>From dfa6a2c842a6e45cab198c9058e753835a81521e Mon Sep 17 00:00:00 2001
From: Scott Bauer <scott.bauer@intel.com>
Date: Wed, 8 Feb 2017 14:49:32 -0700
Subject: [PATCH] Unionize stack parameters for sed_ioctl to prevent oversized
 stack

block/sed-opal.c: In function 'sed_ioctl':
block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]

Moved all the ioctl structures into a union to prevent oversized stack frame size.

Fixes: 455a7b238cd6 ("block: Add Sed-opal library")

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Scott Bauer <scott.bauer@intel.com>
---
 block/sed-opal.c | 118 ++++++++++++++++++++++---------------------------------
 1 file changed, 46 insertions(+), 72 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index bf1406e..f509168 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -2347,6 +2347,15 @@ EXPORT_SYMBOL(opal_unlock_from_suspend);
 int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
 {
 	void __user *arg = (void __user *)ptr;
+	union {
+		struct opal_lock_unlock lk_unlk;
+		struct opal_key opal_key;
+		struct opal_lr_act opal_lr_act;
+		struct opal_new_pw opal_pw;
+		struct opal_session_info session;
+		struct opal_user_lr_setup lrs;
+		struct opal_mbr_data mbr;
+	}u;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
@@ -2355,91 +2364,56 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
 		return -ENOTSUPP;
 	}
 
+	memset(&u, 0, sizeof(u));
 	switch (cmd) {
-	case IOC_OPAL_SAVE: {
-		struct opal_lock_unlock lk_unlk;
-
-		if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
+	case IOC_OPAL_SAVE:
+		if (copy_from_user(&u.lk_unlk, arg, sizeof(u.lk_unlk)))
 			return -EFAULT;
-		return opal_save(dev, &lk_unlk);
-	}
-	case IOC_OPAL_LOCK_UNLOCK: {
-		struct opal_lock_unlock lk_unlk;
-
-		if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
+		return opal_save(dev, &u.lk_unlk);
+	case IOC_OPAL_LOCK_UNLOCK:
+		if (copy_from_user(&u.lk_unlk, arg, sizeof(u.lk_unlk)))
 			return -EFAULT;
-		return opal_lock_unlock(dev, &lk_unlk);
-	}
-	case IOC_OPAL_TAKE_OWNERSHIP: {
-		struct opal_key opal_key;
-
-		if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
+		return opal_lock_unlock(dev, &u.lk_unlk);
+	case IOC_OPAL_TAKE_OWNERSHIP:
+		if (copy_from_user(&u.opal_key, arg, sizeof(u.opal_key)))
 			return -EFAULT;
-		return opal_take_ownership(dev, &opal_key);
-	}
-	case IOC_OPAL_ACTIVATE_LSP: {
-		struct opal_lr_act opal_lr_act;
-
-		if (copy_from_user(&opal_lr_act, arg, sizeof(opal_lr_act)))
+		return opal_take_ownership(dev, &u.opal_key);
+	case IOC_OPAL_ACTIVATE_LSP:
+		if (copy_from_user(&u.opal_lr_act, arg, sizeof(u.opal_lr_act)))
 			return -EFAULT;
-		return opal_activate_lsp(dev, &opal_lr_act);
-	}
-	case IOC_OPAL_SET_PW: {
-		struct opal_new_pw opal_pw;
-
-		if (copy_from_user(&opal_pw, arg, sizeof(opal_pw)))
+		return opal_activate_lsp(dev, &u.opal_lr_act);
+	case IOC_OPAL_SET_PW:
+		if (copy_from_user(&u.opal_pw, arg, sizeof(u.opal_pw)))
 			return -EFAULT;
-		return opal_set_new_pw(dev, &opal_pw);
-	}
-	case IOC_OPAL_ACTIVATE_USR: {
-		struct opal_session_info session;
-
-		if (copy_from_user(&session, arg, sizeof(session)))
+		return opal_set_new_pw(dev, &u.opal_pw);
+	case IOC_OPAL_ACTIVATE_USR:
+		if (copy_from_user(&u.session, arg, sizeof(u.session)))
 			return -EFAULT;
-		return opal_activate_user(dev, &session);
-	}
-	case IOC_OPAL_REVERT_TPR: {
-		struct opal_key opal_key;
-
-		if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
+		return opal_activate_user(dev, &u.session);
+	case IOC_OPAL_REVERT_TPR:
+		if (copy_from_user(&u.opal_key, arg, sizeof(u.opal_key)))
 			return -EFAULT;
-		return opal_reverttper(dev, &opal_key);
-	}
-	case IOC_OPAL_LR_SETUP: {
-		struct opal_user_lr_setup lrs;
-
-		if (copy_from_user(&lrs, arg, sizeof(lrs)))
+		return opal_reverttper(dev, &u.opal_key);
+	case IOC_OPAL_LR_SETUP:
+		if (copy_from_user(&u.lrs, arg, sizeof(u.lrs)))
 			return -EFAULT;
-		return opal_setup_locking_range(dev, &lrs);
-	}
-	case IOC_OPAL_ADD_USR_TO_LR: {
-		struct opal_lock_unlock lk_unlk;
-
-		if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
+		return opal_setup_locking_range(dev, &u.lrs);
+	case IOC_OPAL_ADD_USR_TO_LR:
+		if (copy_from_user(&u.lk_unlk, arg, sizeof(u.lk_unlk)))
 			return -EFAULT;
-		return opal_add_user_to_lr(dev, &lk_unlk);
-	}
-	case IOC_OPAL_ENABLE_DISABLE_MBR: {
-		struct opal_mbr_data mbr;
-
-		if (copy_from_user(&mbr, arg, sizeof(mbr)))
+		return opal_add_user_to_lr(dev, &u.lk_unlk);
+	case IOC_OPAL_ENABLE_DISABLE_MBR:
+		if (copy_from_user(&u.mbr, arg, sizeof(u.mbr)))
 			return -EFAULT;
-		return opal_enable_disable_shadow_mbr(dev, &mbr);
-	}
-	case IOC_OPAL_ERASE_LR: {
-		struct opal_session_info session;
-
-		if (copy_from_user(&session, arg, sizeof(session)))
+		return opal_enable_disable_shadow_mbr(dev, &u.mbr);
+	case IOC_OPAL_ERASE_LR:
+		if (copy_from_user(&u.session, arg, sizeof(u.session)))
 			return -EFAULT;
-		return opal_erase_locking_range(dev, &session);
-	}
-	case IOC_OPAL_SECURE_ERASE_LR: {
-		struct opal_session_info session;
-
-		if (copy_from_user(&session, arg, sizeof(session)))
+		return opal_erase_locking_range(dev, &u.session);
+	case IOC_OPAL_SECURE_ERASE_LR:
+		if (copy_from_user(&u.session, arg, sizeof(u.session)))
 			return -EFAULT;
-		return opal_secure_erase_locking_range(dev, &session);
-	}
+		return opal_secure_erase_locking_range(dev, &u.session);
 	default:
 		pr_warn("No such Opal Ioctl %u\n", cmd);
 	}
-- 
2.7.4

Re: [PATCH] block: sed-opal: reduce stack size of ioctl handler

[Scott Bauer]

On Wed, Feb 08, 2017 at 02:58:28PM -0700, Scott Bauer wrote:
> On Wed, Feb 08, 2017 at 10:15:28PM +0100, Arnd Bergmann wrote:
> > When CONFIG_KASAN is in use, the sed_ioctl function uses unusually large stack,
> > as each possible ioctl argument gets its own stack area plus redzone:
> > 
> > block/sed-opal.c: In function 'sed_ioctl':
> > block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> > 
> > Moving the copy_from_user() calls into the individual functions has little
> > effect on readablility, but significantly reduces the stack size, with the
> > largest individual function (opal_enable_disable_shadow_mbr) now at
> > reasonable 456 bytes.
> > 
> > Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> 
> Hi Arnd,
> 
> Thank you for the report. We want to keep the function calls agnostic to userland.
> In the future we will have in-kernel callers and I don't want to have to do any
> get_fs(KERNEL_DS) wizardry.
> 
> Instead I think we can use a union to lessen the stack burden. I tested this patch just now
> with config_ksasan and was able to build.

Nack on this patch, it only really masks the issue. Keith pointed out we have a call chain
up to this ioctl then deeper down into nvme then the block layer. If we use 25% of the stack
just for this function it's still too dangerous and we'll run into corruption later on and not
remember this fix. I'll come up with another solution.

Re: [PATCH] block: sed-opal: reduce stack size of ioctl handler

[Arnd Bergmann]

On Wed, Feb 8, 2017 at 11:12 PM, Scott Bauer <scott.bauer@intel.com> wrote:
> On Wed, Feb 08, 2017 at 02:58:28PM -0700, Scott Bauer wrote:
>> Thank you for the report. We want to keep the function calls agnostic to userland.
>> In the future we will have in-kernel callers and I don't want to have to do any
>> get_fs(KERNEL_DS) wizardry.
>>
>> Instead I think we can use a union to lessen the stack burden. I tested this patch just now
>> with config_ksasan and was able to build.
>
> Nack on this patch, it only really masks the issue. Keith pointed out we have a call chain
> up to this ioctl then deeper down into nvme then the block layer. If we use 25% of the stack
> just for this function it's still too dangerous and we'll run into corruption later on and not
> remember this fix. I'll come up with another solution.

I think there are two issues that cause the stack frame to explode
with KASAN, and
your patch addresses one but not the other:

1. checks for variables being accessed after they go out of scope.
This is solved by the
   union at the start of the function, as they never go out of scope now.

2. checks for array overflows when passing a local variable by
reference to another
   function that is not inlined.

To solve the second problem while keeping the in-kernel interface, the
approach that
David suggesteed would work, or you could have a wrapper around each function to
do the copy_from_user in a more obvious but verbose way as I had in my version.

With David's approach, you could actually replace the switch() with a
lookup table
as well.

    Arnd

RE: [PATCH] block: sed-opal: reduce stack size of ioctl handler

[David Laight]

From: Arnd Bergmann
> Sent: 08 February 2017 21:15
>
> When CONFIG_KASAN is in use, the sed_ioctl function uses unusually large =
stack,
> as each possible ioctl argument gets its own stack area plus redzone:

Why not do a single copy_from_user() at the top of sed_ioctl() based on
the _IOC_DIR() and __IOC_SIZE() values?

Something like:
int sed_ioctl(..., unsigned int cmd, void __user *arg)
{
	u64 buf[??]; /* or a union */
	unsigned int cmd_sz =3D _IOC_SIZE(cmd);

	if (_IOC_DIR(cmd) & (_IOC_WRITE | _IOC_READ) && cmd_sz > sizeof buf)
		return -EINVAL;

	if (_IOC_DIR(cmd) & _IOC_WRITE) {
		if (copy_from_user(buf, arg, cmd_sz))
			return -EFAULT;
	} else {
		if (IOC_DIR(cmd) & _IOC_READ))
			memzero(buf, cmd_sz);
	}

	switch (cmd) {
	...
		rval =3D ...
	...
	}

	if (rval >=3D 0 && (_IOC_DIR(cmd) & _IOC_READ)
			&& copy_to_user(arg, buf, cmd_sz));
		return -EFAULT;

	return rval;
}

	David