Message-ID: <YoUVb8CeWRIErJBY@stefanha-x1.localdomain>
Date:   Wed, 18 May 2022 16:49:03 +0100
From:   Stefan Hajnoczi <stefanha@...hat.com>
To:     Ming Lei <ming.lei@...hat.com>
Cc:     Jens Axboe <axboe@...nel.dk>, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Harris James R <james.r.harris@...el.com>,
        io-uring@...r.kernel.org,
        Gabriel Krisman Bertazi <krisman@...labora.com>,
        ZiyangZhang <ZiyangZhang@...ux.alibaba.com>,
        Xiaoguang Wang <xiaoguang.wang@...ux.alibaba.com>
Subject: Re: [PATCH V2 0/1] ubd: add io_uring based userspace block driver

On Wed, May 18, 2022 at 08:53:54PM +0800, Ming Lei wrote:
> On Wed, May 18, 2022 at 11:45:32AM +0100, Stefan Hajnoczi wrote:
> > On Wed, May 18, 2022 at 03:09:46PM +0800, Ming Lei wrote:
> > > On Tue, May 17, 2022 at 03:06:34PM +0100, Stefan Hajnoczi wrote:
> > > > Here are some more thoughts on the ubd-control device:
> > > > 
> > > > The current patch provides a ubd-control device for processes with
> > > > suitable permissions (i.e. root) to create, start, stop, and fetch
> > > > information about devices.
> > > > 
> > > > There is no isolation between devices created by one process and those
> > > 
> > > I understand linux hasn't device namespace yet, so can you share the
> > > rational behind the idea of device isolation, is it because ubd device
> > > is served by ubd daemon which belongs to one pid NS? Or the user creating
> > > /dev/ubdbN belongs to one user NS?
> > 
> > With the current model a process with access to ubd-control has control
> > over all ubd devices. This is not desirable for most container use cases
> > because ubd-control usage within a container means that container could
> > stop any ubd device on the system.
> > 
> > Even for non-container use cases it's problematic that two applications
> > that use ubd can interfere with each other. If an application passes the
> > wrong device ID they can stop the other application's device, for
> > example.
> > 
> > I think it's worth supporting a model where there are multiple ubd
> > daemons that are not cooperating/aware of each other. They should be
> > isolated from each other.
> 
> Maybe I didn't mention it clearly, I meant the following model in last email:
> 
> 1) every user can send UBD_CMD_ADD_DEV to /dev/ubd-control
> 
> 2) the created /dev/ubdcN & /dev/udcbN are owned by the user who creates
> it

How does this work? Does userspace (udev) somehow get the uid/gid from
the uevent so it can set the device node permissions?

> 3) only the user who has permission to /dev/ubdcN can send other control
> commands(START_DEV/STOP_DEV/GET_DEV_INFO/GET_QUEUE_AFFINITY/DEL_DEV);
> and same with /dev/ubdbN
> 
> 4) for unprivileged user who owns /dev/ubdbN, limit kernel behavior,
> such as, not probed for partitions and LVM, only allow unprivileged
> mounts,...
> 
> So ubd device can be isolated wrt. user NS.

Cool!

> 
> > 
> > > IMO, ubd device is one file in VFS, and FS permission should be applied,
> > > then here the closest model should be user NS, and process privilege &
> > > file ownership.
> > 
> > Yes, /dev/ubdbN can has file ownership/permissions and the cgroup device
> > controller can restrict access too. That works fine when the device was
> > created previously.
> > 
> > But what about ubd device creation via ubd-control?
> > 
> > The problem is a global control interface like ubd-control gives access
> > to all ubd devices. There is no way to let an application/container
> > control (create/start/stop/etc) some ubd devices but not all. I think
> > ubd-control must be more fine-grained so multiple
> > applications/containers can use it without the possibility of
> > interference.
> > 
> > /dev/ubdcN is a separate problem. The cgroup device controller can limit
> > the device nodes that are accessible from a process. However, this
> > requires reserving device minor number ranges for each
> > application/container so they can only mknod/open their own ubd devices
> > and not devices that don't belong to them. Maybe there is a better
> > solution?
> > 
> > /dev/ubdbN has similar requirements to /dev/ubdcN. It should be possible
> > to create a new /dev/ubdbN but not access an existing device that belong
> > 
> > So if we want to let containers create ubd devices without granting them
> > access to all devices on the system, then the ubd-control interface
> > needs to be changed (see below) and the container needs a reserved range
> > of ubdcN minor numbers. Any container using ubdbN needs the cgroup
> > device controller and file ownership/permissions to open the block
> > device.
> > 
> > > > created by another. Therefore two processes that do not trust each other
> > > > cannot both use UBD without potential interference. There is also no
> > > 
> > > Can you share what the expectation is for this situation?
> > 
> > Two users should be able to run ubd daemons on the same system without
> > being able to stop each other's devices.
> 
> Yeah, the above process privilege & file ownership based way can reach
> the goal in user NS.
> 
> > 
> > > It is the created UBD which can only be used in this NS, or can only be
> > > visible inside this NS? I guess the latter isn't possible since we don't
> > > have this kind of isolation framework yet.
> > 
> > It should be possible to access the ubd device according to file
> > ownership/permissions. No new isolation framework is needed for that.
> > 
> > But ubd-control should not grant global access to all ubd devices, at
> > least not in the typical case of a ubd daemon that just wishes to
> > create/start/stop its own devices.
> 
> Yeah, I agree.
> 
> > 
> > > > isolation for containers.
> > > > 
> > > > I think it would be a mistake to keep the ubd-control interface in its
> > > > current form since the current global/root model is limited. Instead I
> > > > suggest:
> > > > - Creating a device returns a new file descriptor instead of a global
> > > >   dev_id. The device can be started/stopped/configured through this (and
> > > >   only through this) per-device file descriptor. The device is not
> > > >   visible to other processes through ubd-control so interference is not
> > > >   possible. In order to give another process control over the device the
> > > >   fd can be passed (e.g. SCM_RIGHTS). 
> > > > 
> > > 
> > > /dev/ubdcN can only be opened by the process which is the descendant of
> > > the process which creates the device by sending ADD_DEV.
> > > 
> > > But the device can be deleted/queried by other processes, however, I
> > > think it is reasonable if all these processes has permission to do that,
> > > such as all processes owns the device with same uid.
> > 
> > I don't think it's a good idea to require all ubd daemons to have
> > CAP_SYS_ADMIN/same uid. That's the main point I'm trying to make and the
> > discussion is based on that.
> 
> I meant only the user who owns /dev/ubdcN can send the command to
> /dev/ubd-control for controlling /dev/ubdcN. I believe this way is
> straightforward.
> 
> > 
> > > So can we apply process privilege & file ownership for isolating ubd device?
> > > 
> > > If per-process FD is used, it may confuse people, because process can
> > > not delete/query ubd device even though its uid shows it has the
> > > privilege.
> > 
> > Is it better to stop the device via ubd-control instead of a
> > daemon-specific command (or just killing the daemon process)?
> > 
> > Regarding querying the device, the daemon has more information
> > associated with the device (e.g. if it's an iSCSI initiator it will have
> > the iSCSI URI). The ubd driver can only tell you the daemon pid and the
> > block device attributes that should already be available via sysfs.
> > Quering the daemon will yield more useful information than using
> > ubd-control.
> 
> I don't think it is good to interrupt daemon for this admin/control job,
> which may distract daemon from handling normal IO tasks, also not necessary
> to make daemon implementation more complicated.
> 
> We should separate admin task from normal IO handling, which is one
> common design pattern.
> 
> > 
> > > > Now multiple applications/containers/etc can use ubd-control without
> > > > interfering with each other. The security model still requires root
> > > > though since devices can be malicious.
> > > > 
> > > > FUSE allows unprivileged mounts (see fuse_allow_current_process()). Only
> > > > processes with the same uid as the FUSE daemon can access such mounts
> > > > (in the default configuration). This prevents security issues while
> > > > still allowing unprivileged use cases.
> > > 
> > > OK, looks FUSE applies process privilege & file ownership for dealing
> > > with unprivileged mounts.
> > > 
> > > > 
> > > > I suggest adapting the FUSE security model to block devices:
> > > > - Devices can be created without CAP_SYS_ADMIN but they have an
> > > >   'unprivileged' flag set to true.
> > > > - Unprivileged devices are not probed for partitions and LVM doesn't
> > > >   touch them. This means the kernel doesn't access these devices via
> > > >   code paths that might be exploitable.
> > > 
> > > The above two makes sense.
> > > 
> > > > - When another process with a different uid from ubdsrv opens an
> > > >   unprivileged device, -EACCES is returned. This protects other
> > > >   uids from the unprivileged device.
> > > 
> > > OK, only the user who owns the device can access unprivileged device.
> > > 
> > > > - When another process with a different uid from ubdsrv opens a
> > > >   _privileged_ device there is no special access check because ubdsrv is
> > > >   privileged.
> > > 
> > > IMO, it depends if uid of this process has permission to access the
> > > ubd device, and we can set ubd device's owership by the process
> > > credentials.
> > 
> > Yes, file ownership/permissions are still relevant.
> > 
> > > 
> > > > 
> > > > With these changes UBD can be used by unprivileged processes and
> > > > containers. I think it's worth discussing the details and having this
> > > > model from the start so UBD can be used in a wide range of use cases.
> > > 
> > > I am pretty happy to discuss & figure out the details, but not sure
> > > it is one blocker for ubd:
> > > 
> > > 1) kernel driver of loop/nbd or others haven't support the isolation
> > 
> > It may be better to compare it with FUSE where unprivileged users can
> > run their own servers. Imagine FUSE required a global root control
> > interface like ubd-control, then it wouldn't be possible to have
> > unprivileged FUSE mounts.
> > 
> > > 2) still don't know exact ubd use case for containers
> > 
> > There are two common use cases for block devices:
> > 1. File systems or volume managers
> > 2. Direct access for databases, backup tools, disk image tools, etc
> > 
> > The file system use case involved kernel code and probably needs to be
> > restricted to untrusted containers cannot exploit the kernel file system
> > implementations. I'll ignore this use case and containers probably
> > shouldn't do this.
> > 
> > The second use case is when you have any program that can operate on a
> > block device. It could be an application that imports/exports a block
> > device from network storage. This kind of application should be able to
> > do its job without CAP_SYS_ADMIN and it should be able to run in a
> > container. It might be part of KubeVirt's Containerized Data Importer,
> > for example, and is deployed as a container.
> > 
> > If ubd supports unprivileged operation then this container use case is
> > straightforward. If not, then it's problematic because it either
> > requires a privileged container or some kind of privileged helper
> > outside the container. At that point people may avoid ubd because it's
> > too hard to deploy with privilege requirements.
> 
> OK, thanks for the sharing. In short, container requires unprivileged
> operation on block device. I think it makes sense.

Thanks!

Stefan

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists