* Sed-opal fixups
@ 2017-02-09 17:19 Scott Bauer
2017-02-09 17:20 ` [PATCH V3 1/2] uapi: sed-opal fix IOW for activate lsp to use correct struct Scott Bauer
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Scott Bauer @ 2017-02-09 17:19 UTC (permalink / raw)
To: linux-nvme
Cc: keith.busch, arnd, hch, linux-kernel, axboe, David.Laight,
linux-block, jonathan.derrick
It may be too late to change anyhting in the uapi header. When we
switched over to using IOC_SIZE I found a bug where I had switched
up a structure in one of the series from v4 to v5 but never changed
the structure in the IOW. The structure that was in there was to small
so when we kzalloc on it we don't request enough space. It worked before
because we were using the cmd strictly as a command #, not using the IOC
and friends.
If it's too late to modify that IOW, I can work around it by reallocing
on the correct size for that command only. I verified the rest of the
commands and the structures are the same.
Let me know what you think, please.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH V3 1/2] uapi: sed-opal fix IOW for activate lsp to use correct struct 2017-02-09 17:19 Sed-opal fixups Scott Bauer @ 2017-02-09 17:20 ` Scott Bauer 2017-02-09 17:20 ` [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN Scott Bauer 2017-02-09 17:43 ` Sed-opal fixups David Laight 2 siblings, 0 replies; 13+ messages in thread From: Scott Bauer @ 2017-02-09 17:20 UTC (permalink / raw) To: linux-nvme Cc: keith.busch, arnd, hch, linux-kernel, axboe, David.Laight, linux-block, Scott Bauer, jonathan.derrick the IOW for the IOC_OPAL_ACTIVATE_LSP took the wrong strcure which would give us the wrong size when using _IOC_SIZE, switch it to the right structure. Fixes: 058f8a2 ("Include: Uapi: Add user ABI for Sed/Opal") Signed-off-by: Scott Bauer <scott.bauer@intel.com> --- include/uapi/linux/sed-opal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/uapi/linux/sed-opal.h b/include/uapi/linux/sed-opal.h index fc06e3a..c72e073 100644 --- a/include/uapi/linux/sed-opal.h +++ b/include/uapi/linux/sed-opal.h @@ -106,7 +106,7 @@ struct opal_mbr_data { #define IOC_OPAL_SAVE _IOW('p', 220, struct opal_lock_unlock) #define IOC_OPAL_LOCK_UNLOCK _IOW('p', 221, struct opal_lock_unlock) #define IOC_OPAL_TAKE_OWNERSHIP _IOW('p', 222, struct opal_key) -#define IOC_OPAL_ACTIVATE_LSP _IOW('p', 223, struct opal_key) +#define IOC_OPAL_ACTIVATE_LSP _IOW('p', 223, struct opal_lr_act) #define IOC_OPAL_SET_PW _IOW('p', 224, struct opal_new_pw) #define IOC_OPAL_ACTIVATE_USR _IOW('p', 225, struct opal_session_info) #define IOC_OPAL_REVERT_TPR _IOW('p', 226, struct opal_key) -- 2.7.4 _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN 2017-02-09 17:19 Sed-opal fixups Scott Bauer 2017-02-09 17:20 ` [PATCH V3 1/2] uapi: sed-opal fix IOW for activate lsp to use correct struct Scott Bauer @ 2017-02-09 17:20 ` Scott Bauer 2017-02-09 19:51 ` Rafael Antognolli ` (2 more replies) 2017-02-09 17:43 ` Sed-opal fixups David Laight 2 siblings, 3 replies; 13+ messages in thread From: Scott Bauer @ 2017-02-09 17:20 UTC (permalink / raw) To: linux-nvme Cc: keith.busch, arnd, hch, linux-kernel, axboe, David.Laight, linux-block, Scott Bauer, jonathan.derrick 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; @@ -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 ^ permalink raw reply related [flat|nested] 13+ 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 ` [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN Scott Bauer @ 2017-02-09 19:51 ` Rafael Antognolli 2017-02-10 7:45 ` Johannes Thumshirn 2017-02-10 8:01 ` Arnd Bergmann 2 siblings, 0 replies; 13+ 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] 13+ 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 ` [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN Scott Bauer 2017-02-09 19:51 ` Rafael Antognolli @ 2017-02-10 7:45 ` Johannes Thumshirn 2017-02-10 10:28 ` David Laight 2017-02-10 8:01 ` Arnd Bergmann 2 siblings, 1 reply; 13+ 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] 13+ 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 2017-02-10 11:00 ` Arnd Bergmann 0 siblings, 1 reply; 13+ 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] 13+ 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 0 siblings, 0 replies; 13+ 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] 13+ 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 ` [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN Scott Bauer 2017-02-09 19:51 ` Rafael Antognolli 2017-02-10 7:45 ` Johannes Thumshirn @ 2017-02-10 8:01 ` Arnd Bergmann 2017-02-10 15:57 ` Scott Bauer 2 siblings, 1 reply; 13+ 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] 13+ 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 0 siblings, 0 replies; 13+ 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] 13+ messages in thread
* RE: Sed-opal fixups 2017-02-09 17:19 Sed-opal fixups Scott Bauer 2017-02-09 17:20 ` [PATCH V3 1/2] uapi: sed-opal fix IOW for activate lsp to use correct struct Scott Bauer 2017-02-09 17:20 ` [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN Scott Bauer @ 2017-02-09 17:43 ` David Laight 2017-02-09 17:45 ` Scott Bauer 2 siblings, 1 reply; 13+ messages in thread From: David Laight @ 2017-02-09 17:43 UTC (permalink / raw) To: '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: Scott Bauer > Sent: 09 February 2017 17:20 > It may be too late to change anyhting in the uapi header. When we > switched over to using IOC_SIZE I found a bug where I had switched > up a structure in one of the series from v4 to v5 but never changed > the structure in the IOW. The structure that was in there was to small > so when we kzalloc on it we don't request enough space. It worked before > because we were using the cmd strictly as a command #, not using the IOC > and friends. > > If it's too late to modify that IOW, I can work around it by reallocing > on the correct size for that command only. I verified the rest of the > commands and the structures are the same. > > Let me know what you think, please. Maybe define IOC_OPAL_ACTIVATE_LSP_OLD to the incorrect value and IOC_OPAL_ACTIVATE_LSP to the correct one. But that relies on any users specifying the correct structure. I wouldn't guarantee that. At the top of the driver's ioctl path add: if (cmd == IOC_OPAL_ACTIVATE_LSP_OLD) cmd = IOC_OPAL_ACTIVATE_LSP; For some code I added a userspace wrapper on ioctl() to check the size of the supplied arg matched that required by the 'cmd'. I've also done the same in the kernel. (all as compile time checks). David _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Sed-opal fixups 2017-02-09 17:43 ` Sed-opal fixups David Laight @ 2017-02-09 17:45 ` Scott Bauer 2017-02-09 18:24 ` Jens Axboe 0 siblings, 1 reply; 13+ messages in thread From: Scott Bauer @ 2017-02-09 17:45 UTC (permalink / raw) To: David Laight Cc: keith.busch@intel.com, arnd@arndb.de, hch@infradead.org, linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org, axboe@fb.com, linux-block@vger.kernel.org, jonathan.derrick@intel.com On Thu, Feb 09, 2017 at 05:43:20PM +0000, David Laight wrote: > From: Scott Bauer > > Sent: 09 February 2017 17:20 > > It may be too late to change anyhting in the uapi header. When we > > switched over to using IOC_SIZE I found a bug where I had switched > > up a structure in one of the series from v4 to v5 but never changed > > the structure in the IOW. The structure that was in there was to small > > so when we kzalloc on it we don't request enough space. It worked before > > because we were using the cmd strictly as a command #, not using the IOC > > and friends. > > > > If it's too late to modify that IOW, I can work around it by reallocing > > on the correct size for that command only. I verified the rest of the > > commands and the structures are the same. > > > > Let me know what you think, please. > > Maybe define IOC_OPAL_ACTIVATE_LSP_OLD to the incorrect value and > IOC_OPAL_ACTIVATE_LSP to the correct one. > But that relies on any users specifying the correct structure. > I wouldn't guarantee that. I think I'm the only userspace user right now, this went in on monday, so I can can change my tooling easily. I just wasnt sure if there was a set time where the user ABI cannot be changed. > > At the top of the driver's ioctl path add: > if (cmd == IOC_OPAL_ACTIVATE_LSP_OLD) cmd = IOC_OPAL_ACTIVATE_LSP; > I think it would have to be the other way around the correct sized one would be IOC_OPAL_ACTIAVE_LSP_NEW so the check would be: if (cmd == IOC_OPAL_ACTIVATE_LSP) cmd = IOC_OPAL_ACTIVATE_LSP_NEW. If we're allowed to change it (the bad sized one) from LSP to LSP_OLD then we should just change the structure. If we have to leave it we need to introduce a _NEW with the correct size. > For some code I added a userspace wrapper on ioctl() to check the > size of the supplied arg matched that required by the 'cmd'. > I've also done the same in the kernel. > (all as compile time checks). > > David > > _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Sed-opal fixups 2017-02-09 17:45 ` Scott Bauer @ 2017-02-09 18:24 ` Jens Axboe 2017-02-09 20:01 ` Scott Bauer 0 siblings, 1 reply; 13+ messages in thread From: Jens Axboe @ 2017-02-09 18:24 UTC (permalink / raw) To: Scott Bauer, David Laight Cc: keith.busch@intel.com, arnd@arndb.de, hch@infradead.org, linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org, linux-block@vger.kernel.org, jonathan.derrick@intel.com On 02/09/2017 10:45 AM, Scott Bauer wrote: > On Thu, Feb 09, 2017 at 05:43:20PM +0000, David Laight wrote: >> From: Scott Bauer >>> Sent: 09 February 2017 17:20 >>> It may be too late to change anyhting in the uapi header. When we >>> switched over to using IOC_SIZE I found a bug where I had switched >>> up a structure in one of the series from v4 to v5 but never changed >>> the structure in the IOW. The structure that was in there was to small >>> so when we kzalloc on it we don't request enough space. It worked before >>> because we were using the cmd strictly as a command #, not using the IOC >>> and friends. >>> >>> If it's too late to modify that IOW, I can work around it by reallocing >>> on the correct size for that command only. I verified the rest of the >>> commands and the structures are the same. >>> >>> Let me know what you think, please. >> >> Maybe define IOC_OPAL_ACTIVATE_LSP_OLD to the incorrect value and >> IOC_OPAL_ACTIVATE_LSP to the correct one. >> But that relies on any users specifying the correct structure. >> I wouldn't guarantee that. > > I think I'm the only userspace user right now, this went in on monday, > so I can can change my tooling easily. I just wasnt sure if there was a > set time where the user ABI cannot be changed. We can still change it, and we definitely should if it improves the interface. It's not a ABI until it's in a released, final kernel. -- Jens Axboe ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Sed-opal fixups 2017-02-09 18:24 ` Jens Axboe @ 2017-02-09 20:01 ` Scott Bauer 0 siblings, 0 replies; 13+ messages in thread From: Scott Bauer @ 2017-02-09 20:01 UTC (permalink / raw) To: Jens Axboe Cc: hch@infradead.org, arnd@arndb.de, linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org, keith.busch@intel.com, David Laight, linux-block@vger.kernel.org, jonathan.derrick@intel.com e0;136;0csOn Thu, Feb 09, 2017 at 11:24:58AM -0700, Jens Axboe wrote: > On 02/09/2017 10:45 AM, Scott Bauer wrote: > > On Thu, Feb 09, 2017 at 05:43:20PM +0000, David Laight wrote: > >> From: Scott Bauer > >>> Sent: 09 February 2017 17:20 > >>> It may be too late to change anyhting in the uapi header. When we > >>> switched over to using IOC_SIZE I found a bug where I had switched > >>> up a structure in one of the series from v4 to v5 but never changed > >>> the structure in the IOW. The structure that was in there was to small > >>> so when we kzalloc on it we don't request enough space. It worked before > >>> because we were using the cmd strictly as a command #, not using the IOC > >>> and friends. > >>> > >>> If it's too late to modify that IOW, I can work around it by reallocing > >>> on the correct size for that command only. I verified the rest of the > >>> commands and the structures are the same. > >>> > >>> Let me know what you think, please. > >> > >> Maybe define IOC_OPAL_ACTIVATE_LSP_OLD to the incorrect value and > >> IOC_OPAL_ACTIVATE_LSP to the correct one. > >> But that relies on any users specifying the correct structure. > >> I wouldn't guarantee that. > > > > I think I'm the only userspace user right now, this went in on monday, > > so I can can change my tooling easily. I just wasnt sure if there was a > > set time where the user ABI cannot be changed. > > We can still change it, and we definitely should if it improves the > interface. It's not a ABI until it's in a released, final kernel. > Thanks for the clarification, Jens. In that case I'd like to keep the ABI fixup in patch one and the dynamic allocation in patch 2. I'd like to wait for Christoph's blessing as well before you take it. Thanks _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-02-10 15:57 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-02-09 17:19 Sed-opal fixups Scott Bauer 2017-02-09 17:20 ` [PATCH V3 1/2] uapi: sed-opal fix IOW for activate lsp to use correct struct Scott Bauer 2017-02-09 17:20 ` [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN Scott Bauer 2017-02-09 19:51 ` Rafael Antognolli 2017-02-10 7:45 ` Johannes Thumshirn 2017-02-10 10:28 ` David Laight 2017-02-10 11:00 ` Arnd Bergmann 2017-02-10 8:01 ` Arnd Bergmann 2017-02-10 15:57 ` Scott Bauer 2017-02-09 17:43 ` Sed-opal fixups David Laight 2017-02-09 17:45 ` Scott Bauer 2017-02-09 18:24 ` Jens Axboe 2017-02-09 20:01 ` Scott Bauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox