* [PATCH 0/2] allow blk-zoned ioctls without CAP_SYS_ADMIN @ 2021-05-31 13:54 Niklas Cassel 2021-05-31 13:54 ` [PATCH 1/2] blk-zoned: allow zone management send operations " Niklas Cassel ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Niklas Cassel @ 2021-05-31 13:54 UTC (permalink / raw) To: Jens Axboe; +Cc: Damien Le Moal, Niklas Cassel, linux-block, linux-kernel From: Niklas Cassel <niklas.cassel@wdc.com> Allow the following blk-zoned ioctls: BLKREPORTZONE, BLKRESETZONE, BLKOPENZONE, BLKCLOSEZONE, and BLKFINISHZONE to be performed without CAP_SYS_ADMIN. These ioctls instead only requires that the corresponding R/W access control flag to be successfully set on the opened file descriptor. (open()/openat() will fail with -EPERM if you try to open a file with flags that you lack permission for.) Niklas Cassel (2): blk-zoned: allow zone management send operations without CAP_SYS_ADMIN blk-zoned: allow BLKREPORTZONE without CAP_SYS_ADMIN block/blk-zoned.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] blk-zoned: allow zone management send operations without CAP_SYS_ADMIN 2021-05-31 13:54 [PATCH 0/2] allow blk-zoned ioctls without CAP_SYS_ADMIN Niklas Cassel @ 2021-05-31 13:54 ` Niklas Cassel 2021-05-31 13:54 ` [PATCH 2/2] blk-zoned: allow BLKREPORTZONE " Niklas Cassel 2021-05-31 23:34 ` [PATCH 0/2] allow blk-zoned ioctls " Damien Le Moal 2 siblings, 0 replies; 10+ messages in thread From: Niklas Cassel @ 2021-05-31 13:54 UTC (permalink / raw) To: Jens Axboe; +Cc: Damien Le Moal, Niklas Cassel, linux-block, linux-kernel From: Niklas Cassel <niklas.cassel@wdc.com> Zone management send operations (BLKRESETZONE, BLKOPENZONE, BLKCLOSEZONE and BLKFINISHZONE) should be allowed under the same permissions as write(). (write() does not require CAP_SYS_ADMIN). Additionally, other ioctls like BLKSECDISCARD and BLKZEROOUT only check if the fd was successfully opened with FMODE_WRITE. (They do not require CAP_SYS_ADMIN). Currently, zone management send operations require both CAP_SYS_ADMIN and that the fd was successfully opened with FMODE_WRITE. Remove the CAP_SYS_ADMIN requirement, so that zone management send operations match the access control requirement of write(), BLKSECDISCARD and BLKZEROOUT. Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> --- block/blk-zoned.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/block/blk-zoned.c b/block/blk-zoned.c index 250cb76ee615..0789e6e9f7db 100644 --- a/block/blk-zoned.c +++ b/block/blk-zoned.c @@ -349,9 +349,6 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, fmode_t mode, if (!blk_queue_is_zoned(q)) return -ENOTTY; - if (!capable(CAP_SYS_ADMIN)) - return -EACCES; - if (!(mode & FMODE_WRITE)) return -EBADF; -- 2.31.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] blk-zoned: allow BLKREPORTZONE without CAP_SYS_ADMIN 2021-05-31 13:54 [PATCH 0/2] allow blk-zoned ioctls without CAP_SYS_ADMIN Niklas Cassel 2021-05-31 13:54 ` [PATCH 1/2] blk-zoned: allow zone management send operations " Niklas Cassel @ 2021-05-31 13:54 ` Niklas Cassel 2021-06-03 9:51 ` David Sterba 2021-05-31 23:34 ` [PATCH 0/2] allow blk-zoned ioctls " Damien Le Moal 2 siblings, 1 reply; 10+ messages in thread From: Niklas Cassel @ 2021-05-31 13:54 UTC (permalink / raw) To: Jens Axboe; +Cc: Damien Le Moal, Niklas Cassel, linux-block, linux-kernel From: Niklas Cassel <niklas.cassel@wdc.com> Performing a BLKREPORTZONE operation should be allowed under the same permissions as read(). (read() does not require CAP_SYS_ADMIN). Remove the CAP_SYS_ADMIN requirement, and instead check that the fd was successfully opened with FMODE_READ. This way BLKREPORTZONE will match the access control requirement of read(). Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> --- block/blk-zoned.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block/blk-zoned.c b/block/blk-zoned.c index 0789e6e9f7db..e05fe8dbb06d 100644 --- a/block/blk-zoned.c +++ b/block/blk-zoned.c @@ -288,8 +288,8 @@ int blkdev_report_zones_ioctl(struct block_device *bdev, fmode_t mode, if (!blk_queue_is_zoned(q)) return -ENOTTY; - if (!capable(CAP_SYS_ADMIN)) - return -EACCES; + if (!(mode & FMODE_READ)) + return -EBADF; if (copy_from_user(&rep, argp, sizeof(struct blk_zone_report))) return -EFAULT; -- 2.31.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] blk-zoned: allow BLKREPORTZONE without CAP_SYS_ADMIN 2021-05-31 13:54 ` [PATCH 2/2] blk-zoned: allow BLKREPORTZONE " Niklas Cassel @ 2021-06-03 9:51 ` David Sterba 2021-06-03 10:00 ` Damien Le Moal 0 siblings, 1 reply; 10+ messages in thread From: David Sterba @ 2021-06-03 9:51 UTC (permalink / raw) To: Niklas Cassel; +Cc: Jens Axboe, Damien Le Moal, linux-block, linux-kernel On Mon, May 31, 2021 at 01:54:53PM +0000, Niklas Cassel wrote: > From: Niklas Cassel <niklas.cassel@wdc.com> > > Performing a BLKREPORTZONE operation should be allowed under the same > permissions as read(). (read() does not require CAP_SYS_ADMIN). > > Remove the CAP_SYS_ADMIN requirement, and instead check that the fd was > successfully opened with FMODE_READ. This way BLKREPORTZONE will match > the access control requirement of read(). Does this mean that a process that does not have read nor write access to the device itself (blocks) is capable of reading the zone information? Eg. some monitoring tool. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] blk-zoned: allow BLKREPORTZONE without CAP_SYS_ADMIN 2021-06-03 9:51 ` David Sterba @ 2021-06-03 10:00 ` Damien Le Moal 2021-06-03 10:04 ` David Sterba 0 siblings, 1 reply; 10+ messages in thread From: Damien Le Moal @ 2021-06-03 10:00 UTC (permalink / raw) To: dsterba, Niklas Cassel; +Cc: Jens Axboe, linux-block, linux-kernel On 2021/06/03 18:54, David Sterba wrote: > On Mon, May 31, 2021 at 01:54:53PM +0000, Niklas Cassel wrote: >> From: Niklas Cassel <niklas.cassel@wdc.com> >> >> Performing a BLKREPORTZONE operation should be allowed under the same >> permissions as read(). (read() does not require CAP_SYS_ADMIN). >> >> Remove the CAP_SYS_ADMIN requirement, and instead check that the fd was >> successfully opened with FMODE_READ. This way BLKREPORTZONE will match >> the access control requirement of read(). > > Does this mean that a process that does not have read nor write access > to the device itself (blocks) is capable of reading the zone > information? Eg. some monitoring tool. With this change, to do a report zones, the process will only need to have read access to the device. And if it has read access, it also means that it can read the zones content. -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] blk-zoned: allow BLKREPORTZONE without CAP_SYS_ADMIN 2021-06-03 10:00 ` Damien Le Moal @ 2021-06-03 10:04 ` David Sterba 2021-06-03 11:20 ` Damien Le Moal 0 siblings, 1 reply; 10+ messages in thread From: David Sterba @ 2021-06-03 10:04 UTC (permalink / raw) To: Damien Le Moal Cc: dsterba, Niklas Cassel, Jens Axboe, linux-block, linux-kernel On Thu, Jun 03, 2021 at 10:00:08AM +0000, Damien Le Moal wrote: > On 2021/06/03 18:54, David Sterba wrote: > > On Mon, May 31, 2021 at 01:54:53PM +0000, Niklas Cassel wrote: > >> From: Niklas Cassel <niklas.cassel@wdc.com> > >> > >> Performing a BLKREPORTZONE operation should be allowed under the same > >> permissions as read(). (read() does not require CAP_SYS_ADMIN). > >> > >> Remove the CAP_SYS_ADMIN requirement, and instead check that the fd was > >> successfully opened with FMODE_READ. This way BLKREPORTZONE will match > >> the access control requirement of read(). > > > > Does this mean that a process that does not have read nor write access > > to the device itself (blocks) is capable of reading the zone > > information? Eg. some monitoring tool. > > With this change, to do a report zones, the process will only need to have read > access to the device. And if it has read access, it also means that it can read > the zones content. Ok, so this is a bit restricting. The zone information is like block device metadata, comparing it to a file that has permissionx 0600 I can see the all the stat info (name, tiemstamps) but can't read the data. But as the ioctl work, it needs a file descriptor and there's probably no way to separate the permissions to read blocks and just the metadata. For a monitoring/reporting tool this would be useful. Eg. for btrfs it could be part of filesystem status overview regarding full or near-full zones and emitting an early warning or poking some service to start the reclaim. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] blk-zoned: allow BLKREPORTZONE without CAP_SYS_ADMIN 2021-06-03 10:04 ` David Sterba @ 2021-06-03 11:20 ` Damien Le Moal 2021-06-03 11:59 ` Niklas Cassel 2021-06-03 14:44 ` David Sterba 0 siblings, 2 replies; 10+ messages in thread From: Damien Le Moal @ 2021-06-03 11:20 UTC (permalink / raw) To: dsterba; +Cc: Niklas Cassel, Jens Axboe, linux-block, linux-kernel On 2021/06/03 19:07, David Sterba wrote: > On Thu, Jun 03, 2021 at 10:00:08AM +0000, Damien Le Moal wrote: >> On 2021/06/03 18:54, David Sterba wrote: >>> On Mon, May 31, 2021 at 01:54:53PM +0000, Niklas Cassel wrote: >>>> From: Niklas Cassel <niklas.cassel@wdc.com> >>>> >>>> Performing a BLKREPORTZONE operation should be allowed under the same >>>> permissions as read(). (read() does not require CAP_SYS_ADMIN). >>>> >>>> Remove the CAP_SYS_ADMIN requirement, and instead check that the fd was >>>> successfully opened with FMODE_READ. This way BLKREPORTZONE will match >>>> the access control requirement of read(). >>> >>> Does this mean that a process that does not have read nor write access >>> to the device itself (blocks) is capable of reading the zone >>> information? Eg. some monitoring tool. >> >> With this change, to do a report zones, the process will only need to have read >> access to the device. And if it has read access, it also means that it can read >> the zones content. > > Ok, so this is a bit restricting. The zone information is like block > device metadata, comparing it to a file that has permissionx 0600 I can > see the all the stat info (name, tiemstamps) but can't read the data. > > But as the ioctl work, it needs a file descriptor and there's probably > no way to separate the permissions to read blocks and just the metadata. > For a monitoring/reporting tool this would be useful. Eg. for btrfs it > could be part of filesystem status overview regarding full or near-full > zones and emitting an early warning or poking some service to start the > reclaim. You lost me... the change is less restrictive than before because the process does not need SYS_CAP_ADMIN anymore. The block device file open is untouched, no change. So whatever process could open it before, will still be able to do so as is. More processes will be able to do report zones with the change. That is all really that changes, so I do not see what potentially breaks, nor how this may prevent writing some monitoring tool. Whoever can open the block device file has FMODE_READ rights, no ? Am I missing something here ? -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] blk-zoned: allow BLKREPORTZONE without CAP_SYS_ADMIN 2021-06-03 11:20 ` Damien Le Moal @ 2021-06-03 11:59 ` Niklas Cassel 2021-06-03 14:44 ` David Sterba 1 sibling, 0 replies; 10+ messages in thread From: Niklas Cassel @ 2021-06-03 11:59 UTC (permalink / raw) To: Damien Le Moal; +Cc: dsterba, Jens Axboe, linux-block, linux-kernel On Thu, Jun 03, 2021 at 11:20:33AM +0000, Damien Le Moal wrote: > On 2021/06/03 19:07, David Sterba wrote: > > On Thu, Jun 03, 2021 at 10:00:08AM +0000, Damien Le Moal wrote: > >> On 2021/06/03 18:54, David Sterba wrote: > >>> On Mon, May 31, 2021 at 01:54:53PM +0000, Niklas Cassel wrote: > >>>> From: Niklas Cassel <niklas.cassel@wdc.com> > >>>> > >>>> Performing a BLKREPORTZONE operation should be allowed under the same > >>>> permissions as read(). (read() does not require CAP_SYS_ADMIN). > >>>> > >>>> Remove the CAP_SYS_ADMIN requirement, and instead check that the fd was > >>>> successfully opened with FMODE_READ. This way BLKREPORTZONE will match > >>>> the access control requirement of read(). > >>> > >>> Does this mean that a process that does not have read nor write access > >>> to the device itself (blocks) is capable of reading the zone > >>> information? Eg. some monitoring tool. > >> > >> With this change, to do a report zones, the process will only need to have read > >> access to the device. And if it has read access, it also means that it can read > >> the zones content. > > > > Ok, so this is a bit restricting. The zone information is like block > > device metadata, comparing it to a file that has permissionx 0600 I can > > see the all the stat info (name, tiemstamps) but can't read the data. > > > > But as the ioctl work, it needs a file descriptor and there's probably > > no way to separate the permissions to read blocks and just the metadata. > > For a monitoring/reporting tool this would be useful. Eg. for btrfs it > > could be part of filesystem status overview regarding full or near-full > > zones and emitting an early warning or poking some service to start the > > reclaim. > > You lost me... the change is less restrictive than before because the process > does not need SYS_CAP_ADMIN anymore. The block device file open is untouched, no > change. So whatever process could open it before, will still be able to do so as > is. More processes will be able to do report zones with the change. That is all > really that changes, so I do not see what potentially breaks, nor how this may > prevent writing some monitoring tool. Whoever can open the block device file has > FMODE_READ rights, no ? Am I missing something here ? What David is saying is that for e.g. stat(), you can get metadata when you don't even have read permission for a device, since stat() takes a pathname. An ioctl requires you to first do an open(), which will will check permissions, so implementing the same is not really possible for an ioctl like BLKREPORTZONE. However, I think the current ioctl is fine. The amount of data that is transferred from a zoned block device for the zone report is more than the data that is transferred when someone does a stat(), so in one way getting the zone report is more like a read. Doing what David suggests would, as far as I can tell, require another solution than the existing ioctl method, which this patch changes. We can think about his suggestion, but it would need to be addressed is a separate patch series. (If his suggestion is something that we want to pursue.) Kind regards, Niklas ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] blk-zoned: allow BLKREPORTZONE without CAP_SYS_ADMIN 2021-06-03 11:20 ` Damien Le Moal 2021-06-03 11:59 ` Niklas Cassel @ 2021-06-03 14:44 ` David Sterba 1 sibling, 0 replies; 10+ messages in thread From: David Sterba @ 2021-06-03 14:44 UTC (permalink / raw) To: Damien Le Moal; +Cc: Niklas Cassel, Jens Axboe, linux-block, linux-kernel On Thu, Jun 03, 2021 at 11:20:33AM +0000, Damien Le Moal wrote: > On 2021/06/03 19:07, David Sterba wrote: > > On Thu, Jun 03, 2021 at 10:00:08AM +0000, Damien Le Moal wrote: > >> On 2021/06/03 18:54, David Sterba wrote: > >>> On Mon, May 31, 2021 at 01:54:53PM +0000, Niklas Cassel wrote: > >>>> From: Niklas Cassel <niklas.cassel@wdc.com> > >>>> > >>>> Performing a BLKREPORTZONE operation should be allowed under the same > >>>> permissions as read(). (read() does not require CAP_SYS_ADMIN). > >>>> > >>>> Remove the CAP_SYS_ADMIN requirement, and instead check that the fd was > >>>> successfully opened with FMODE_READ. This way BLKREPORTZONE will match > >>>> the access control requirement of read(). > >>> > >>> Does this mean that a process that does not have read nor write access > >>> to the device itself (blocks) is capable of reading the zone > >>> information? Eg. some monitoring tool. > >> > >> With this change, to do a report zones, the process will only need to have read > >> access to the device. And if it has read access, it also means that it can read > >> the zones content. > > > > Ok, so this is a bit restricting. The zone information is like block > > device metadata, comparing it to a file that has permissionx 0600 I can > > see the all the stat info (name, tiemstamps) but can't read the data. > > > > But as the ioctl work, it needs a file descriptor and there's probably > > no way to separate the permissions to read blocks and just the metadata. > > For a monitoring/reporting tool this would be useful. Eg. for btrfs it > > could be part of filesystem status overview regarding full or near-full > > zones and emitting an early warning or poking some service to start the > > reclaim. > > You lost me... the change is less restrictive than before because the process > does not need SYS_CAP_ADMIN anymore. The block device file open is untouched, no > change. So whatever process could open it before, will still be able to do so as > is. More processes will be able to do report zones with the change. That is all > really that changes, so I do not see what potentially breaks, nor how this may > prevent writing some monitoring tool. Whoever can open the block device file has > FMODE_READ rights, no ? Am I missing something here ? I'm not saying the patch is wrong or not doing what it says. What caught my attention was the fact that the admin capabilities is not that different from requiring the read permissions. Which for a block device is not easy to get. Normally block devices have 0660 and group is 'disk', so yeah I can add a monitoring daemon to the group. But that still would allow it to read all block devices. So yeah, the patch is making it easier but from my POV it's not that practical. A more fine grained access control would be needed, or another way how to read just the zone info. Reading the information from eg. sysfs has it's own issues. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] allow blk-zoned ioctls without CAP_SYS_ADMIN 2021-05-31 13:54 [PATCH 0/2] allow blk-zoned ioctls without CAP_SYS_ADMIN Niklas Cassel 2021-05-31 13:54 ` [PATCH 1/2] blk-zoned: allow zone management send operations " Niklas Cassel 2021-05-31 13:54 ` [PATCH 2/2] blk-zoned: allow BLKREPORTZONE " Niklas Cassel @ 2021-05-31 23:34 ` Damien Le Moal 2 siblings, 0 replies; 10+ messages in thread From: Damien Le Moal @ 2021-05-31 23:34 UTC (permalink / raw) To: Niklas Cassel, Jens Axboe; +Cc: linux-block On 2021/05/31 22:54, Niklas Cassel wrote: > From: Niklas Cassel <niklas.cassel@wdc.com> > > Allow the following blk-zoned ioctls: BLKREPORTZONE, BLKRESETZONE, > BLKOPENZONE, BLKCLOSEZONE, and BLKFINISHZONE to be performed without > CAP_SYS_ADMIN. > > These ioctls instead only requires that the corresponding R/W access > control flag to be successfully set on the opened file descriptor. > > (open()/openat() will fail with -EPERM if you try to open a file with > flags that you lack permission for.) > > Niklas Cassel (2): > blk-zoned: allow zone management send operations without CAP_SYS_ADMIN > blk-zoned: allow BLKREPORTZONE without CAP_SYS_ADMIN > > block/blk-zoned.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > Looks good to me. For the series: Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com> Note that it may be good to have a Fixes & Cc:stable tag for both patches, no ? -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-06-03 14:46 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-31 13:54 [PATCH 0/2] allow blk-zoned ioctls without CAP_SYS_ADMIN Niklas Cassel 2021-05-31 13:54 ` [PATCH 1/2] blk-zoned: allow zone management send operations " Niklas Cassel 2021-05-31 13:54 ` [PATCH 2/2] blk-zoned: allow BLKREPORTZONE " Niklas Cassel 2021-06-03 9:51 ` David Sterba 2021-06-03 10:00 ` Damien Le Moal 2021-06-03 10:04 ` David Sterba 2021-06-03 11:20 ` Damien Le Moal 2021-06-03 11:59 ` Niklas Cassel 2021-06-03 14:44 ` David Sterba 2021-05-31 23:34 ` [PATCH 0/2] allow blk-zoned ioctls " Damien Le Moal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.