| Message ID | 20221116060835.159945-7-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ublk_drv: add mechanism for supporting unprivileged ublk device | expand |
On Wed, Nov 16, 2022 at 02:08:35PM +0800, Ming Lei wrote: > unprivileged ublk device is helpful for container use case, such > as: ublk device created in one unprivileged container can be controlled > and accessed by this container only. BTW, this patch may cause kernel panic after allocating one un-privileged device for a while, and one security hole, follows the fixed version: From 0edcd7d7513cab2982b37d8e9a7224762f42e262 Mon Sep 17 00:00:00 2001 From: Ming Lei <ming.lei@redhat.com> Date: Mon, 14 Nov 2022 07:50:41 +0000 Subject: [PATCH fixed] ublk_drv: add mechanism for supporting unprivileged ublk device unprivileged ublk device is helpful for container use case, such as: ublk device created in one unprivileged container can be controlled and accessed by this container only. Implement this feature by adding flag of UBLK_F_UNPRIVILEGED_DEV, and if this flag isn't set, any control command has been run from privileged user. Otherwise, any control command can be sent from any unprivileged user, but the user has to be permitted to access the ublk char device to be controlled. In case of UBLK_F_UNPRIVILEGED_DEV: 1) for command UBLK_CMD_ADD_DEV, it is always allowed, and user needs to provide owner's uid/gid in this command, so that udev can set correct ownership for the created ublk device, since the device owner uid/gid can be queried via command of UBLK_CMD_GET_DEV_INFO. 2) for other control commands, they can only be run successfully if the current user is allowed to access the specified ublk char device, for running the permission check, path of the ublk char device has to be provided by these commands. Also add one control of command UBLK_CMD_GET_DEV_INFO2 which always include the char dev path in payload since userspace may not have knowledge if this device is created in unprivileged mode. For applying this mechanism, system administrator needs to take the following policies: 1) chmod 0666 /dev/ublk-control 2) change ownership of ublkcN & ublkbN - chown owner_uid:owner_gid /dev/ublkcN - chown owner_uid:owner_gid /dev/ublkbN Both can be done via one simple udev rule. Userspace: https://github.com/ming1/ubdsrv/tree/unprivileged-ublk 'ublk add -t $TYPE --un_privileged' is for creating one un-privileged ublk device. Link: https://lore.kernel.org/linux-block/YoOr6jBfgVm8GvWg@stefanha-x1.localdomain/ Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- Documentation/block/ublk.rst | 18 ++--- drivers/block/ublk_drv.c | 147 ++++++++++++++++++++++++++++++++-- include/uapi/linux/ublk_cmd.h | 36 ++++++++- 3 files changed, 184 insertions(+), 17 deletions(-) diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst index ba45c46cc0da..403ffd0f4511 100644 --- a/Documentation/block/ublk.rst +++ b/Documentation/block/ublk.rst @@ -180,6 +180,15 @@ managing and controlling ublk devices with help of several control commands: double-write since the driver may issue the same I/O request twice. It might be useful to a read-only FS or a VM backend. +Unprivileged ublk device is supported by passing UBLK_F_UNPRIVILEGED_DEV. +Once the flag is set, all control commands can be sent by unprivileged +user. Except for command of ``UBLK_CMD_ADD_DEV``, permission check on +the specified char device(``/dev/ublkc*``) is done for all other control +commands by ublk driver, for doing that, path of the char device has to +be provided in these commands' payload from ublk server. With this way, +ublk device becomes container-ware, and device created in one container +can be controlled/accessed just inside this container. + Data plane ---------- @@ -254,15 +263,6 @@ with specified IO tag in the command data: Future development ================== -Container-aware ublk deivice ----------------------------- - -ublk driver doesn't handle any IO logic. Its function is well defined -for now and very limited userspace interfaces are needed, which is also -well defined too. It is possible to make ublk devices container-aware block -devices in future as Stefan Hajnoczi suggested [#stefan]_, by removing -ADMIN privilege. - Zero copy --------- diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 28e9f1a19c9e..69049172b036 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -42,6 +42,7 @@ #include <linux/mm.h> #include <asm/page.h> #include <linux/task_work.h> +#include <linux/namei.h> #include <uapi/linux/ublk_cmd.h> #define UBLK_MINORS (1U << MINORBITS) @@ -51,7 +52,8 @@ | UBLK_F_URING_CMD_COMP_IN_TASK \ | UBLK_F_NEED_GET_DATA \ | UBLK_F_USER_RECOVERY \ - | UBLK_F_USER_RECOVERY_REISSUE) + | UBLK_F_USER_RECOVERY_REISSUE \ + | UBLK_F_UNPRIVILEGED_DEV) /* All UBLK_PARAM_TYPE_* should be included here */ #define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | \ @@ -1651,15 +1653,33 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) __func__, header->queue_id); return -EINVAL; } + if (copy_from_user(&info, argp, sizeof(info))) return -EFAULT; - ublk_dump_dev_info(&info); + + if (info.flags & UBLK_F_UNPRIVILEGED_DEV) { + /* + * user needs to provide proper owner_uid/owner_gid, + * which will be provided to udev rule for setting + * ownership on the ublk device to be created. + */ + ; + } else { + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + + if (info.owner_uid != 0 || info.owner_gid != 0) + return -EINVAL; + } + if (header->dev_id != info.dev_id) { pr_warn("%s: dev id not match %u %u\n", __func__, header->dev_id, info.dev_id); return -EINVAL; } + ublk_dump_dev_info(&info); + ret = mutex_lock_killable(&ublk_ctl_mutex); if (ret) return ret; @@ -1991,6 +2011,114 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub, return ret; } +/* + * All control commands are sent via /dev/ublk-control, so we have to check + * the destination device's permission + */ +static int ublk_char_dev_permission(struct ublk_device *ub, + const char *dev_path, int mask) +{ + int err; + struct path path; + struct kstat stat; + + err = kern_path(dev_path, LOOKUP_FOLLOW, &path); + if (err) + return err; + + err = vfs_getattr(&path, &stat, STATX_TYPE, AT_STATX_SYNC_AS_STAT); + if (err) + goto exit; + + err = -EINVAL; + if (stat.rdev != ub->cdev_dev.devt || !S_ISCHR(stat.mode)) + goto exit; + + err = inode_permission(&init_user_ns, + d_backing_inode(path.dentry), mask); +exit: + path_put(&path); + return err; +} + +static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub, + struct io_uring_cmd *cmd) +{ + struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; + bool unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV; + void __user *argp = (void __user *)(unsigned long)header->addr; + char *dev_path = NULL; + int ret = 0; + int mask; + + if (!unprivileged) { + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + /* + * The new added command of UBLK_CMD_GET_DEV_INFO2 includes + * char_dev_path in payload too, since userspace may not + * know if the specified device is created as unprivileged + * mode. + */ + if (cmd->cmd_op != UBLK_CMD_GET_DEV_INFO2) + return 0; + } + + /* + * User has to provide the char device path for unprivileged ublk + * + * header->addr always points to the dev path buffer, and + * header->dev_path_len records length of dev path buffer. + */ + if (!header->dev_path_len || header->dev_path_len > PATH_MAX) + return -EINVAL; + + if (header->len < header->dev_path_len) + return -EINVAL; + + dev_path = kmalloc(header->dev_path_len + 1, GFP_KERNEL); + if (!dev_path) + return -ENOMEM; + + ret = -EFAULT; + if (copy_from_user(dev_path, argp, header->dev_path_len)) + goto exit; + dev_path[header->dev_path_len] = 0; + + switch (cmd->cmd_op) { + case UBLK_CMD_GET_DEV_INFO: + case UBLK_CMD_GET_DEV_INFO2: + case UBLK_CMD_GET_QUEUE_AFFINITY: + case UBLK_CMD_GET_PARAMS: + mask = MAY_READ; + break; + case UBLK_CMD_START_DEV: + case UBLK_CMD_STOP_DEV: + case UBLK_CMD_ADD_DEV: + case UBLK_CMD_DEL_DEV: + case UBLK_CMD_SET_PARAMS: + case UBLK_CMD_START_USER_RECOVERY: + case UBLK_CMD_END_USER_RECOVERY: + mask = MAY_READ | MAY_WRITE; + break; + default: + break; + } + + ret = ublk_char_dev_permission(ub, dev_path, mask); + if (!ret) { + header->len -= header->dev_path_len; + header->addr += header->dev_path_len; + } + pr_devel("%s: dev id %d cmd_op %x uid %d gid %d path %s ret %d\n", + __func__, ub->ub_number, cmd->cmd_op, + ub->dev_info.owner_uid, ub->dev_info.owner_gid, + dev_path, ret); +exit: + kfree(dev_path); + return ret; +} + static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags) { @@ -2003,17 +2131,21 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, if (!(issue_flags & IO_URING_F_SQE128)) goto out; - ret = -EPERM; - if (!capable(CAP_SYS_ADMIN)) - goto out; - if (cmd->cmd_op != UBLK_CMD_ADD_DEV) { ret = -ENODEV; ub = ublk_get_device_from_id(header->dev_id); if (!ub) goto out; + + ret = ublk_ctrl_uring_cmd_permission(ub, cmd); + } else { + /* ADD_DEV permission check is done in command handler */ + ret = 0; } + if (ret) + goto put_dev; + switch (cmd->cmd_op) { case UBLK_CMD_START_DEV: ret = ublk_ctrl_start_dev(ub, cmd); @@ -2022,6 +2154,7 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, ret = ublk_ctrl_stop_dev(ub); break; case UBLK_CMD_GET_DEV_INFO: + case UBLK_CMD_GET_DEV_INFO2: ret = ublk_ctrl_get_dev_info(ub, cmd); break; case UBLK_CMD_ADD_DEV: @@ -2049,6 +2182,8 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, ret = -ENOTSUPP; break; } + + put_dev: if (ub) ublk_put_device(ub); out: diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h index 4e38b9aa0293..ae80bfef3b9f 100644 --- a/include/uapi/linux/ublk_cmd.h +++ b/include/uapi/linux/ublk_cmd.h @@ -19,6 +19,8 @@ #define UBLK_CMD_GET_PARAMS 0x09 #define UBLK_CMD_START_USER_RECOVERY 0x10 #define UBLK_CMD_END_USER_RECOVERY 0x11 +#define UBLK_CMD_GET_DEV_INFO2 0x12 + /* * IO commands, issued by ublk server, and handled by ublk driver. * @@ -79,6 +81,27 @@ #define UBLK_F_USER_RECOVERY_REISSUE (1UL << 4) +/* + * Unprivileged user can create /dev/ublkcN and /dev/ublkbN. + * + * /dev/ublk-control needs to be available for unprivileged user, and it + * can be done via udev rule to make all control commands available to + * unprivileged user. Except for the command of UBLK_CMD_ADD_DEV, all + * other commands are only allowed for the owner of the specified device. + * + * When userspace sends UBLK_CMD_ADD_DEV, the device pair's owner_uid and + * owner_gid need to be provided via ublksrv_ctrl_dev_info, and the two + * ids need to match with ublk server's uid/gid, otherwise the created + * ublk device can't be started successfully. + * + * We still need udev rule to set correct OWNER/GROUP with the stored + * owner_uid and owner_gid. + * + * Then ublk server can be run as unprivileged user, and /dev/ublkbN can + * be accessed by user of owner_uid/owner_gid. + */ +#define UBLK_F_UNPRIVILEGED_DEV (1UL << 5) + /* device state */ #define UBLK_S_DEV_DEAD 0 #define UBLK_S_DEV_LIVE 1 @@ -98,7 +121,15 @@ struct ublksrv_ctrl_cmd { __u64 addr; /* inline data */ - __u64 data[2]; + __u64 data[1]; + + /* + * Used for UBLK_F_UNPRIVILEGED_DEV and UBLK_CMD_GET_DEV_INFO2 + * only, include null char + */ + __u16 dev_path_len; + __u16 pad; + __u32 reserved; }; struct ublksrv_ctrl_dev_info { @@ -118,7 +149,8 @@ struct ublksrv_ctrl_dev_info { /* For ublksrv internal use, invisible to ublk driver */ __u64 ublksrv_flags; - __u64 reserved0; + __u32 owner_uid; + __u32 owner_gid; __u64 reserved1; __u64 reserved2; };
Hi Ming,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ming-Lei/ublk_drv-add-mechanism-for-supporting-unprivileged-ublk-device/20221116-141131
patch link: https://lore.kernel.org/r/20221116060835.159945-7-ming.lei%40redhat.com
patch subject: [PATCH 6/6] ublk_drv: add mechanism for supporting unprivileged ublk device
config: x86_64-randconfig-m001-20221121
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
smatch warnings:
drivers/block/ublk_drv.c:2107 ublk_ctrl_uring_cmd_permission() error: uninitialized symbol 'mask'.
vim +/mask +2107 drivers/block/ublk_drv.c
a922b5da71a7776 Ming Lei 2022-11-16 2043 static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
a922b5da71a7776 Ming Lei 2022-11-16 2044 struct io_uring_cmd *cmd)
a922b5da71a7776 Ming Lei 2022-11-16 2045 {
a922b5da71a7776 Ming Lei 2022-11-16 2046 struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
a922b5da71a7776 Ming Lei 2022-11-16 2047 bool unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV;
a922b5da71a7776 Ming Lei 2022-11-16 2048 void __user *argp = (void __user *)(unsigned long)header->addr;
a922b5da71a7776 Ming Lei 2022-11-16 2049 char *dev_path = NULL;
a922b5da71a7776 Ming Lei 2022-11-16 2050 int ret = 0;
a922b5da71a7776 Ming Lei 2022-11-16 2051 int mask;
a922b5da71a7776 Ming Lei 2022-11-16 2052
a922b5da71a7776 Ming Lei 2022-11-16 2053 if (!unprivileged) {
a922b5da71a7776 Ming Lei 2022-11-16 2054 if (!capable(CAP_SYS_ADMIN))
a922b5da71a7776 Ming Lei 2022-11-16 2055 return -EPERM;
a922b5da71a7776 Ming Lei 2022-11-16 2056 /*
a922b5da71a7776 Ming Lei 2022-11-16 2057 * The new added command of UBLK_CMD_GET_DEV_INFO2 includes
a922b5da71a7776 Ming Lei 2022-11-16 2058 * char_dev_path in payload too, since userspace may not
a922b5da71a7776 Ming Lei 2022-11-16 2059 * know if the specified device is created as unprivileged
a922b5da71a7776 Ming Lei 2022-11-16 2060 * mode.
a922b5da71a7776 Ming Lei 2022-11-16 2061 */
a922b5da71a7776 Ming Lei 2022-11-16 2062 if (cmd->cmd_op != UBLK_CMD_GET_DEV_INFO2)
a922b5da71a7776 Ming Lei 2022-11-16 2063 return 0;
a922b5da71a7776 Ming Lei 2022-11-16 2064 }
a922b5da71a7776 Ming Lei 2022-11-16 2065
a922b5da71a7776 Ming Lei 2022-11-16 2066 /*
a922b5da71a7776 Ming Lei 2022-11-16 2067 * User has to provide the char device path for unprivileged ublk
a922b5da71a7776 Ming Lei 2022-11-16 2068 *
a922b5da71a7776 Ming Lei 2022-11-16 2069 * header->addr always points to the dev path buffer, and
a922b5da71a7776 Ming Lei 2022-11-16 2070 * header->dev_path_len records length of dev path buffer.
a922b5da71a7776 Ming Lei 2022-11-16 2071 */
a922b5da71a7776 Ming Lei 2022-11-16 2072 if (!header->dev_path_len || header->dev_path_len > PATH_MAX)
a922b5da71a7776 Ming Lei 2022-11-16 2073 return -EINVAL;
a922b5da71a7776 Ming Lei 2022-11-16 2074
a922b5da71a7776 Ming Lei 2022-11-16 2075 if (header->len < header->dev_path_len)
a922b5da71a7776 Ming Lei 2022-11-16 2076 return -EINVAL;
a922b5da71a7776 Ming Lei 2022-11-16 2077
a922b5da71a7776 Ming Lei 2022-11-16 2078 dev_path = kmalloc(header->dev_path_len, GFP_KERNEL);
a922b5da71a7776 Ming Lei 2022-11-16 2079 if (!dev_path)
a922b5da71a7776 Ming Lei 2022-11-16 2080 return -ENOMEM;
a922b5da71a7776 Ming Lei 2022-11-16 2081
a922b5da71a7776 Ming Lei 2022-11-16 2082 ret = -EFAULT;
a922b5da71a7776 Ming Lei 2022-11-16 2083 if (copy_from_user(dev_path, argp, header->dev_path_len))
a922b5da71a7776 Ming Lei 2022-11-16 2084 goto exit;
a922b5da71a7776 Ming Lei 2022-11-16 2085 dev_path[header->dev_path_len] = 0;
a922b5da71a7776 Ming Lei 2022-11-16 2086
a922b5da71a7776 Ming Lei 2022-11-16 2087 switch (cmd->cmd_op) {
a922b5da71a7776 Ming Lei 2022-11-16 2088 case UBLK_CMD_GET_DEV_INFO:
a922b5da71a7776 Ming Lei 2022-11-16 2089 case UBLK_CMD_GET_DEV_INFO2:
a922b5da71a7776 Ming Lei 2022-11-16 2090 case UBLK_CMD_GET_QUEUE_AFFINITY:
a922b5da71a7776 Ming Lei 2022-11-16 2091 case UBLK_CMD_GET_PARAMS:
a922b5da71a7776 Ming Lei 2022-11-16 2092 mask = MAY_READ;
a922b5da71a7776 Ming Lei 2022-11-16 2093 break;
a922b5da71a7776 Ming Lei 2022-11-16 2094 case UBLK_CMD_START_DEV:
a922b5da71a7776 Ming Lei 2022-11-16 2095 case UBLK_CMD_STOP_DEV:
a922b5da71a7776 Ming Lei 2022-11-16 2096 case UBLK_CMD_ADD_DEV:
a922b5da71a7776 Ming Lei 2022-11-16 2097 case UBLK_CMD_DEL_DEV:
a922b5da71a7776 Ming Lei 2022-11-16 2098 case UBLK_CMD_SET_PARAMS:
a922b5da71a7776 Ming Lei 2022-11-16 2099 case UBLK_CMD_START_USER_RECOVERY:
a922b5da71a7776 Ming Lei 2022-11-16 2100 case UBLK_CMD_END_USER_RECOVERY:
a922b5da71a7776 Ming Lei 2022-11-16 2101 mask = MAY_READ | MAY_WRITE;
a922b5da71a7776 Ming Lei 2022-11-16 2102 break;
a922b5da71a7776 Ming Lei 2022-11-16 2103 default:
mask not set on default path.
a922b5da71a7776 Ming Lei 2022-11-16 2104 break;
a922b5da71a7776 Ming Lei 2022-11-16 2105 }
a922b5da71a7776 Ming Lei 2022-11-16 2106
a922b5da71a7776 Ming Lei 2022-11-16 @2107 ret = ublk_char_dev_permission(ub, dev_path, mask);
a922b5da71a7776 Ming Lei 2022-11-16 2108 if (!ret) {
a922b5da71a7776 Ming Lei 2022-11-16 2109 header->len -= header->dev_path_len;
a922b5da71a7776 Ming Lei 2022-11-16 2110 header->addr += header->dev_path_len;
a922b5da71a7776 Ming Lei 2022-11-16 2111 }
a922b5da71a7776 Ming Lei 2022-11-16 2112 pr_devel("%s: dev id %d cmd_op %x uid %d gid %d path %s ret %d\n",
a922b5da71a7776 Ming Lei 2022-11-16 2113 __func__, ub->ub_number, cmd->cmd_op,
a922b5da71a7776 Ming Lei 2022-11-16 2114 ub->dev_info.owner_uid, ub->dev_info.owner_gid,
a922b5da71a7776 Ming Lei 2022-11-16 2115 dev_path, ret);
a922b5da71a7776 Ming Lei 2022-11-16 2116 exit:
a922b5da71a7776 Ming Lei 2022-11-16 2117 kfree(dev_path);
a922b5da71a7776 Ming Lei 2022-11-16 2118 return ret;
a922b5da71a7776 Ming Lei 2022-11-16 2119 }
On Tue, Nov 22, 2022 at 10:17:47AM +0300, Dan Carpenter wrote: > Hi Ming, > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Ming-Lei/ublk_drv-add-mechanism-for-supporting-unprivileged-ublk-device/20221116-141131 > patch link: https://lore.kernel.org/r/20221116060835.159945-7-ming.lei%40redhat.com > patch subject: [PATCH 6/6] ublk_drv: add mechanism for supporting unprivileged ublk device > config: x86_64-randconfig-m001-20221121 > compiler: gcc-11 (Debian 11.3.0-8) 11.3.0 > > If you fix the issue, kindly add following tag where applicable > | Reported-by: kernel test robot <lkp@intel.com> > | Reported-by: Dan Carpenter <error27@gmail.com> > > smatch warnings: > drivers/block/ublk_drv.c:2107 ublk_ctrl_uring_cmd_permission() error: uninitialized symbol 'mask'. > > vim +/mask +2107 drivers/block/ublk_drv.c > > a922b5da71a7776 Ming Lei 2022-11-16 2043 static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub, > a922b5da71a7776 Ming Lei 2022-11-16 2044 struct io_uring_cmd *cmd) > a922b5da71a7776 Ming Lei 2022-11-16 2045 { > a922b5da71a7776 Ming Lei 2022-11-16 2046 struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; > a922b5da71a7776 Ming Lei 2022-11-16 2047 bool unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV; > a922b5da71a7776 Ming Lei 2022-11-16 2048 void __user *argp = (void __user *)(unsigned long)header->addr; > a922b5da71a7776 Ming Lei 2022-11-16 2049 char *dev_path = NULL; > a922b5da71a7776 Ming Lei 2022-11-16 2050 int ret = 0; > a922b5da71a7776 Ming Lei 2022-11-16 2051 int mask; > a922b5da71a7776 Ming Lei 2022-11-16 2052 > a922b5da71a7776 Ming Lei 2022-11-16 2053 if (!unprivileged) { > a922b5da71a7776 Ming Lei 2022-11-16 2054 if (!capable(CAP_SYS_ADMIN)) > a922b5da71a7776 Ming Lei 2022-11-16 2055 return -EPERM; > a922b5da71a7776 Ming Lei 2022-11-16 2056 /* > a922b5da71a7776 Ming Lei 2022-11-16 2057 * The new added command of UBLK_CMD_GET_DEV_INFO2 includes > a922b5da71a7776 Ming Lei 2022-11-16 2058 * char_dev_path in payload too, since userspace may not > a922b5da71a7776 Ming Lei 2022-11-16 2059 * know if the specified device is created as unprivileged > a922b5da71a7776 Ming Lei 2022-11-16 2060 * mode. > a922b5da71a7776 Ming Lei 2022-11-16 2061 */ > a922b5da71a7776 Ming Lei 2022-11-16 2062 if (cmd->cmd_op != UBLK_CMD_GET_DEV_INFO2) > a922b5da71a7776 Ming Lei 2022-11-16 2063 return 0; > a922b5da71a7776 Ming Lei 2022-11-16 2064 } > a922b5da71a7776 Ming Lei 2022-11-16 2065 > a922b5da71a7776 Ming Lei 2022-11-16 2066 /* > a922b5da71a7776 Ming Lei 2022-11-16 2067 * User has to provide the char device path for unprivileged ublk > a922b5da71a7776 Ming Lei 2022-11-16 2068 * > a922b5da71a7776 Ming Lei 2022-11-16 2069 * header->addr always points to the dev path buffer, and > a922b5da71a7776 Ming Lei 2022-11-16 2070 * header->dev_path_len records length of dev path buffer. > a922b5da71a7776 Ming Lei 2022-11-16 2071 */ > a922b5da71a7776 Ming Lei 2022-11-16 2072 if (!header->dev_path_len || header->dev_path_len > PATH_MAX) > a922b5da71a7776 Ming Lei 2022-11-16 2073 return -EINVAL; > a922b5da71a7776 Ming Lei 2022-11-16 2074 > a922b5da71a7776 Ming Lei 2022-11-16 2075 if (header->len < header->dev_path_len) > a922b5da71a7776 Ming Lei 2022-11-16 2076 return -EINVAL; > a922b5da71a7776 Ming Lei 2022-11-16 2077 > a922b5da71a7776 Ming Lei 2022-11-16 2078 dev_path = kmalloc(header->dev_path_len, GFP_KERNEL); > a922b5da71a7776 Ming Lei 2022-11-16 2079 if (!dev_path) > a922b5da71a7776 Ming Lei 2022-11-16 2080 return -ENOMEM; > a922b5da71a7776 Ming Lei 2022-11-16 2081 > a922b5da71a7776 Ming Lei 2022-11-16 2082 ret = -EFAULT; > a922b5da71a7776 Ming Lei 2022-11-16 2083 if (copy_from_user(dev_path, argp, header->dev_path_len)) > a922b5da71a7776 Ming Lei 2022-11-16 2084 goto exit; > a922b5da71a7776 Ming Lei 2022-11-16 2085 dev_path[header->dev_path_len] = 0; > a922b5da71a7776 Ming Lei 2022-11-16 2086 > a922b5da71a7776 Ming Lei 2022-11-16 2087 switch (cmd->cmd_op) { > a922b5da71a7776 Ming Lei 2022-11-16 2088 case UBLK_CMD_GET_DEV_INFO: > a922b5da71a7776 Ming Lei 2022-11-16 2089 case UBLK_CMD_GET_DEV_INFO2: > a922b5da71a7776 Ming Lei 2022-11-16 2090 case UBLK_CMD_GET_QUEUE_AFFINITY: > a922b5da71a7776 Ming Lei 2022-11-16 2091 case UBLK_CMD_GET_PARAMS: > a922b5da71a7776 Ming Lei 2022-11-16 2092 mask = MAY_READ; > a922b5da71a7776 Ming Lei 2022-11-16 2093 break; > a922b5da71a7776 Ming Lei 2022-11-16 2094 case UBLK_CMD_START_DEV: > a922b5da71a7776 Ming Lei 2022-11-16 2095 case UBLK_CMD_STOP_DEV: > a922b5da71a7776 Ming Lei 2022-11-16 2096 case UBLK_CMD_ADD_DEV: > a922b5da71a7776 Ming Lei 2022-11-16 2097 case UBLK_CMD_DEL_DEV: > a922b5da71a7776 Ming Lei 2022-11-16 2098 case UBLK_CMD_SET_PARAMS: > a922b5da71a7776 Ming Lei 2022-11-16 2099 case UBLK_CMD_START_USER_RECOVERY: > a922b5da71a7776 Ming Lei 2022-11-16 2100 case UBLK_CMD_END_USER_RECOVERY: > a922b5da71a7776 Ming Lei 2022-11-16 2101 mask = MAY_READ | MAY_WRITE; > a922b5da71a7776 Ming Lei 2022-11-16 2102 break; > a922b5da71a7776 Ming Lei 2022-11-16 2103 default: > > mask not set on default path. Good catch, and it is really one bug, will fix it in V2. Thanks, Ming
diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst index ba45c46cc0da..403ffd0f4511 100644 --- a/Documentation/block/ublk.rst +++ b/Documentation/block/ublk.rst @@ -180,6 +180,15 @@ managing and controlling ublk devices with help of several control commands: double-write since the driver may issue the same I/O request twice. It might be useful to a read-only FS or a VM backend. +Unprivileged ublk device is supported by passing UBLK_F_UNPRIVILEGED_DEV. +Once the flag is set, all control commands can be sent by unprivileged +user. Except for command of ``UBLK_CMD_ADD_DEV``, permission check on +the specified char device(``/dev/ublkc*``) is done for all other control +commands by ublk driver, for doing that, path of the char device has to +be provided in these commands' payload from ublk server. With this way, +ublk device becomes container-ware, and device created in one container +can be controlled/accessed just inside this container. + Data plane ---------- @@ -254,15 +263,6 @@ with specified IO tag in the command data: Future development ================== -Container-aware ublk deivice ----------------------------- - -ublk driver doesn't handle any IO logic. Its function is well defined -for now and very limited userspace interfaces are needed, which is also -well defined too. It is possible to make ublk devices container-aware block -devices in future as Stefan Hajnoczi suggested [#stefan]_, by removing -ADMIN privilege. - Zero copy --------- diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 28e9f1a19c9e..a21b323711d0 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -42,6 +42,7 @@ #include <linux/mm.h> #include <asm/page.h> #include <linux/task_work.h> +#include <linux/namei.h> #include <uapi/linux/ublk_cmd.h> #define UBLK_MINORS (1U << MINORBITS) @@ -51,7 +52,8 @@ | UBLK_F_URING_CMD_COMP_IN_TASK \ | UBLK_F_NEED_GET_DATA \ | UBLK_F_USER_RECOVERY \ - | UBLK_F_USER_RECOVERY_REISSUE) + | UBLK_F_USER_RECOVERY_REISSUE \ + | UBLK_F_UNPRIVILEGED_DEV) /* All UBLK_PARAM_TYPE_* should be included here */ #define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | \ @@ -1651,15 +1653,33 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) __func__, header->queue_id); return -EINVAL; } + if (copy_from_user(&info, argp, sizeof(info))) return -EFAULT; - ublk_dump_dev_info(&info); + + if (info.flags & UBLK_F_UNPRIVILEGED_DEV) { + /* + * user needs to provide proper owner_uid/owner_gid, + * which will be provided to udev rule for setting + * ownership on the ublk device to be created. + */ + ; + } else { + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + + if (info.owner_uid != 0 || info.owner_gid != 0) + return -EINVAL; + } + if (header->dev_id != info.dev_id) { pr_warn("%s: dev id not match %u %u\n", __func__, header->dev_id, info.dev_id); return -EINVAL; } + ublk_dump_dev_info(&info); + ret = mutex_lock_killable(&ublk_ctl_mutex); if (ret) return ret; @@ -1991,6 +2011,113 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub, return ret; } +/* + * All control commands are sent via /dev/ublk-control, so we have to check + * the destination device's permission + */ +static int ublk_char_dev_permission(struct ublk_device *ub, + const char *dev_path, int mask) +{ + int err; + struct path path; + struct kstat stat; + + err = kern_path(dev_path, LOOKUP_FOLLOW, &path); + if (err) + return err; + + err = vfs_getattr(&path, &stat, STATX_TYPE, AT_STATX_SYNC_AS_STAT); + if (err) + goto exit; + + if (stat.rdev != ub->cdev_dev.devt || !S_ISCHR(stat.mode)) + goto exit; + + err = inode_permission(&init_user_ns, + d_backing_inode(path.dentry), mask); +exit: + path_put(&path); + return err; +} + +static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub, + struct io_uring_cmd *cmd) +{ + struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; + bool unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV; + void __user *argp = (void __user *)(unsigned long)header->addr; + char *dev_path = NULL; + int ret = 0; + int mask; + + if (!unprivileged) { + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + /* + * The new added command of UBLK_CMD_GET_DEV_INFO2 includes + * char_dev_path in payload too, since userspace may not + * know if the specified device is created as unprivileged + * mode. + */ + if (cmd->cmd_op != UBLK_CMD_GET_DEV_INFO2) + return 0; + } + + /* + * User has to provide the char device path for unprivileged ublk + * + * header->addr always points to the dev path buffer, and + * header->dev_path_len records length of dev path buffer. + */ + if (!header->dev_path_len || header->dev_path_len > PATH_MAX) + return -EINVAL; + + if (header->len < header->dev_path_len) + return -EINVAL; + + dev_path = kmalloc(header->dev_path_len, GFP_KERNEL); + if (!dev_path) + return -ENOMEM; + + ret = -EFAULT; + if (copy_from_user(dev_path, argp, header->dev_path_len)) + goto exit; + dev_path[header->dev_path_len] = 0; + + switch (cmd->cmd_op) { + case UBLK_CMD_GET_DEV_INFO: + case UBLK_CMD_GET_DEV_INFO2: + case UBLK_CMD_GET_QUEUE_AFFINITY: + case UBLK_CMD_GET_PARAMS: + mask = MAY_READ; + break; + case UBLK_CMD_START_DEV: + case UBLK_CMD_STOP_DEV: + case UBLK_CMD_ADD_DEV: + case UBLK_CMD_DEL_DEV: + case UBLK_CMD_SET_PARAMS: + case UBLK_CMD_START_USER_RECOVERY: + case UBLK_CMD_END_USER_RECOVERY: + mask = MAY_READ | MAY_WRITE; + break; + default: + break; + } + + ret = ublk_char_dev_permission(ub, dev_path, mask); + if (!ret) { + header->len -= header->dev_path_len; + header->addr += header->dev_path_len; + } + pr_devel("%s: dev id %d cmd_op %x uid %d gid %d path %s ret %d\n", + __func__, ub->ub_number, cmd->cmd_op, + ub->dev_info.owner_uid, ub->dev_info.owner_gid, + dev_path, ret); +exit: + kfree(dev_path); + return ret; +} + static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags) { @@ -2003,17 +2130,21 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, if (!(issue_flags & IO_URING_F_SQE128)) goto out; - ret = -EPERM; - if (!capable(CAP_SYS_ADMIN)) - goto out; - if (cmd->cmd_op != UBLK_CMD_ADD_DEV) { ret = -ENODEV; ub = ublk_get_device_from_id(header->dev_id); if (!ub) goto out; + + ret = ublk_ctrl_uring_cmd_permission(ub, cmd); + } else { + /* ADD_DEV permission check is done in command handler */ + ret = 0; } + if (ret) + goto put_dev; + switch (cmd->cmd_op) { case UBLK_CMD_START_DEV: ret = ublk_ctrl_start_dev(ub, cmd); @@ -2022,6 +2153,7 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, ret = ublk_ctrl_stop_dev(ub); break; case UBLK_CMD_GET_DEV_INFO: + case UBLK_CMD_GET_DEV_INFO2: ret = ublk_ctrl_get_dev_info(ub, cmd); break; case UBLK_CMD_ADD_DEV: @@ -2049,6 +2181,8 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, ret = -ENOTSUPP; break; } + + put_dev: if (ub) ublk_put_device(ub); out: diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h index 4e38b9aa0293..ae80bfef3b9f 100644 --- a/include/uapi/linux/ublk_cmd.h +++ b/include/uapi/linux/ublk_cmd.h @@ -19,6 +19,8 @@ #define UBLK_CMD_GET_PARAMS 0x09 #define UBLK_CMD_START_USER_RECOVERY 0x10 #define UBLK_CMD_END_USER_RECOVERY 0x11 +#define UBLK_CMD_GET_DEV_INFO2 0x12 + /* * IO commands, issued by ublk server, and handled by ublk driver. * @@ -79,6 +81,27 @@ #define UBLK_F_USER_RECOVERY_REISSUE (1UL << 4) +/* + * Unprivileged user can create /dev/ublkcN and /dev/ublkbN. + * + * /dev/ublk-control needs to be available for unprivileged user, and it + * can be done via udev rule to make all control commands available to + * unprivileged user. Except for the command of UBLK_CMD_ADD_DEV, all + * other commands are only allowed for the owner of the specified device. + * + * When userspace sends UBLK_CMD_ADD_DEV, the device pair's owner_uid and + * owner_gid need to be provided via ublksrv_ctrl_dev_info, and the two + * ids need to match with ublk server's uid/gid, otherwise the created + * ublk device can't be started successfully. + * + * We still need udev rule to set correct OWNER/GROUP with the stored + * owner_uid and owner_gid. + * + * Then ublk server can be run as unprivileged user, and /dev/ublkbN can + * be accessed by user of owner_uid/owner_gid. + */ +#define UBLK_F_UNPRIVILEGED_DEV (1UL << 5) + /* device state */ #define UBLK_S_DEV_DEAD 0 #define UBLK_S_DEV_LIVE 1 @@ -98,7 +121,15 @@ struct ublksrv_ctrl_cmd { __u64 addr; /* inline data */ - __u64 data[2]; + __u64 data[1]; + + /* + * Used for UBLK_F_UNPRIVILEGED_DEV and UBLK_CMD_GET_DEV_INFO2 + * only, include null char + */ + __u16 dev_path_len; + __u16 pad; + __u32 reserved; }; struct ublksrv_ctrl_dev_info { @@ -118,7 +149,8 @@ struct ublksrv_ctrl_dev_info { /* For ublksrv internal use, invisible to ublk driver */ __u64 ublksrv_flags; - __u64 reserved0; + __u32 owner_uid; + __u32 owner_gid; __u64 reserved1; __u64 reserved2; };
unprivileged ublk device is helpful for container use case, such as: ublk device created in one unprivileged container can be controlled and accessed by this container only. Implement this feature by adding flag of UBLK_F_UNPRIVILEGED_DEV, and if this flag isn't set, any control command has been run from privileged user. Otherwise, any control command can be sent from any unprivileged user, but the user has to be permitted to access the ublk char device to be controlled. In case of UBLK_F_UNPRIVILEGED_DEV: 1) for command UBLK_CMD_ADD_DEV, it is always allowed, and user needs to provide owner's uid/gid in this command, so that udev can set correct ownership for the created ublk device, since the device owner uid/gid can be queried via command of UBLK_CMD_GET_DEV_INFO. 2) for other control commands, they can only be run successfully if the current user is allowed to access the specified ublk char device, for running the permission check, path of the ublk char device has to be provided by these commands. Also add one control of command UBLK_CMD_GET_DEV_INFO2 which always include the char dev path in payload since userspace may not have knowledge if this device is created in unprivileged mode. For applying this mechanism, system administrator needs to take the following policies: 1) chmod 0666 /dev/ublk-control 2) change ownership of ublkcN & ublkbN - chown owner_uid:owner_gid /dev/ublkcN - chown owner_uid:owner_gid /dev/ublkbN Both can be done via one simple udev rule. Userspace: https://github.com/ming1/ubdsrv/tree/unprivileged-ublk 'ublk add -t $TYPE --un_privileged=1' is for creating one un-privileged ublk device if the user is un-privileged. Link: https://lore.kernel.org/linux-block/YoOr6jBfgVm8GvWg@stefanha-x1.localdomain/ Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- Documentation/block/ublk.rst | 18 ++--- drivers/block/ublk_drv.c | 146 ++++++++++++++++++++++++++++++++-- include/uapi/linux/ublk_cmd.h | 36 ++++++++- 3 files changed, 183 insertions(+), 17 deletions(-)