Re: [Devel] [PATCH hci-8.1] block: let container root set the block size of and flush its device

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] block: blkdev_flushbuf() relaxed to ve_capable() with no write or exclusive check
--

commit 4af19ef77a12f7b83b4ce6c7194a3cac87d71388
Author: Eva Kurchatova <eva.kurchatova@virtuozzo.com>

block: let container root set the block size of and flush its device

This patch changes the BLKBSZSET and BLKFLSBUF ioctls to check
CAP_SYS_ADMIN with ve_capable() instead of capable(). Root in a container
can then set the block size of, and flush, a block device it has been
given.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-137668

> diff --git a/block/ioctl.c b/block/ioctl.c
> index a3c53b04e1e34..14047cd8e6116 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -419,7 +419,7 @@ static int blkdev_pr_clear(struct block_device *bdev, blk_mode_t mode,
>  static int blkdev_flushbuf(struct block_device *bdev, unsigned cmd,
>  		unsigned long arg)
>  {
> -	if (!capable(CAP_SYS_ADMIN))
> +	if (!ve_capable(CAP_SYS_ADMIN))
>  		return -EACCES;
>  
>  	mutex_lock(&bdev->bd_holder_lock);
>  	if (bdev->bd_holder_ops && bdev->bd_holder_ops->sync)
>  		bdev->bd_holder_ops->sync(bdev);
>  	else {
>  		mutex_unlock(&bdev->bd_holder_lock);
>  		sync_blockdev(bdev);
>  	}
>  
>  	invalidate_bdev(bdev);

[Severity: Medium]
Can this relaxation be reached through a read-only fd? In
blkdev_common_ioctl() the dispatch is:

    case BLKFLSBUF:
            return blkdev_flushbuf(bdev, cmd, arg);

No mode is passed down, so blkdev_flushbuf() has no FMODE_WRITE check and
takes no exclusive claim. After the ve_capable() test it goes straight to
bd_holder_ops->sync(bdev) or sync_blockdev(bdev), and then
invalidate_bdev(bdev).

With CONFIG_VE, ve_capable() evaluates ns_capable() against the container's
user namespace. Root in the container could therefore open a device that the
device cgroup allows read-only, including one held by a filesystem mounted
outside the container. It could then force writeback, invoke the holder's
->sync() op, and drop the clean page cache of that device.

The commit message says "a flush discards only cached pages of that device,
so nothing a container could not already write to is affected". Is that
accurate when the container only has read access through the device cgroup?
That statement also doesn't cover the holder ->sync() side effect.

Unlike BLKBSZSET, where set_blocksize() still requires an exclusive open,
nothing compensates for the relaxation here. Would it make sense to require
FMODE_WRITE (passing mode into blkdev_flushbuf()) when the capability is
only held in the container's user namespace?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261021175410.2339797-1-eva.kurchatova@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel