Re: [PATCH V4 0/6] ublk_drv: add mechanism for supporting unprivileged ublk device

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3106 bytes --]

On Fri, Jan 06, 2023 at 12:17:05PM +0800, Ming Lei wrote:
> Hello,
> 
> Stefan Hajnoczi suggested un-privileged ublk device[1] for container
> use case.
> 
> So far only administrator can create/control ublk device which is too
> strict and increase system administrator burden, and this patchset
> implements un-privileged ublk device:
> 
> - any user can create ublk device, which can only be controlled &
>   accessed by the owner of the device or administrator
> 
> For using such mechanism, system administrator needs to deploy two
> simple udev rules[2] after running 'make install' in ublksrv.
> 
> Userspace(ublksrv):
> 
> 	https://github.com/ming1/ubdsrv/tree/unprivileged-ublk
>     
> 'ublk add -t $TYPE --un_privileged' is for creating one un-privileged
> ublk device if the user is un-privileged.

Hi Ming,
Sorry for the late reply. Is there anything stopping processes with a
different uid/gid from accessing the unprivileged block device
(/dev/ublkbN)?

The scenario I'm thinking about is:
1. Evil user runs "chmod 666 /dev/ublkbN". They are allowed to do this
   because they are the owner of the block device node.
2. Evil user causes another user's process (e.g. suid) to open the block
   device.
3. Evil user's ublksrv either abuses timing (e.g. never responding or
   responding after an artifical delay) to DoS or returns corrupted data
   to exploit bugs in the victim process.

FUSE has exactly the same issue and I think that's why an unprivileged
FUSE mount cannot be accessed by processes with a different uid/gid.

That extra protection is probably necessary for ublk too.

Stefan

> 
> 
> [1] https://lore.kernel.org/linux-block/YoOr6jBfgVm8GvWg@stefanha-x1.localdomain/
> [2] https://github.com/ming1/ubdsrv/blob/unprivileged-ublk/README.rst#un-privileged-mode
> 
> V4:
> 	- only allow to create unprivileged udev for current user, as
> 	  suggested by Jonathan Corbet
> 	- fix misc bug for handling failure
> 	- add detailed document
> 	- update userspace
> 
> V3:
> 	- don't warn on invalid user input for setting devt parameter, as
> 	  suggested by Ziyang, patch 4/6
> 	- fix one memory corruption issue, patch 6/6
> 
> V2:
> 	- fix "ublk_ctrl_uring_cmd_permission() error: uninitialized symbol 'mask'", reported
> 	by  Dan Carpenter' test robot
> 	- address Ziyang's comment on dealing with nr_privileged_daemon
> 
> 
> 
> Ming Lei (6):
>   ublk_drv: remove nr_aborted_queues from ublk_device
>   ublk_drv: don't probe partitions if the ubq daemon isn't trusted
>   ublk_drv: move ublk_get_device_from_id into ublk_ctrl_uring_cmd
>   ublk_drv: add device parameter UBLK_PARAM_TYPE_DEVT
>   ublk_drv: add module parameter of ublks_max for limiting max allowed
>     ublk dev
>   ublk_drv: add mechanism for supporting unprivileged ublk device
> 
>  Documentation/block/ublk.rst  |  49 ++++-
>  drivers/block/ublk_drv.c      | 341 ++++++++++++++++++++++++----------
>  include/uapi/linux/ublk_cmd.h |  49 ++++-
>  3 files changed, 332 insertions(+), 107 deletions(-)
> 
> -- 
> 2.31.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]