| Message ID | 1487002272-17940-4-git-send-email-scott.bauer@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 13, 2017 at 04:30:36PM +0000, David Laight wrote: > From: Scott Bauer Sent: 13 February 2017 16:11 > > 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() > > Think I'd not that this simplifies the code considerably. > AFAICT CONFIG_KASAN is a just brainf*ck. > It at least needs annotation that copy_from_user() has properties > similar to memset(). > So if the size matches that of the type then no guard space (etc) > is required. > I don't really follow what you're saying. Do you want me to just include that the patch cleans up the ioctl path a bit. I need to include the KASAN part since there was build breakage and the series does fix it even if it simplifies the path as well. As for the memset part, we never copy back to userland so there's no chance of data leakage which is what it seems you're hinting at. > ... > > + ioctl_ptr = memdup_user(arg, _IOC_SIZE(cmd)); > > + if (IS_ERR_OR_NULL(ioctl_ptr)) { > > + ret = PTR_ERR(ioctl_ptr); > > + goto out; > ... > > + out: > > + kfree(ioctl_ptr); > > + return ret; > > } > > That error path doesn't look quite right to me. > > David > good god, this is a mess this morning. Thanks for the catch, I'll review these more aggressively before sending out.
From: Scott Bauer Sent: 13 February 2017 16:11 > 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() Think I'd not that this simplifies the code considerably. AFAICT CONFIG_KASAN is a just brainf*ck. It at least needs annotation that copy_from_user() has properties similar to memset(). So if the size matches that of the type then no guard space (etc) is required. ... > + ioctl_ptr = memdup_user(arg, _IOC_SIZE(cmd)); > + if (IS_ERR_OR_NULL(ioctl_ptr)) { > + ret = PTR_ERR(ioctl_ptr); > + goto out; ... > + out: > + kfree(ioctl_ptr); > + return ret; > } That error path doesn't look quite right to me. David
On Mon, Feb 13, 2017 at 5:29 PM, Scott Bauer <scott.bauer@intel.com> wrote: > On Mon, Feb 13, 2017 at 04:30:36PM +0000, David Laight wrote: >> From: Scott Bauer Sent: 13 February 2017 16:11 >> > 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() >> >> Think I'd not that this simplifies the code considerably. >> AFAICT CONFIG_KASAN is a just brainf*ck. >> It at least needs annotation that copy_from_user() has properties >> similar to memset(). >> So if the size matches that of the type then no guard space (etc) >> is required. I think it still would, as the pointer to the local variable gets passed through dev->func_data[]. >> ... >> > + ioctl_ptr = memdup_user(arg, _IOC_SIZE(cmd)); >> > + if (IS_ERR_OR_NULL(ioctl_ptr)) { >> > + ret = PTR_ERR(ioctl_ptr); >> > + goto out; >> ... >> > + out: >> > + kfree(ioctl_ptr); >> > + return ret; >> > } > > >> >> That error path doesn't look quite right to me. >> >> David >> > > good god, this is a mess this morning. Thanks for the catch, I'll review these > more aggressively before sending out. memdup_user() never returns NULL, and generally speaking any use of IS_ERR_OR_NULL() indicates that there is something wrong with the interface, so aside from passing the right pointer (or NULL) into kfree() I think using IS_ERR() is the correct solution. Arnd
From: Arnd Bergmann > Sent: 13 February 2017 17:02 ... > >> > + ioctl_ptr = memdup_user(arg, _IOC_SIZE(cmd)); > >> > + if (IS_ERR_OR_NULL(ioctl_ptr)) { > >> > + ret = PTR_ERR(ioctl_ptr); > >> > + goto out; > >> ... > >> > + out: > >> > + kfree(ioctl_ptr); > >> > + return ret; ... > >> That error path doesn't look quite right to me. ... > > good god, this is a mess this morning. Thanks for the catch, I'll review these > > more aggressively before sending out. > > memdup_user() never returns NULL, and generally speaking any use of > IS_ERR_OR_NULL() indicates that there is something wrong with the > interface, so aside from passing the right pointer (or NULL) into kfree() > I think using IS_ERR() is the correct solution. You missed the problem entirely. Code needs to be: if (IS_ERR(ioctl_ptr)) return PTR_ERR(ioctl_ptr); David
On Mon, Feb 13, 2017 at 6:07 PM, David Laight <David.Laight@aculab.com> wrote: > From: Arnd Bergmann >> Sent: 13 February 2017 17:02 > ... >> >> > + ioctl_ptr = memdup_user(arg, _IOC_SIZE(cmd)); >> >> > + if (IS_ERR_OR_NULL(ioctl_ptr)) { >> >> > + ret = PTR_ERR(ioctl_ptr); >> >> > + goto out; >> >> ... >> >> > + out: >> >> > + kfree(ioctl_ptr); >> >> > + return ret; > ... >> >> That error path doesn't look quite right to me. > ... >> > good god, this is a mess this morning. Thanks for the catch, I'll review these >> > more aggressively before sending out. >> >> memdup_user() never returns NULL, and generally speaking any use of >> IS_ERR_OR_NULL() indicates that there is something wrong with the >> interface, so aside from passing the right pointer (or NULL) into kfree() >> I think using IS_ERR() is the correct solution. > > You missed the problem entirely. > Code needs to be: > if (IS_ERR(ioctl_ptr)) > return PTR_ERR(ioctl_ptr); Sorry if that wasn't clear, I expected the part about the kfree(errptr) to be obvious but was trying to avoid having Scott go through another revision for removing the IS_ERR_OR_NULL() after fixing the first problem. Arnd
diff --git a/block/sed-opal.c b/block/sed-opal.c index 2448d4a..5733248 100644 --- a/block/sed-opal.c +++ b/block/sed-opal.c @@ -2348,7 +2348,6 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg) { void *ioctl_ptr; int ret = -ENOTTY; - unsigned int cmd_size = _IOC_SIZE(cmd); if (!capable(CAP_SYS_ADMIN)) return -EACCES; @@ -2357,94 +2356,55 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg) 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 = memdup_user(arg, _IOC_SIZE(cmd)); + if (IS_ERR_OR_NULL(ioctl_ptr)) { + ret = PTR_ERR(ioctl_ptr); + 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);
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 | 130 +++++++++++++++++++------------------------------------ 1 file changed, 45 insertions(+), 85 deletions(-)