[PATCH] block: check CAP_SYS_ADMIN before calling driver BLKROSET

[PATCH] block: check CAP_SYS_ADMIN before calling driver BLKROSET

[Josh Durgin]

Calling the driver-specific ioctl first, then bailing because
CAP_SYS_ADMIN is not set, can lead to an inconsistency between the
driver and the block layer, where one thinks the device is read only,
and the other considers it readwrite.

Fix this by checking the capability first, like the rest of the block
ioctls that require it. This bug was introduced pre-git in commit
23deaf89e3075b9271c7f48a2764ae3ed8f9573a.

Signed-off-by: Josh Durgin <josh.durgin@inktank.com>
---
 block/ioctl.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index a31d91d..6e5933e 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -293,11 +293,11 @@ int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 		return 0;
 
 	case BLKROSET:
+		if (!capable(CAP_SYS_ADMIN))
+			return -EACCES;
 		ret = __blkdev_driver_ioctl(bdev, mode, cmd, arg);
 		if (!is_unrecognized_ioctl(ret))
 			return ret;
-		if (!capable(CAP_SYS_ADMIN))
-			return -EACCES;
 		if (get_user(n, (int __user *)(arg)))
 			return -EFAULT;
 		set_device_ro(bdev, n);
-- 
1.7.2.5

Re: [PATCH] block: check CAP_SYS_ADMIN before calling driver BLKROSET

[Jeff Moyer]

Josh Durgin <josh.durgin@inktank.com> writes:

> Calling the driver-specific ioctl first, then bailing because
> CAP_SYS_ADMIN is not set, can lead to an inconsistency between the
> driver and the block layer, where one thinks the device is read only,
> and the other considers it readwrite.

How?  If the driver returns anything other than EINVAL, ENOTTY or
ENOIOCTLCMD, that return code is propagated to the caller (that's what
the is_unrecognized_ioctl bit is about).

So, what problem are you actually running into?

> Fix this by checking the capability first, like the rest of the block
> ioctls that require it. This bug was introduced pre-git in commit
> 23deaf89e3075b9271c7f48a2764ae3ed8f9573a.

Mentioning a commit pre-git is confusing, to say the least.  You could
at least include a pointer to the tree you are using to generate this
hash, or maybe even include the commit message.

Anyway, if it wasn't clear:

NACK

Cheers,
Jeff

>
> Signed-off-by: Josh Durgin <josh.durgin@inktank.com>
> ---
>  block/ioctl.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block/ioctl.c b/block/ioctl.c
> index a31d91d..6e5933e 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -293,11 +293,11 @@ int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
>  		return 0;
>  
>  	case BLKROSET:
> +		if (!capable(CAP_SYS_ADMIN))
> +			return -EACCES;
>  		ret = __blkdev_driver_ioctl(bdev, mode, cmd, arg);
>  		if (!is_unrecognized_ioctl(ret))
>  			return ret;
> -		if (!capable(CAP_SYS_ADMIN))
> -			return -EACCES;
>  		if (get_user(n, (int __user *)(arg)))
>  			return -EFAULT;
>  		set_device_ro(bdev, n);