Re: [Qemu-devel] [PATCH RFC] tcmu: Introduce qemu-tcmu

[Fam Zheng <famz@redhat.com>]

On Thu, 10/20 15:08, Stefan Hajnoczi wrote:
> On Wed, Oct 19, 2016 at 06:08:28PM +0800, Fam Zheng wrote:
> > libtcmu is a Linux library for userspace programs to handle TCMU
> > protocol, which is a SCSI transport between the target_core_user.ko and
> > a userspace backend implementation. The former can be seen as a kernel
> > SCSI Low-Level-Driver that forwards scsi requests to userspace via a
> > UIO ring buffer. By linking to libtcmu, a program can serve as a scsi
> > HBA.
> > 
> > This patch adds qemu-tcmu utility that serves as a LIO userspace
> > backend.  Apart from how it interacts with the rest of the world, the
> > role of qemu-tcmu is much alike to qemu-nbd: it can export any
> > format/protocol that QEMU supports (in iscsi/loopback instead of NBD),
> > for local or remote access. The main advantage with qemu-tcmu lies in
> > the more flexible command protocol (SCSI) and more flexible iscsi or
> > loopback frontends, the latter of which can theoretically allow zero-copy
> > I/O from local machine, which makes qemu-tcmu potentially possible to
> > serve data in better performance.
> > 
> > RFC because only minimal scsi commands are now handled. The plan is to
> > refactor and reuse scsi-disk emulation code. Also there is no test code.
> > 
> > Similar to NBD, there will be QMP commands to start built-in targets so
> > that guest images can be exported as well.
> > 
> > Usage example script (tested on Fedora 24):
> > 
> >     set -e
> > 
> >     # For targetcli integration
> >     sudo systemctl start tcmu-runner
> > 
> >     qemu-img create test-img 1G
> > 
> >     # libtcmu needs to open UIO, give it access
> >     sudo chmod 777 /dev/uio0
> 
> What are the security implications of tcmu?
> 
> If a corrupt image is able to execute arbitrary code in the qemu-tcmu
> process, does /dev/uio0 or the tcmu shared memory interface allow get
> root or kernel privileges?

I haven't audited the code, but target_core_user.ko should contain the access to
/dev/uioX and make sure there is no security risk regarding buggy or malicious
handlers. Otherwise it's a bug that should be fixed. Andy can correct me if I'm
wrong.

> 
> > 
> >     qemu-tcmu test-img &
> > 
> >     sleep 1
> > 
> >     IQN=iqn.2003-01.org.linux-iscsi.lemon.x8664:sn.4939fc29108f
> > 
> >     # Check that we have initialized correctly
> >     sudo targetcli ls | grep -q user:qemu
> > 
> >     # Create iscsi target
> >     sudo targetcli ls | grep -q $IQN || sudo targetcli /iscsi create wwn=$IQN
> >     sudo targetcli /iscsi/$IQN/tpg1 set attribute \
> >         authentication=0 generate_node_acls=1 demo_mode_write_protect=0 \
> >         prod_mode_write_protect=0
> > 
> >     # Create the qemu-tcmu target
> >     sudo targetcli ls | grep -q d0 || \
> >         sudo targetcli /backstores/user:qemu create d0 1G @drive
> > 
> >     # Export it as an iscsi LUN
> >     sudo targetcli ls | grep -q lun0 || \
> >         sudo targetcli /iscsi/$IQN/tpg1/luns create storage_object=/backstores/user:qemu/d0
> > 
> >     # Inspect the nodes again
> >     sudo targetcli ls
> > 
> >     # Test that the LIO export works
> >     qemu-img info iscsi://127.0.0.1/$IQN/0
> >     qemu-io iscsi://127.0.0.1/$IQN/0 \
> >         -c 'read -P 1 4k 1k' \
> >         -c 'write -P 2 4k 1k' \
> >         -c 'read -P 2 4k 1k' \
> 
> Users probably want either:
> 
> 1. Expose disk image as a loopback SCSI device (/dev/sdb) so that qcow2,
>    vmdk, etc files can be accessed from the host.
> 
> 2. Expose disk image as over iSCSI for access from other applications or
>    machines.
> 
> It would be nice to offer user-friendly commands for doing this.
> Manually setting up targetcli and qemu-tcmu looks clunky for ad-hoc
> users.  If you only need this feature every once in a while then it's a
> pain to look up and run these commands manually.

Yes, that can be done with a separate helper script in scripts/.

> > +#define TCMU_DEBUG 1
> 
> Unused

Will remove together when switching to tracing.

> 
> > +
> > +#define DPRINTF(...) do { \
> > +    printf("[%s:%04d] ", __FILE__, __LINE__); \
> > +    printf(__VA_ARGS__); \
> > +} while (0)
> 
> Please use tracing instead.  The default tracing backend ("log") prints
> to stderr and is therefore very easy to use.

Yes, will use in a formal version.

> 
> > +
> > +typedef struct TCMUExport TCMUExport;
> > +
> > +struct TCMUExport {
> > +    BlockBackend *blk;
> > +    struct tcmu_device *tcmu_dev;
> > +    bool writable;
> > +    QLIST_ENTRY(TCMUExport) next;
> > +};
> > +
> > +typedef struct {
> > +    struct tcmulib_context *tcmulib_ctx;
> > +} TCMUHandlerState;
> > +
> > +static QLIST_HEAD(, TCMUExport) tcmu_exports =
> > +    QLIST_HEAD_INITIALIZER(tcmu_exports);
> > +
> > +static TCMUHandlerState *handler_state;
> > +
> > +#define ASCQ_INVALID_FIELD_IN_CDB 0x2400
> 
> Should this go into a scsi header?

Yes, this is ad-hoc, will extract things rom hw/scsi/.

> 
> > +
> > +typedef struct {
> > +    struct tcmulib_cmd *cmd;
> > +    TCMUExport *exp;
> > +    QEMUIOVector *qiov;
> 
> Where is this freed?

Should free it before this struct, will fix.

Thanks for taking a look!

Fam