Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

block: ublk: switch to ioctl command encoding

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>

authored by

Ming Lei and committed by
Jens Axboe
2d786e66 26a42b61

+94 -13
+17
drivers/block/Kconfig
··· 385 385 can handle batch more effectively, but task_work_add() isn't exported 386 386 for module, so ublk has to be built to kernel. 387 387 388 + config BLKDEV_UBLK_LEGACY_OPCODES 389 + bool "Support legacy command opcode" 390 + depends on BLK_DEV_UBLK 391 + default y 392 + help 393 + ublk driver started to take plain command encoding, which turns out 394 + one bad way. The traditional ioctl command opcode encodes more 395 + info and basically defines each code uniquely, so opcode conflict 396 + is avoided, and driver can handle wrong command easily, meantime it 397 + may help security subsystem to audit io_uring command. 398 + 399 + Say Y if your application still uses legacy command opcode. 400 + 401 + Say N if you don't want to support legacy command opcode. It is 402 + suggested to enable N if your application(ublk server) switches to 403 + ioctl command encoding. 404 + 388 405 source "drivers/block/rnbd/Kconfig" 389 406 390 407 endif # BLK_DEV
+34 -13
drivers/block/ublk_drv.c
··· 53 53 | UBLK_F_NEED_GET_DATA \ 54 54 | UBLK_F_USER_RECOVERY \ 55 55 | UBLK_F_USER_RECOVERY_REISSUE \ 56 - | UBLK_F_UNPRIVILEGED_DEV) 56 + | UBLK_F_UNPRIVILEGED_DEV \ 57 + | UBLK_F_CMD_IOCTL_ENCODE) 57 58 58 59 /* All UBLK_PARAM_TYPE_* should be included here */ 59 60 #define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | \ ··· 1254 1253 ublk_queue_cmd(ubq, req); 1255 1254 } 1256 1255 1256 + static inline int ublk_check_cmd_op(u32 cmd_op) 1257 + { 1258 + u32 ioc_type = _IOC_TYPE(cmd_op); 1259 + 1260 + if (IS_ENABLED(CONFIG_BLKDEV_UBLK_LEGACY_OPCODES) && ioc_type != 'u') 1261 + return -EOPNOTSUPP; 1262 + 1263 + if (ioc_type != 'u' && ioc_type != 0) 1264 + return -EOPNOTSUPP; 1265 + 1266 + return 0; 1267 + } 1268 + 1257 1269 static int ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags) 1258 1270 { 1259 1271 struct ublksrv_io_cmd *ub_cmd = (struct ublksrv_io_cmd *)cmd->cmd; ··· 1308 1294 * iff the driver have set the UBLK_IO_FLAG_NEED_GET_DATA. 1309 1295 */ 1310 1296 if ((!!(io->flags & UBLK_IO_FLAG_NEED_GET_DATA)) 1311 - ^ (cmd_op == UBLK_IO_NEED_GET_DATA)) 1297 + ^ (_IOC_NR(cmd_op) == UBLK_IO_NEED_GET_DATA)) 1312 1298 goto out; 1313 1299 1314 - switch (cmd_op) { 1300 + ret = ublk_check_cmd_op(cmd_op); 1301 + if (ret) 1302 + goto out; 1303 + 1304 + switch (_IOC_NR(cmd_op)) { 1315 1305 case UBLK_IO_FETCH_REQ: 1316 1306 /* UBLK_IO_FETCH_REQ is only allowed before queue is setup */ 1317 1307 if (ublk_queue_ready(ubq)) { ··· 1761 1743 if (!IS_BUILTIN(CONFIG_BLK_DEV_UBLK)) 1762 1744 ub->dev_info.flags |= UBLK_F_URING_CMD_COMP_IN_TASK; 1763 1745 1746 + ub->dev_info.flags |= UBLK_F_CMD_IOCTL_ENCODE; 1747 + 1764 1748 /* We are not ready to support zero copy */ 1765 1749 ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY; 1766 1750 ··· 2119 2099 * know if the specified device is created as unprivileged 2120 2100 * mode. 2121 2101 */ 2122 - if (cmd->cmd_op != UBLK_CMD_GET_DEV_INFO2) 2102 + if (_IOC_NR(cmd->cmd_op) != UBLK_CMD_GET_DEV_INFO2) 2123 2103 return 0; 2124 2104 } 2125 2105 ··· 2145 2125 dev_path[header->dev_path_len] = 0; 2146 2126 2147 2127 ret = -EINVAL; 2148 - switch (cmd->cmd_op) { 2128 + switch (_IOC_NR(cmd->cmd_op)) { 2149 2129 case UBLK_CMD_GET_DEV_INFO: 2150 2130 case UBLK_CMD_GET_DEV_INFO2: 2151 2131 case UBLK_CMD_GET_QUEUE_AFFINITY: ··· 2184 2164 { 2185 2165 struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; 2186 2166 struct ublk_device *ub = NULL; 2167 + u32 cmd_op = cmd->cmd_op; 2187 2168 int ret = -EINVAL; 2188 2169 2189 2170 if (issue_flags & IO_URING_F_NONBLOCK) ··· 2195 2174 if (!(issue_flags & IO_URING_F_SQE128)) 2196 2175 goto out; 2197 2176 2198 - if (cmd->cmd_op != UBLK_CMD_ADD_DEV) { 2177 + ret = ublk_check_cmd_op(cmd_op); 2178 + if (ret) 2179 + goto out; 2180 + 2181 + if (_IOC_NR(cmd_op) != UBLK_CMD_ADD_DEV) { 2199 2182 ret = -ENODEV; 2200 2183 ub = ublk_get_device_from_id(header->dev_id); 2201 2184 if (!ub) 2202 2185 goto out; 2203 2186 2204 2187 ret = ublk_ctrl_uring_cmd_permission(ub, cmd); 2205 - } else { 2206 - /* ADD_DEV permission check is done in command handler */ 2207 - ret = 0; 2188 + if (ret) 2189 + goto put_dev; 2208 2190 } 2209 2191 2210 - if (ret) 2211 - goto put_dev; 2212 - 2213 - switch (cmd->cmd_op) { 2192 + switch (_IOC_NR(cmd_op)) { 2214 2193 case UBLK_CMD_START_DEV: 2215 2194 ret = ublk_ctrl_start_dev(ub, cmd); 2216 2195 break;
+43
include/uapi/linux/ublk_cmd.h
··· 8 8 9 9 /* 10 10 * Admin commands, issued by ublk server, and handled by ublk driver. 11 + * 12 + * Legacy command definition, don't use in new application, and don't 13 + * add new such definition any more 11 14 */ 12 15 #define UBLK_CMD_GET_QUEUE_AFFINITY 0x01 13 16 #define UBLK_CMD_GET_DEV_INFO 0x02 ··· 23 20 #define UBLK_CMD_START_USER_RECOVERY 0x10 24 21 #define UBLK_CMD_END_USER_RECOVERY 0x11 25 22 #define UBLK_CMD_GET_DEV_INFO2 0x12 23 + 24 + /* Any new ctrl command should encode by __IO*() */ 25 + #define UBLK_U_CMD_GET_QUEUE_AFFINITY \ 26 + _IOR('u', UBLK_CMD_GET_QUEUE_AFFINITY, struct ublksrv_ctrl_cmd) 27 + #define UBLK_U_CMD_GET_DEV_INFO \ 28 + _IOR('u', UBLK_CMD_GET_DEV_INFO, struct ublksrv_ctrl_cmd) 29 + #define UBLK_U_CMD_ADD_DEV \ 30 + _IOWR('u', UBLK_CMD_ADD_DEV, struct ublksrv_ctrl_cmd) 31 + #define UBLK_U_CMD_DEL_DEV \ 32 + _IOWR('u', UBLK_CMD_DEL_DEV, struct ublksrv_ctrl_cmd) 33 + #define UBLK_U_CMD_START_DEV \ 34 + _IOWR('u', UBLK_CMD_START_DEV, struct ublksrv_ctrl_cmd) 35 + #define UBLK_U_CMD_STOP_DEV \ 36 + _IOWR('u', UBLK_CMD_STOP_DEV, struct ublksrv_ctrl_cmd) 37 + #define UBLK_U_CMD_SET_PARAMS \ 38 + _IOWR('u', UBLK_CMD_SET_PARAMS, struct ublksrv_ctrl_cmd) 39 + #define UBLK_U_CMD_GET_PARAMS \ 40 + _IOR('u', UBLK_CMD_GET_PARAMS, struct ublksrv_ctrl_cmd) 41 + #define UBLK_U_CMD_START_USER_RECOVERY \ 42 + _IOWR('u', UBLK_CMD_START_USER_RECOVERY, struct ublksrv_ctrl_cmd) 43 + #define UBLK_U_CMD_END_USER_RECOVERY \ 44 + _IOWR('u', UBLK_CMD_END_USER_RECOVERY, struct ublksrv_ctrl_cmd) 45 + #define UBLK_U_CMD_GET_DEV_INFO2 \ 46 + _IOR('u', UBLK_CMD_GET_DEV_INFO2, struct ublksrv_ctrl_cmd) 26 47 27 48 /* 28 49 * IO commands, issued by ublk server, and handled by ublk driver. ··· 68 41 * It is only used if ublksrv set UBLK_F_NEED_GET_DATA flag 69 42 * while starting a ublk device. 70 43 */ 44 + 45 + /* 46 + * Legacy IO command definition, don't use in new application, and don't 47 + * add new such definition any more 48 + */ 71 49 #define UBLK_IO_FETCH_REQ 0x20 72 50 #define UBLK_IO_COMMIT_AND_FETCH_REQ 0x21 73 51 #define UBLK_IO_NEED_GET_DATA 0x22 52 + 53 + /* Any new IO command should encode by __IOWR() */ 54 + #define UBLK_U_IO_FETCH_REQ \ 55 + _IOWR('u', UBLK_IO_FETCH_REQ, struct ublksrv_io_cmd) 56 + #define UBLK_U_IO_COMMIT_AND_FETCH_REQ \ 57 + _IOWR('u', UBLK_IO_COMMIT_AND_FETCH_REQ, struct ublksrv_io_cmd) 58 + #define UBLK_U_IO_NEED_GET_DATA \ 59 + _IOWR('u', UBLK_IO_NEED_GET_DATA, struct ublksrv_io_cmd) 74 60 75 61 /* only ABORT means that no re-fetch */ 76 62 #define UBLK_IO_RES_OK 0 ··· 141 101 * be accessed and managed by its owner represented by owner_uid/owner_gid. 142 102 */ 143 103 #define UBLK_F_UNPRIVILEGED_DEV (1UL << 5) 104 + 105 + /* use ioctl encoding for uring command */ 106 + #define UBLK_F_CMD_IOCTL_ENCODE (1UL << 6) 144 107 145 108 /* device state */ 146 109 #define UBLK_S_DEV_DEAD 0