Sed-opal fixups

Sed-opal fixups

[Scott Bauer]

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.

[PATCH V3 1/2] 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 V3 2/2] 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 | 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

RE: Sed-opal fixups

[David Laight]

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

Re: Sed-opal fixups

[Scott Bauer]

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

Re: Sed-opal fixups

[Jens Axboe]

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

Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[Rafael Antognolli]

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

Re: Sed-opal fixups

[Scott Bauer]

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

Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[Johannes Thumshirn]

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

Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[Arnd Bergmann]

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

RE: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[David Laight]

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

Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[Arnd Bergmann]

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

Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[Scott Bauer]

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.