[PATCH] block: move CAP_SYS_ADMIN check in blkdev_roset()

[PATCH] block: move CAP_SYS_ADMIN check in blkdev_roset()

[Ilya Dryomov]

Check for CAP_SYS_ADMIN before calling into the driver, similar to
blkdev_flushbuf().  This is safer and can spare a check in the driver.

(Currently BLKROSET is overridden by md and rbd, rbd is missing the
check.  md has the check, but it covers a lot more than BLKROSET.)

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
Al, this appears to go back to your "[PATCH] block ioctl cleanup",
history commit c6973580141c.  2002 was a long time ago, but still ;)
Was there a reason you made BLKFLSBUF check for CAP_SYS_ADMIN before
->ioctl() and BLKROSET after?
---
 block/ioctl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index 0de02ee67eed..3f81bc50ac00 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -437,11 +437,12 @@ static int blkdev_roset(struct block_device *bdev, fmode_t mode,
 {
 	int ret, n;
 
+	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);
-- 
2.4.3

Re: [PATCH] block: move CAP_SYS_ADMIN check in blkdev_roset()

[Al Viro]

On Wed, Oct 18, 2017 at 02:38:38PM +0200, Ilya Dryomov wrote:
> Check for CAP_SYS_ADMIN before calling into the driver, similar to
> blkdev_flushbuf().  This is safer and can spare a check in the driver.
> 
> (Currently BLKROSET is overridden by md and rbd, rbd is missing the
> check.  md has the check, but it covers a lot more than BLKROSET.)
> 
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
> Al, this appears to go back to your "[PATCH] block ioctl cleanup",
> history commit c6973580141c.  2002 was a long time ago, but still ;)
> Was there a reason you made BLKFLSBUF check for CAP_SYS_ADMIN before
> ->ioctl() and BLKROSET after?

It was a long time ago, indeed...  The funny part is, at the time
there had been no ->ioctl() instances with unusual BLKROSET handling
left; I really don't remember what had left to the override for
those remaining and (assuming it hadn't been a plain and simple braino)
the reasons for leaving the check to drivers that might eventually
want to add such overrides would be in whatever discussion that
had lead to leaving that override...

There was a *lot* of patch series (semi)manual reordering/rebasing, so
it might have easily been braindamage on conflict resolution during
rebase.

gendisk work had been literally hundreds of patches all over the
drivers/* over the summer and autumn of 2002; I have bits and pieces of
email archives from back then, but quick grep doesn't catch any
discussions along those lines and they are incomplete ;-/

Anyway,
	a) I don't see any reason for drivers to relax the checks on
BLKROSET and rbd lacking those is almost certainly a bug
	b) Acked-by: Al Viro <viro@zeniv.linux.org.uk>
	c) I can push it through vfs tree, but it would probably make
more sense block one.

Re: [PATCH] block: move CAP_SYS_ADMIN check in blkdev_roset()

[Ilya Dryomov]

On Thu, Oct 19, 2017 at 2:14 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Oct 18, 2017 at 02:38:38PM +0200, Ilya Dryomov wrote:
>> Check for CAP_SYS_ADMIN before calling into the driver, similar to
>> blkdev_flushbuf().  This is safer and can spare a check in the driver.
>>
>> (Currently BLKROSET is overridden by md and rbd, rbd is missing the
>> check.  md has the check, but it covers a lot more than BLKROSET.)
>>
>> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
>> ---
>> Al, this appears to go back to your "[PATCH] block ioctl cleanup",
>> history commit c6973580141c.  2002 was a long time ago, but still ;)
>> Was there a reason you made BLKFLSBUF check for CAP_SYS_ADMIN before
>> ->ioctl() and BLKROSET after?
>
> It was a long time ago, indeed...  The funny part is, at the time
> there had been no ->ioctl() instances with unusual BLKROSET handling
> left; I really don't remember what had left to the override for
> those remaining and (assuming it hadn't been a plain and simple braino)
> the reasons for leaving the check to drivers that might eventually
> want to add such overrides would be in whatever discussion that
> had lead to leaving that override...
>
> There was a *lot* of patch series (semi)manual reordering/rebasing, so
> it might have easily been braindamage on conflict resolution during
> rebase.
>
> gendisk work had been literally hundreds of patches all over the
> drivers/* over the summer and autumn of 2002; I have bits and pieces of
> email archives from back then, but quick grep doesn't catch any
> discussions along those lines and they are incomplete ;-/
>
> Anyway,
>         a) I don't see any reason for drivers to relax the checks on
> BLKROSET and rbd lacking those is almost certainly a bug
>         b) Acked-by: Al Viro <viro@zeniv.linux.org.uk>
>         c) I can push it through vfs tree, but it would probably make
> more sense block one.

Jens, can you pick this up for 4.15?

Thanks,

                Ilya

Re: [PATCH] block: move CAP_SYS_ADMIN check in blkdev_roset()

[Jens Axboe]

On 10/24/2017 11:30 PM, Ilya Dryomov wrote:
> On Thu, Oct 19, 2017 at 2:14 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Wed, Oct 18, 2017 at 02:38:38PM +0200, Ilya Dryomov wrote:
>>> Check for CAP_SYS_ADMIN before calling into the driver, similar to
>>> blkdev_flushbuf().  This is safer and can spare a check in the driver.
>>>
>>> (Currently BLKROSET is overridden by md and rbd, rbd is missing the
>>> check.  md has the check, but it covers a lot more than BLKROSET.)
>>>
>>> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
>>> ---
>>> Al, this appears to go back to your "[PATCH] block ioctl cleanup",
>>> history commit c6973580141c.  2002 was a long time ago, but still ;)
>>> Was there a reason you made BLKFLSBUF check for CAP_SYS_ADMIN before
>>> ->ioctl() and BLKROSET after?
>>
>> It was a long time ago, indeed...  The funny part is, at the time
>> there had been no ->ioctl() instances with unusual BLKROSET handling
>> left; I really don't remember what had left to the override for
>> those remaining and (assuming it hadn't been a plain and simple braino)
>> the reasons for leaving the check to drivers that might eventually
>> want to add such overrides would be in whatever discussion that
>> had lead to leaving that override...
>>
>> There was a *lot* of patch series (semi)manual reordering/rebasing, so
>> it might have easily been braindamage on conflict resolution during
>> rebase.
>>
>> gendisk work had been literally hundreds of patches all over the
>> drivers/* over the summer and autumn of 2002; I have bits and pieces of
>> email archives from back then, but quick grep doesn't catch any
>> discussions along those lines and they are incomplete ;-/
>>
>> Anyway,
>>         a) I don't see any reason for drivers to relax the checks on
>> BLKROSET and rbd lacking those is almost certainly a bug
>>         b) Acked-by: Al Viro <viro@zeniv.linux.org.uk>
>>         c) I can push it through vfs tree, but it would probably make
>> more sense block one.
> 
> Jens, can you pick this up for 4.15?

Done, thanks.

-- 
Jens Axboe