block: ublk: switch to ioctl command encoding
authorMing Lei <ming.lei@redhat.com>
Tue, 18 Apr 2023 13:18:10 +0000 (21:18 +0800)
committerJens Axboe <axboe@kernel.dk>
Wed, 19 Apr 2023 02:13:30 +0000 (20:13 -0600)
All ublk commands(control, IO) should have taken ioctl command encoding
from the beginning, because ioctl command encoding defines each code
uniquely, so driver can figure out wrong command sent from userspace
easily; 2) it might help security subsystem for audit uring cmd[1].

Unfortunately we didn't do that way, and it could be one lesson for
ublk driver.

So switch to ioctl command encoding now, we still support commands encoded
in old way, but they become legacy definition. Any new command should take
ioctl encoding.

See ublksrv code for switching to ioctl command encoding in [2].

[1] https://lore.kernel.org/io-uring/CAHC9VhSVzujW9LOj5Km80AjU0EfAuukoLrxO6BEfnXeK_s6bAg@mail.gmail.com/
[2] https://github.com/ming1/ubdsrv/commits/ioctl_cmd_encoding

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Ken Kurematsu <k.kurematsu@nskint.co.jp>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Link: https://lore.kernel.org/r/20230418131810.855959-1-ming.lei@redhat.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
drivers/block/Kconfig
drivers/block/ublk_drv.c
include/uapi/linux/ublk_cmd.h

index f79f20430ef7a403a52d513f038316815a98b100..5b9d4aaebb81d48550db8b09cb6a2ce9fe38e6b2 100644 (file)
@@ -385,6 +385,23 @@ config BLK_DEV_UBLK
          can handle batch more effectively, but task_work_add() isn't exported
          for module, so ublk has to be built to kernel.
 
+config BLKDEV_UBLK_LEGACY_OPCODES
+       bool "Support legacy command opcode"
+       depends on BLK_DEV_UBLK
+       default y
+       help
+         ublk driver started to take plain command encoding, which turns out
+         one bad way. The traditional ioctl command opcode encodes more
+         info and basically defines each code uniquely, so opcode conflict
+         is avoided, and driver can handle wrong command easily, meantime it
+         may help security subsystem to audit io_uring command.
+
+         Say Y if your application still uses legacy command opcode.
+
+         Say N if you don't want to support legacy command opcode. It is
+         suggested to enable N if your application(ublk server) switches to
+         ioctl command encoding.
+
 source "drivers/block/rnbd/Kconfig"
 
 endif # BLK_DEV
index 1223fcbfc6c9ccb686a5dcadbc5fe7250264483d..5da5876a444341e85fa43aa4ae22f511e317f426 100644 (file)
@@ -53,7 +53,8 @@
                | UBLK_F_NEED_GET_DATA \
                | UBLK_F_USER_RECOVERY \
                | UBLK_F_USER_RECOVERY_REISSUE \
-               | UBLK_F_UNPRIVILEGED_DEV)
+               | UBLK_F_UNPRIVILEGED_DEV \
+               | UBLK_F_CMD_IOCTL_ENCODE)
 
 /* All UBLK_PARAM_TYPE_* should be included here */
 #define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | \
@@ -1253,6 +1254,19 @@ static void ublk_handle_need_get_data(struct ublk_device *ub, int q_id,
        ublk_queue_cmd(ubq, req);
 }
 
+static inline int ublk_check_cmd_op(u32 cmd_op)
+{
+       u32 ioc_type = _IOC_TYPE(cmd_op);
+
+       if (IS_ENABLED(CONFIG_BLKDEV_UBLK_LEGACY_OPCODES) && ioc_type != 'u')
+               return -EOPNOTSUPP;
+
+       if (ioc_type != 'u' && ioc_type != 0)
+               return -EOPNOTSUPP;
+
+       return 0;
+}
+
 static int ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
 {
        struct ublksrv_io_cmd *ub_cmd = (struct ublksrv_io_cmd *)cmd->cmd;
@@ -1294,10 +1308,14 @@ static int ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
         * iff the driver have set the UBLK_IO_FLAG_NEED_GET_DATA.
         */
        if ((!!(io->flags & UBLK_IO_FLAG_NEED_GET_DATA))
-                       ^ (cmd_op == UBLK_IO_NEED_GET_DATA))
+                       ^ (_IOC_NR(cmd_op) == UBLK_IO_NEED_GET_DATA))
+               goto out;
+
+       ret = ublk_check_cmd_op(cmd_op);
+       if (ret)
                goto out;
 
-       switch (cmd_op) {
+       switch (_IOC_NR(cmd_op)) {
        case UBLK_IO_FETCH_REQ:
                /* UBLK_IO_FETCH_REQ is only allowed before queue is setup */
                if (ublk_queue_ready(ubq)) {
@@ -1743,6 +1761,8 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
        if (!IS_BUILTIN(CONFIG_BLK_DEV_UBLK))
                ub->dev_info.flags |= UBLK_F_URING_CMD_COMP_IN_TASK;
 
+       ub->dev_info.flags |= UBLK_F_CMD_IOCTL_ENCODE;
+
        /* We are not ready to support zero copy */
        ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY;
 
@@ -2099,7 +2119,7 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
                 * know if the specified device is created as unprivileged
                 * mode.
                 */
-               if (cmd->cmd_op != UBLK_CMD_GET_DEV_INFO2)
+               if (_IOC_NR(cmd->cmd_op) != UBLK_CMD_GET_DEV_INFO2)
                        return 0;
        }
 
@@ -2125,7 +2145,7 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
        dev_path[header->dev_path_len] = 0;
 
        ret = -EINVAL;
-       switch (cmd->cmd_op) {
+       switch (_IOC_NR(cmd->cmd_op)) {
        case UBLK_CMD_GET_DEV_INFO:
        case UBLK_CMD_GET_DEV_INFO2:
        case UBLK_CMD_GET_QUEUE_AFFINITY:
@@ -2164,6 +2184,7 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 {
        struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
        struct ublk_device *ub = NULL;
+       u32 cmd_op = cmd->cmd_op;
        int ret = -EINVAL;
 
        if (issue_flags & IO_URING_F_NONBLOCK)
@@ -2174,22 +2195,22 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
        if (!(issue_flags & IO_URING_F_SQE128))
                goto out;
 
-       if (cmd->cmd_op != UBLK_CMD_ADD_DEV) {
+       ret = ublk_check_cmd_op(cmd_op);
+       if (ret)
+               goto out;
+
+       if (_IOC_NR(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;
        }
 
-       if (ret)
-               goto put_dev;
-
-       switch (cmd->cmd_op) {
+       switch (_IOC_NR(cmd_op)) {
        case UBLK_CMD_START_DEV:
                ret = ublk_ctrl_start_dev(ub, cmd);
                break;
index f6238ccc7800c91677ed4e84f3c049e9feac8b1b..640bf687b94a9e235751956cf36d9c1210258d8a 100644 (file)
@@ -8,6 +8,9 @@
 
 /*
  * Admin commands, issued by ublk server, and handled by ublk driver.
+ *
+ * Legacy command definition, don't use in new application, and don't
+ * add new such definition any more
  */
 #define        UBLK_CMD_GET_QUEUE_AFFINITY     0x01
 #define        UBLK_CMD_GET_DEV_INFO   0x02
 #define        UBLK_CMD_END_USER_RECOVERY      0x11
 #define        UBLK_CMD_GET_DEV_INFO2          0x12
 
+/* Any new ctrl command should encode by __IO*() */
+#define UBLK_U_CMD_GET_QUEUE_AFFINITY  \
+       _IOR('u', UBLK_CMD_GET_QUEUE_AFFINITY, struct ublksrv_ctrl_cmd)
+#define UBLK_U_CMD_GET_DEV_INFO                \
+       _IOR('u', UBLK_CMD_GET_DEV_INFO, struct ublksrv_ctrl_cmd)
+#define UBLK_U_CMD_ADD_DEV             \
+       _IOWR('u', UBLK_CMD_ADD_DEV, struct ublksrv_ctrl_cmd)
+#define UBLK_U_CMD_DEL_DEV             \
+       _IOWR('u', UBLK_CMD_DEL_DEV, struct ublksrv_ctrl_cmd)
+#define UBLK_U_CMD_START_DEV           \
+       _IOWR('u', UBLK_CMD_START_DEV, struct ublksrv_ctrl_cmd)
+#define UBLK_U_CMD_STOP_DEV            \
+       _IOWR('u', UBLK_CMD_STOP_DEV, struct ublksrv_ctrl_cmd)
+#define UBLK_U_CMD_SET_PARAMS          \
+       _IOWR('u', UBLK_CMD_SET_PARAMS, struct ublksrv_ctrl_cmd)
+#define UBLK_U_CMD_GET_PARAMS          \
+       _IOR('u', UBLK_CMD_GET_PARAMS, struct ublksrv_ctrl_cmd)
+#define UBLK_U_CMD_START_USER_RECOVERY \
+       _IOWR('u', UBLK_CMD_START_USER_RECOVERY, struct ublksrv_ctrl_cmd)
+#define UBLK_U_CMD_END_USER_RECOVERY   \
+       _IOWR('u', UBLK_CMD_END_USER_RECOVERY, struct ublksrv_ctrl_cmd)
+#define UBLK_U_CMD_GET_DEV_INFO2       \
+       _IOR('u', UBLK_CMD_GET_DEV_INFO2, struct ublksrv_ctrl_cmd)
+
 /*
  * IO commands, issued by ublk server, and handled by ublk driver.
  *
  *      It is only used if ublksrv set UBLK_F_NEED_GET_DATA flag
  *      while starting a ublk device.
  */
+
+/*
+ * Legacy IO command definition, don't use in new application, and don't
+ * add new such definition any more
+ */
 #define        UBLK_IO_FETCH_REQ               0x20
 #define        UBLK_IO_COMMIT_AND_FETCH_REQ    0x21
 #define        UBLK_IO_NEED_GET_DATA   0x22
 
+/* Any new IO command should encode by __IOWR() */
+#define        UBLK_U_IO_FETCH_REQ             \
+       _IOWR('u', UBLK_IO_FETCH_REQ, struct ublksrv_io_cmd)
+#define        UBLK_U_IO_COMMIT_AND_FETCH_REQ  \
+       _IOWR('u', UBLK_IO_COMMIT_AND_FETCH_REQ, struct ublksrv_io_cmd)
+#define        UBLK_U_IO_NEED_GET_DATA         \
+       _IOWR('u', UBLK_IO_NEED_GET_DATA, struct ublksrv_io_cmd)
+
 /* only ABORT means that no re-fetch */
 #define UBLK_IO_RES_OK                 0
 #define UBLK_IO_RES_NEED_GET_DATA      1
  */
 #define UBLK_F_UNPRIVILEGED_DEV        (1UL << 5)
 
+/* use ioctl encoding for uring command */
+#define UBLK_F_CMD_IOCTL_ENCODE        (1UL << 6)
+
 /* device state */
 #define UBLK_S_DEV_DEAD        0
 #define UBLK_S_DEV_LIVE        1