SED Opal Fixups

[Scott Bauer]

So we have a few patches here, they're pretty small. First patch changes
the sed-opal ioctl function parameters to take a void __user* instead of
an unsigned long, this required a small cast in the nvme driver.
Patch 2 is a UAPI fixup for the IOW to make an ioctl
the right size. Patch 3 fixes a compiliation error when building using
KSAN due to the stack frame being too large. And lastly we move the
Maintainers list from nvme to linux-block.

[Scott Bauer]

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...@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

[Scott Bauer]

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 <ar...@arndb.de>

Signed-off-by: Scott Bauer <scott...@intel.com>
---

block/sed-opal.c | 130 +++++++++++++++++++------------------------------------
1 file changed, 45 insertions(+), 85 deletions(-)

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);
--
2.7.4

[Scott Bauer]

Signed-off-by: Scott Bauer <scott...@intel.com>
---

block/sed-opal.c | 6 ++++--
drivers/nvme/host/core.c | 3 ++-
include/linux/sed-opal.h | 4 ++--
3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index bf1406e..2448d4a 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -2344,9 +2344,11 @@ bool opal_unlock_from_suspend(struct opal_dev *dev)
}
EXPORT_SYMBOL(opal_unlock_from_suspend);

-int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
+int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg)
{
- void __user *arg = (void __user *)ptr;
+ void *ioctl_ptr;
+ int ret = -ENOTTY;
+ unsigned int cmd_size = _IOC_SIZE(cmd);

if (!capable(CAP_SYS_ADMIN))
return -EACCES;
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d9f4903..04c48e7 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -811,7 +811,8 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
return nvme_nvm_ioctl(ns, cmd, arg);
#endif
if (is_sed_ioctl(cmd))
- return sed_ioctl(&ns->ctrl->opal_dev, cmd, arg);
+ return sed_ioctl(&ns->ctrl->opal_dev, cmd,
+ (void __user *) arg);
return -ENOTTY;
}
}
diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h
index af1a85e..205d520 100644
--- a/include/linux/sed-opal.h
+++ b/include/linux/sed-opal.h
@@ -132,7 +132,7 @@ struct opal_dev {
#ifdef CONFIG_BLK_SED_OPAL
bool opal_unlock_from_suspend(struct opal_dev *dev);
void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv);
-int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr);
+int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *ioctl_ptr);

static inline bool is_sed_ioctl(unsigned int cmd)
{
@@ -160,7 +160,7 @@ static inline bool is_sed_ioctl(unsigned int cmd)
}

static inline int sed_ioctl(struct opal_dev *dev, unsigned int cmd,
- unsigned long ptr)
+ void __user *ioctl_ptr)
{
return 0;
}
--
2.7.4

[Scott Bauer]

Signed-off-by: Scott Bauer <scott...@intel.com>
---

MAINTAINERS | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index e325373..b983b25 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11094,7 +11094,7 @@ SECURE ENCRYPTING DEVICE (SED) OPAL DRIVER
M: Scott Bauer <scott...@intel.com>
M: Jonathan Derrick <jonathan...@intel.com>
M: Rafael Antognolli <rafael.a...@intel.com>
-L: linux...@lists.infradead.org
+L: linux...@vger.kernel.org
S: Supported
F: block/sed*
F: block/opal_proto.h
--
2.7.4

[David Laight]

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

[Scott Bauer]

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.

[Arnd Bergmann]

On Mon, Feb 13, 2017 at 5:29 PM, Scott Bauer <scott...@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

[David Laight]

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

[Arnd Bergmann]

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

[Christoph Hellwig]

Reviewed-by: Christoph Hellwig <h...@lst.de>

Let's get it in ASAP as well.

[Christoph Hellwig]

Reviewed-by: Christoph Hellwig <h...@lst.de>

Let's get this one in ASAP while waiting for a respin of the KASAN
fix.

[Scott Bauer]

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 allocate

using _IOC_SIZE()

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

Reported-by: Arnd Bergmann <ar...@arndb.de>

Signed-off-by: Scott Bauer <scott...@intel.com>
---

block/sed-opal.c | 133 ++++++++++++++++-------------------------------
drivers/nvme/host/core.c | 3 +-
include/linux/sed-opal.h | 4 +-
3 files changed, 50 insertions(+), 90 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index bf1406e..e95b8a5 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -2344,9 +2344,10 @@ bool opal_unlock_from_suspend(struct opal_dev *dev)

}
EXPORT_SYMBOL(opal_unlock_from_suspend);

-int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
+int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg)
{
- void __user *arg = (void __user *)ptr;

+ void *p;

+ int ret = -ENOTTY;

if (!capable(CAP_SYS_ADMIN))
return -EACCES;
@@ -2355,94 +2356,52 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)

+ p = memdup_user(arg, _IOC_SIZE(cmd));
+ if (IS_ERR(p))
+ return PTR_ERR(p);

- 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);

- }

- 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, p);

+ break;
+ case IOC_OPAL_LOCK_UNLOCK:

+ ret = opal_lock_unlock(dev, p);

+ break;
+ case IOC_OPAL_TAKE_OWNERSHIP:

+ ret = opal_take_ownership(dev, p);

+ break;
+ case IOC_OPAL_ACTIVATE_LSP:

+ ret = opal_activate_lsp(dev, p);

+ break;
+ case IOC_OPAL_SET_PW:

+ ret = opal_set_new_pw(dev, p);

+ break;
+ case IOC_OPAL_ACTIVATE_USR:

+ ret = opal_activate_user(dev, p);

+ break;
+ case IOC_OPAL_REVERT_TPR:

+ ret = opal_reverttper(dev, p);

+ break;
+ case IOC_OPAL_LR_SETUP:

+ ret = opal_setup_locking_range(dev, p);

+ break;
+ case IOC_OPAL_ADD_USR_TO_LR:

+ ret = opal_add_user_to_lr(dev, p);

+ break;
+ case IOC_OPAL_ENABLE_DISABLE_MBR:

+ ret = opal_enable_disable_shadow_mbr(dev, p);

+ break;
+ case IOC_OPAL_ERASE_LR:

+ ret = opal_erase_locking_range(dev, p);

+ break;
+ case IOC_OPAL_SECURE_ERASE_LR:

+ ret = opal_secure_erase_locking_range(dev, p);

+ break;
default:
pr_warn("No such Opal Ioctl %u\n", cmd);
}
- return -ENOTTY;
+

+ kfree(p);
+ return ret;
}
EXPORT_SYMBOL_GPL(sed_ioctl);

[Scott Bauer]

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...@intel.com>
---

include/uapi/linux/sed-opal.h | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

[Scott Bauer]

Signed-off-by: Scott Bauer <scott...@intel.com>
---

MAINTAINERS | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)