* [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
@ 2017-02-09 17:20 ` Scott Bauer
0 siblings, 0 replies; 26+ messages in thread
From: Scott Bauer @ 2017-02-09 17:20 UTC (permalink / raw)
When CONFIG_KASAN is enabled, compilation fails:
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 off the stack and dynamically activate
using _IOC_SIZE()
Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
Reported-by: Arnd Bergmann <arnd at arndb.de>
Signed-off-by: Scott Bauer <scott.bauer at intel.com>
---
block/sed-opal.c | 134 +++++++++++++++++++++----------------------------------
1 file changed, 50 insertions(+), 84 deletions(-)
diff --git a/block/sed-opal.c b/block/sed-opal.c
index bf1406e..4985d95 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -2346,7 +2346,10 @@ EXPORT_SYMBOL(opal_unlock_from_suspend);
int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
{
+ void *ioctl_ptr;
+ int ret = -ENOTTY;
void __user *arg = (void __user *)ptr;
+ unsigned int cmd_size = _IOC_SIZE(cmd);
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
@@ -2355,94 +2358,57 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
return -ENOTSUPP;
}
- switch (cmd) {
- case IOC_OPAL_SAVE: {
- struct opal_lock_unlock lk_unlk;
-
- 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;
-
- 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;
-
- 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;
-
- 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;
-
- 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;
-
- 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;
-
- 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;
-
- 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;
-
- 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;
-
- 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;
-
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_erase_locking_range(dev, &session);
+ ioctl_ptr = kzalloc(cmd_size, GFP_KERNEL);
+ if (!ioctl_ptr)
+ return -ENOMEM;
+ if (copy_from_user(ioctl_ptr, arg, cmd_size)) {
+ ret = -EFAULT;
+ goto out;
}
- case IOC_OPAL_SECURE_ERASE_LR: {
- struct opal_session_info session;
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_secure_erase_locking_range(dev, &session);
- }
+ switch (cmd) {
+ case IOC_OPAL_SAVE:
+ ret = opal_save(dev, ioctl_ptr);
+ break;
+ case IOC_OPAL_LOCK_UNLOCK:
+ ret = opal_lock_unlock(dev, ioctl_ptr);
+ break;
+ case IOC_OPAL_TAKE_OWNERSHIP:
+ ret = opal_take_ownership(dev, ioctl_ptr);
+ break;
+ case IOC_OPAL_ACTIVATE_LSP:
+ ret = opal_activate_lsp(dev, ioctl_ptr);
+ break;
+ case IOC_OPAL_SET_PW:
+ ret = opal_set_new_pw(dev, ioctl_ptr);
+ break;
+ case IOC_OPAL_ACTIVATE_USR:
+ ret = opal_activate_user(dev, ioctl_ptr);
+ break;
+ case IOC_OPAL_REVERT_TPR:
+ ret = opal_reverttper(dev, ioctl_ptr);
+ break;
+ case IOC_OPAL_LR_SETUP:
+ ret = opal_setup_locking_range(dev, ioctl_ptr);
+ break;
+ case IOC_OPAL_ADD_USR_TO_LR:
+ ret = opal_add_user_to_lr(dev, ioctl_ptr);
+ break;
+ case IOC_OPAL_ENABLE_DISABLE_MBR:
+ ret = opal_enable_disable_shadow_mbr(dev, ioctl_ptr);
+ break;
+ case IOC_OPAL_ERASE_LR:
+ ret = opal_erase_locking_range(dev, ioctl_ptr);
+ break;
+ case IOC_OPAL_SECURE_ERASE_LR:
+ ret = opal_secure_erase_locking_range(dev, ioctl_ptr);
+ break;
default:
pr_warn("No such Opal Ioctl %u\n", cmd);
}
- return -ENOTTY;
+
+out:
+ kfree(ioctl_ptr);
+ return ret;
}
EXPORT_SYMBOL_GPL(sed_ioctl);
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
2017-02-09 17:20 ` Scott Bauer
@ 2017-02-09 19:51 ` Rafael Antognolli
-1 siblings, 0 replies; 26+ messages in thread
From: Rafael Antognolli @ 2017-02-09 19:51 UTC (permalink / raw)
To: Scott Bauer
Cc: hch, arnd, axboe, linux-kernel, linux-nvme, keith.busch,
David.Laight, linux-block, jonathan.derrick
On Thu, Feb 09, 2017 at 10:20:01AM -0700, Scott Bauer wrote:
> When CONFIG_KASAN is enabled, compilation fails:
>
> 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 off the stack and dynamically activate
> using _IOC_SIZE()
I just went through the other threads about this issue. This approach
looks good to me.
Rafael
> 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 | 134 +++++++++++++++++++++----------------------------------
> 1 file changed, 50 insertions(+), 84 deletions(-)
>
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index bf1406e..4985d95 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -2346,7 +2346,10 @@ EXPORT_SYMBOL(opal_unlock_from_suspend);
>
> int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
> {
> + void *ioctl_ptr;
> + int ret = -ENOTTY;
> void __user *arg = (void __user *)ptr;
> + unsigned int cmd_size = _IOC_SIZE(cmd);
>
> if (!capable(CAP_SYS_ADMIN))
> return -EACCES;
> @@ -2355,94 +2358,57 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
> return -ENOTSUPP;
> }
>
> - switch (cmd) {
> - case IOC_OPAL_SAVE: {
> - struct opal_lock_unlock lk_unlk;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - if (copy_from_user(&session, arg, sizeof(session)))
> - return -EFAULT;
> - return opal_erase_locking_range(dev, &session);
> + ioctl_ptr = kzalloc(cmd_size, GFP_KERNEL);
> + if (!ioctl_ptr)
> + return -ENOMEM;
> + if (copy_from_user(ioctl_ptr, arg, cmd_size)) {
> + ret = -EFAULT;
> + goto out;
> }
> - case IOC_OPAL_SECURE_ERASE_LR: {
> - struct opal_session_info session;
>
> - if (copy_from_user(&session, arg, sizeof(session)))
> - return -EFAULT;
> - return opal_secure_erase_locking_range(dev, &session);
> - }
> + switch (cmd) {
> + case IOC_OPAL_SAVE:
> + ret = opal_save(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_LOCK_UNLOCK:
> + ret = opal_lock_unlock(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_TAKE_OWNERSHIP:
> + ret = opal_take_ownership(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_ACTIVATE_LSP:
> + ret = opal_activate_lsp(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_SET_PW:
> + ret = opal_set_new_pw(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_ACTIVATE_USR:
> + ret = opal_activate_user(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_REVERT_TPR:
> + ret = opal_reverttper(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_LR_SETUP:
> + ret = opal_setup_locking_range(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_ADD_USR_TO_LR:
> + ret = opal_add_user_to_lr(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_ENABLE_DISABLE_MBR:
> + ret = opal_enable_disable_shadow_mbr(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_ERASE_LR:
> + ret = opal_erase_locking_range(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_SECURE_ERASE_LR:
> + ret = opal_secure_erase_locking_range(dev, ioctl_ptr);
> + break;
> default:
> pr_warn("No such Opal Ioctl %u\n", cmd);
> }
> - return -ENOTTY;
> +
> +out:
> + kfree(ioctl_ptr);
> + return ret;
> }
> EXPORT_SYMBOL_GPL(sed_ioctl);
> --
> 2.7.4
>
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
@ 2017-02-09 19:51 ` Rafael Antognolli
0 siblings, 0 replies; 26+ messages in thread
From: Rafael Antognolli @ 2017-02-09 19:51 UTC (permalink / raw)
On Thu, Feb 09, 2017@10:20:01AM -0700, Scott Bauer wrote:
> When CONFIG_KASAN is enabled, compilation fails:
>
> 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 off the stack and dynamically activate
> using _IOC_SIZE()
I just went through the other threads about this issue. This approach
looks good to me.
Rafael
> Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
>
> Reported-by: Arnd Bergmann <arnd at arndb.de>
> Signed-off-by: Scott Bauer <scott.bauer at intel.com>
> ---
> block/sed-opal.c | 134 +++++++++++++++++++++----------------------------------
> 1 file changed, 50 insertions(+), 84 deletions(-)
>
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index bf1406e..4985d95 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -2346,7 +2346,10 @@ EXPORT_SYMBOL(opal_unlock_from_suspend);
>
> int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
> {
> + void *ioctl_ptr;
> + int ret = -ENOTTY;
> void __user *arg = (void __user *)ptr;
> + unsigned int cmd_size = _IOC_SIZE(cmd);
>
> if (!capable(CAP_SYS_ADMIN))
> return -EACCES;
> @@ -2355,94 +2358,57 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
> return -ENOTSUPP;
> }
>
> - switch (cmd) {
> - case IOC_OPAL_SAVE: {
> - struct opal_lock_unlock lk_unlk;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - 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;
> -
> - if (copy_from_user(&session, arg, sizeof(session)))
> - return -EFAULT;
> - return opal_erase_locking_range(dev, &session);
> + ioctl_ptr = kzalloc(cmd_size, GFP_KERNEL);
> + if (!ioctl_ptr)
> + return -ENOMEM;
> + if (copy_from_user(ioctl_ptr, arg, cmd_size)) {
> + ret = -EFAULT;
> + goto out;
> }
> - case IOC_OPAL_SECURE_ERASE_LR: {
> - struct opal_session_info session;
>
> - if (copy_from_user(&session, arg, sizeof(session)))
> - return -EFAULT;
> - return opal_secure_erase_locking_range(dev, &session);
> - }
> + switch (cmd) {
> + case IOC_OPAL_SAVE:
> + ret = opal_save(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_LOCK_UNLOCK:
> + ret = opal_lock_unlock(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_TAKE_OWNERSHIP:
> + ret = opal_take_ownership(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_ACTIVATE_LSP:
> + ret = opal_activate_lsp(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_SET_PW:
> + ret = opal_set_new_pw(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_ACTIVATE_USR:
> + ret = opal_activate_user(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_REVERT_TPR:
> + ret = opal_reverttper(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_LR_SETUP:
> + ret = opal_setup_locking_range(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_ADD_USR_TO_LR:
> + ret = opal_add_user_to_lr(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_ENABLE_DISABLE_MBR:
> + ret = opal_enable_disable_shadow_mbr(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_ERASE_LR:
> + ret = opal_erase_locking_range(dev, ioctl_ptr);
> + break;
> + case IOC_OPAL_SECURE_ERASE_LR:
> + ret = opal_secure_erase_locking_range(dev, ioctl_ptr);
> + break;
> default:
> pr_warn("No such Opal Ioctl %u\n", cmd);
> }
> - return -ENOTTY;
> +
> +out:
> + kfree(ioctl_ptr);
> + return ret;
> }
> EXPORT_SYMBOL_GPL(sed_ioctl);
> --
> 2.7.4
>
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
2017-02-09 17:20 ` Scott Bauer
@ 2017-02-10 7:45 ` Johannes Thumshirn
-1 siblings, 0 replies; 26+ messages in thread
From: Johannes Thumshirn @ 2017-02-10 7:45 UTC (permalink / raw)
To: Scott Bauer, linux-nvme
Cc: keith.busch, arnd, hch, linux-kernel, axboe, David.Laight,
linux-block, jonathan.derrick
On 02/09/2017 06:20 PM, Scott Bauer wrote:
> When CONFIG_KASAN is enabled, compilation fails:
> =
> block/sed-opal.c: In function 'sed_ioctl':
> block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger th=
an 2048 bytes [-Werror=3Dframe-larger-than=3D]
> =
> Moved all the ioctl structures off the stack and dynamically activate
> using _IOC_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 | 134 +++++++++++++++++++++----------------------------=
------
> 1 file changed, 50 insertions(+), 84 deletions(-)
> =
[...]
> - if (copy_from_user(&session, arg, sizeof(session)))
> - return -EFAULT;
> - return opal_erase_locking_range(dev, &session);
> + ioctl_ptr =3D kzalloc(cmd_size, GFP_KERNEL);
> + if (!ioctl_ptr)
> + return -ENOMEM;
> + if (copy_from_user(ioctl_ptr, arg, cmd_size)) {
> + ret =3D -EFAULT;
> + goto out;
> }
Can't we use memdup_user() instead of kzalloc() + copy_from_user()?
-- =
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N=FCrnberg
GF: Felix Imend=F6rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N=FCrnberg)
Key fingerprint =3D EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
@ 2017-02-10 7:45 ` Johannes Thumshirn
0 siblings, 0 replies; 26+ messages in thread
From: Johannes Thumshirn @ 2017-02-10 7:45 UTC (permalink / raw)
On 02/09/2017 06:20 PM, Scott Bauer wrote:
> When CONFIG_KASAN is enabled, compilation fails:
>
> 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 off the stack and dynamically activate
> using _IOC_SIZE()
>
> Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
>
> Reported-by: Arnd Bergmann <arnd at arndb.de>
> Signed-off-by: Scott Bauer <scott.bauer at intel.com>
> ---
> block/sed-opal.c | 134 +++++++++++++++++++++----------------------------------
> 1 file changed, 50 insertions(+), 84 deletions(-)
>
[...]
> - if (copy_from_user(&session, arg, sizeof(session)))
> - return -EFAULT;
> - return opal_erase_locking_range(dev, &session);
> + ioctl_ptr = kzalloc(cmd_size, GFP_KERNEL);
> + if (!ioctl_ptr)
> + return -ENOMEM;
> + if (copy_from_user(ioctl_ptr, arg, cmd_size)) {
> + ret = -EFAULT;
> + goto out;
> }
Can't we use memdup_user() instead of kzalloc() + copy_from_user()?
--
Johannes Thumshirn Storage
jthumshirn at suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply [flat|nested] 26+ messages in thread* RE: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
2017-02-10 7:45 ` Johannes Thumshirn
@ 2017-02-10 10:28 ` David Laight
-1 siblings, 0 replies; 26+ messages in thread
From: David Laight @ 2017-02-10 10:28 UTC (permalink / raw)
To: 'Johannes Thumshirn', Scott Bauer,
linux-nvme@lists.infradead.org
Cc: keith.busch@intel.com, arnd@arndb.de, hch@infradead.org,
linux-kernel@vger.kernel.org, axboe@fb.com,
linux-block@vger.kernel.org, jonathan.derrick@intel.com
From: Johannes Thumshirn
> Sent: 10 February 2017 07:46
> On 02/09/2017 06:20 PM, Scott Bauer wrote:
> > When CONFIG_KASAN is enabled, compilation fails:
Does CONFIG_KASAN allocate guard stack space around everything that
is passed by address?
That sounds completely brain-dead.
There are a lot of functions that have an 'int *' argument to return
a single value - and are never going to do anything else.
...
> > Moved all the ioctl structures off the stack and dynamically activate
> > using _IOC_SIZE()
...
>
> > - if (copy_from_user(&session, arg, sizeof(session)))
> > - return -EFAULT;
> > - return opal_erase_locking_range(dev, &session);
> > + ioctl_ptr = kzalloc(cmd_size, GFP_KERNEL);
> > + if (!ioctl_ptr)
> > + return -ENOMEM;
> > + if (copy_from_user(ioctl_ptr, arg, cmd_size)) {
> > + ret = -EFAULT;
> > + goto out;
> > }
>
> Can't we use memdup_user() instead of kzalloc() + copy_from_user()?
You either want the copy_from_user() or the memzero() not both.
ISTM there could be two 'library' functions, maybe:
void *get_ioctl_buf(unsigned int cmd, long arg)
to malloc the buffer, memzero/copy_from_user, returns -EFAULT if copy fails.
int put_ioctl_buf(int rval, unsigned int cmd, const void *buf)
does copy_to_user() if rval >= 0 and IOR_READ, then frees buf.
return value is rval unless the copyout fails.
David
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
@ 2017-02-10 10:28 ` David Laight
0 siblings, 0 replies; 26+ messages in thread
From: David Laight @ 2017-02-10 10:28 UTC (permalink / raw)
From: Johannes Thumshirn
> Sent: 10 February 2017 07:46
> On 02/09/2017 06:20 PM, Scott Bauer wrote:
> > When CONFIG_KASAN is enabled, compilation fails:
Does CONFIG_KASAN allocate guard stack space around everything that
is passed by address?
That sounds completely brain-dead.
There are a lot of functions that have an 'int *' argument to return
a single value - and are never going to do anything else.
...
> > Moved all the ioctl structures off the stack and dynamically activate
> > using _IOC_SIZE()
...
>
> > - if (copy_from_user(&session, arg, sizeof(session)))
> > - return -EFAULT;
> > - return opal_erase_locking_range(dev, &session);
> > + ioctl_ptr = kzalloc(cmd_size, GFP_KERNEL);
> > + if (!ioctl_ptr)
> > + return -ENOMEM;
> > + if (copy_from_user(ioctl_ptr, arg, cmd_size)) {
> > + ret = -EFAULT;
> > + goto out;
> > }
>
> Can't we use memdup_user() instead of kzalloc() + copy_from_user()?
You either want the copy_from_user() or the memzero() not both.
ISTM there could be two 'library' functions, maybe:
void *get_ioctl_buf(unsigned int cmd, long arg)
to malloc the buffer, memzero/copy_from_user, returns -EFAULT if copy fails.
int put_ioctl_buf(int rval, unsigned int cmd, const void *buf)
does copy_to_user() if rval >= 0 and IOR_READ, then frees buf.
return value is rval unless the copyout fails.
David
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
2017-02-10 10:28 ` David Laight
@ 2017-02-10 11:00 ` Arnd Bergmann
-1 siblings, 0 replies; 26+ messages in thread
From: Arnd Bergmann @ 2017-02-10 11:00 UTC (permalink / raw)
To: David Laight
Cc: Johannes Thumshirn, Scott Bauer, linux-nvme@lists.infradead.org,
axboe@fb.com, keith.busch@intel.com, jonathan.derrick@intel.com,
hch@infradead.org, linux-kernel@vger.kernel.org,
linux-block@vger.kernel.org
On Fri, Feb 10, 2017 at 11:28 AM, David Laight <David.Laight@aculab.com> wrote:
>>
>> > - if (copy_from_user(&session, arg, sizeof(session)))
>> > - return -EFAULT;
>> > - return opal_erase_locking_range(dev, &session);
>> > + ioctl_ptr = kzalloc(cmd_size, GFP_KERNEL);
>> > + if (!ioctl_ptr)
>> > + return -ENOMEM;
>> > + if (copy_from_user(ioctl_ptr, arg, cmd_size)) {
>> > + ret = -EFAULT;
>> > + goto out;
>> > }
>>
>> Can't we use memdup_user() instead of kzalloc() + copy_from_user()?
>
> You either want the copy_from_user() or the memzero() not both.
>
> ISTM there could be two 'library' functions, maybe:
> void *get_ioctl_buf(unsigned int cmd, long arg)
> to malloc the buffer, memzero/copy_from_user, returns -EFAULT if copy fails.
> int put_ioctl_buf(int rval, unsigned int cmd, const void *buf)
> does copy_to_user() if rval >= 0 and IOR_READ, then frees buf.
> return value is rval unless the copyout fails.
All the ioctls commands in this driver are IOW, and no data is passed back
to user space, so there is no need for the memzero(): we can either copy
the data from user space or we fail.
Arnd
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
@ 2017-02-10 11:00 ` Arnd Bergmann
0 siblings, 0 replies; 26+ messages in thread
From: Arnd Bergmann @ 2017-02-10 11:00 UTC (permalink / raw)
On Fri, Feb 10, 2017@11:28 AM, David Laight <David.Laight@aculab.com> wrote:
>>
>> > - if (copy_from_user(&session, arg, sizeof(session)))
>> > - return -EFAULT;
>> > - return opal_erase_locking_range(dev, &session);
>> > + ioctl_ptr = kzalloc(cmd_size, GFP_KERNEL);
>> > + if (!ioctl_ptr)
>> > + return -ENOMEM;
>> > + if (copy_from_user(ioctl_ptr, arg, cmd_size)) {
>> > + ret = -EFAULT;
>> > + goto out;
>> > }
>>
>> Can't we use memdup_user() instead of kzalloc() + copy_from_user()?
>
> You either want the copy_from_user() or the memzero() not both.
>
> ISTM there could be two 'library' functions, maybe:
> void *get_ioctl_buf(unsigned int cmd, long arg)
> to malloc the buffer, memzero/copy_from_user, returns -EFAULT if copy fails.
> int put_ioctl_buf(int rval, unsigned int cmd, const void *buf)
> does copy_to_user() if rval >= 0 and IOR_READ, then frees buf.
> return value is rval unless the copyout fails.
All the ioctls commands in this driver are IOW, and no data is passed back
to user space, so there is no need for the memzero(): we can either copy
the data from user space or we fail.
Arnd
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
2017-02-09 17:20 ` Scott Bauer
@ 2017-02-10 8:01 ` Arnd Bergmann
-1 siblings, 0 replies; 26+ messages in thread
From: Arnd Bergmann @ 2017-02-10 8:01 UTC (permalink / raw)
To: Scott Bauer
Cc: keith.busch, hch, linux-kernel, linux-nvme, axboe, David.Laight,
linux-block, jonathan.derrick
On Thursday, February 9, 2017 10:20:01 AM CET Scott Bauer wrote:
> When CONFIG_KASAN is enabled, compilation fails:
>
> 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 off the stack and dynamically activate
> using _IOC_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 | 134 +++++++++++++++++++++----------------------------------
> 1 file changed, 50 insertions(+), 84 deletions(-)
>
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index bf1406e..4985d95 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -2346,7 +2346,10 @@ EXPORT_SYMBOL(opal_unlock_from_suspend);
>
> int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
> {
> + void *ioctl_ptr;
> + int ret = -ENOTTY;
> void __user *arg = (void __user *)ptr;
> + unsigned int cmd_size = _IOC_SIZE(cmd);
>
> if (!capable(CAP_SYS_ADMIN))
> return -EACCES;
We usually have a size check in there to avoid allocating large amounts
of memory. _IOC_SIZEBITS is 14, so you can have up to 16kb here, which
is probably ok, but I'd recommend either adding a comment to say that
it is, or just checking against the largest realistic size.
Having something like v4l with their tables if ioctl commands and
function pointers would also solve it, as you'd be checking for valid
command numbers before doing the copy then.
Otherwise looks good to me.
Arnd
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
@ 2017-02-10 8:01 ` Arnd Bergmann
0 siblings, 0 replies; 26+ messages in thread
From: Arnd Bergmann @ 2017-02-10 8:01 UTC (permalink / raw)
On Thursday, February 9, 2017 10:20:01 AM CET Scott Bauer wrote:
> When CONFIG_KASAN is enabled, compilation fails:
>
> 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 off the stack and dynamically activate
> using _IOC_SIZE()
>
> Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
>
> Reported-by: Arnd Bergmann <arnd at arndb.de>
> Signed-off-by: Scott Bauer <scott.bauer at intel.com>
> ---
> block/sed-opal.c | 134 +++++++++++++++++++++----------------------------------
> 1 file changed, 50 insertions(+), 84 deletions(-)
>
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index bf1406e..4985d95 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -2346,7 +2346,10 @@ EXPORT_SYMBOL(opal_unlock_from_suspend);
>
> int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
> {
> + void *ioctl_ptr;
> + int ret = -ENOTTY;
> void __user *arg = (void __user *)ptr;
> + unsigned int cmd_size = _IOC_SIZE(cmd);
>
> if (!capable(CAP_SYS_ADMIN))
> return -EACCES;
We usually have a size check in there to avoid allocating large amounts
of memory. _IOC_SIZEBITS is 14, so you can have up to 16kb here, which
is probably ok, but I'd recommend either adding a comment to say that
it is, or just checking against the largest realistic size.
Having something like v4l with their tables if ioctl commands and
function pointers would also solve it, as you'd be checking for valid
command numbers before doing the copy then.
Otherwise looks good to me.
Arnd
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
2017-02-10 8:01 ` Arnd Bergmann
@ 2017-02-10 15:57 ` Scott Bauer
-1 siblings, 0 replies; 26+ messages in thread
From: Scott Bauer @ 2017-02-10 15:57 UTC (permalink / raw)
To: Arnd Bergmann
Cc: keith.busch, hch, linux-kernel, linux-nvme, axboe, David.Laight,
linux-block, jonathan.derrick
On Fri, Feb 10, 2017 at 09:01:23AM +0100, Arnd Bergmann wrote:
> On Thursday, February 9, 2017 10:20:01 AM CET Scott Bauer wrote:
> > When CONFIG_KASAN is enabled, compilation fails:
> >
> > 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 off the stack and dynamically activate
> > using _IOC_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 | 134 +++++++++++++++++++++----------------------------------
> > 1 file changed, 50 insertions(+), 84 deletions(-)
> >
> > diff --git a/block/sed-opal.c b/block/sed-opal.c
> > index bf1406e..4985d95 100644
> > --- a/block/sed-opal.c
> > +++ b/block/sed-opal.c
> > @@ -2346,7 +2346,10 @@ EXPORT_SYMBOL(opal_unlock_from_suspend);
> >
> > int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
> > {
> > + void *ioctl_ptr;
> > + int ret = -ENOTTY;
> > void __user *arg = (void __user *)ptr;
> > + unsigned int cmd_size = _IOC_SIZE(cmd);
> >
> > if (!capable(CAP_SYS_ADMIN))
> > return -EACCES;
>
> We usually have a size check in there to avoid allocating large amounts
> of memory. _IOC_SIZEBITS is 14, so you can have up to 16kb here, which
> is probably ok, but I'd recommend either adding a comment to say that
> it is, or just checking against the largest realistic size.
Right now it's up to the storage driver to call into the sed-opal ioctl.
We have a function is_sed_ioctl() which is called before jumping into sed_ioctl.
So we will be weeding out any non opal ioctls before getting in there so I don't
see any overly large 16kb allocations happening.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
@ 2017-02-10 15:57 ` Scott Bauer
0 siblings, 0 replies; 26+ messages in thread
From: Scott Bauer @ 2017-02-10 15:57 UTC (permalink / raw)
On Fri, Feb 10, 2017@09:01:23AM +0100, Arnd Bergmann wrote:
> On Thursday, February 9, 2017 10:20:01 AM CET Scott Bauer wrote:
> > When CONFIG_KASAN is enabled, compilation fails:
> >
> > 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 off the stack and dynamically activate
> > using _IOC_SIZE()
> >
> > Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
> >
> > Reported-by: Arnd Bergmann <arnd at arndb.de>
> > Signed-off-by: Scott Bauer <scott.bauer at intel.com>
> > ---
> > block/sed-opal.c | 134 +++++++++++++++++++++----------------------------------
> > 1 file changed, 50 insertions(+), 84 deletions(-)
> >
> > diff --git a/block/sed-opal.c b/block/sed-opal.c
> > index bf1406e..4985d95 100644
> > --- a/block/sed-opal.c
> > +++ b/block/sed-opal.c
> > @@ -2346,7 +2346,10 @@ EXPORT_SYMBOL(opal_unlock_from_suspend);
> >
> > int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
> > {
> > + void *ioctl_ptr;
> > + int ret = -ENOTTY;
> > void __user *arg = (void __user *)ptr;
> > + unsigned int cmd_size = _IOC_SIZE(cmd);
> >
> > if (!capable(CAP_SYS_ADMIN))
> > return -EACCES;
>
> We usually have a size check in there to avoid allocating large amounts
> of memory. _IOC_SIZEBITS is 14, so you can have up to 16kb here, which
> is probably ok, but I'd recommend either adding a comment to say that
> it is, or just checking against the largest realistic size.
Right now it's up to the storage driver to call into the sed-opal ioctl.
We have a function is_sed_ioctl() which is called before jumping into sed_ioctl.
So we will be weeding out any non opal ioctls before getting in there so I don't
see any overly large 16kb allocations happening.
^ permalink raw reply [flat|nested] 26+ messages in thread