SED Opal Fixups

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.

[PATCH V5 1/4] block: sed-opal: change ioctl to take user pointer instead of unsinged long

[Scott Bauer]

Signed-off-by: Scott Bauer <scott.bauer@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

[PATCH V5 2/4] uapi: sed-opal fix IOW for activate lsp to use correct struct

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

[PATCH V5 3/4] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[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 <arnd@arndb.de>
Signed-off-by: Scott Bauer <scott.bauer@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

[PATCH V5 4/4] Maintainers: Modify SED list from nvme to block

[Scott Bauer]

Signed-off-by: Scott Bauer <scott.bauer@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.bauer@intel.com>
 M:	Jonathan Derrick <jonathan.derrick@intel.com>
 M:	Rafael Antognolli <rafael.antognolli@intel.com>
-L:	linux-nvme@lists.infradead.org
+L:	linux-block@vger.kernel.org
 S:	Supported
 F:	block/sed*
 F:	block/opal_proto.h
-- 
2.7.4

Re: [PATCH V5 1/4] block: sed-opal: change ioctl to take user pointer instead of unsinged long

[Scott Bauer]

esOn Mon, Feb 13, 2017 at 09:11:09AM -0700, Scott Bauer wrote:
> Signed-off-by: Scott Bauer <scott.bauer@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);

ugh, I apparently messed up my rebase these should be in patch 2 or maybe I should
sqash p1 and p2  together.

Re: [PATCH V5 3/4] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

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

RE: [PATCH V5 3/4] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[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

Re: [PATCH V5 3/4] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[Arnd Bergmann]

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

RE: [PATCH V5 3/4] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[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

Re: [PATCH V5 3/4] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[Arnd Bergmann]

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

Re: [PATCH V5 1/4] block: sed-opal: change ioctl to take user pointer instead of unsinged long

[Christoph Hellwig]

On Mon, Feb 13, 2017 at 09:15:10AM -0700, Scott Bauer wrote:
> esOn Mon, Feb 13, 2017 at 09:11:09AM -0700, Scott Bauer wrote:
> > Signed-off-by: Scott Bauer <scott.bauer@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);
> 
> ugh, I apparently messed up my rebase these should be in patch 2 or maybe I should
> sqash p1 and p2  together.

I guess it should be 1 and 3.  No real opinipon on that, but can you
chose a simpler and more fitting name than ioctl_ptr?  I'd suggest 'p'.

Re: [PATCH V5 2/4] uapi: sed-opal fix IOW for activate lsp to use correct struct

[Christoph Hellwig]

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

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

Re: [PATCH V5 4/4] Maintainers: Modify SED list from nvme to block

[Christoph Hellwig]

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

Let's get it in ASAP as well.