From nobody Thu Nov 28 00:45:10 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.xenproject.org designates 192.237.175.120 as permitted sender) client-ip=192.237.175.120; envelope-from=xen-devel-bounces@lists.xenproject.org; helo=lists.xenproject.org; Authentication-Results: mx.zohomail.com; spf=pass (zohomail.com: domain of lists.xenproject.org designates 192.237.175.120 as permitted sender) smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Return-Path: Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) by mx.zohomail.com with SMTPS id 1685478758306772.2804677850128; Tue, 30 May 2023 13:32:38 -0700 (PDT) Received: from list by lists.xenproject.org with outflank-mailman.541402.844190 (Exim 4.92) (envelope-from ) id 1q460e-00044H-UZ; Tue, 30 May 2023 20:32:00 +0000 Received: by outflank-mailman (output) from mailman id 541402.844190; Tue, 30 May 2023 20:32:00 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1q460e-000445-Ox; Tue, 30 May 2023 20:32:00 +0000 Received: by outflank-mailman (input) for mailman id 541402; Tue, 30 May 2023 20:31:58 +0000 Received: from se1-gles-sth1-in.inumbo.com ([159.253.27.254] helo=se1-gles-sth1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1q460c-0001yj-Jx for xen-devel@lists.xenproject.org; Tue, 30 May 2023 20:31:58 +0000 Received: from wout5-smtp.messagingengine.com (wout5-smtp.messagingengine.com [64.147.123.21]) by se1-gles-sth1.inumbo.com (Halon) with ESMTPS id 04d32778-ff29-11ed-b231-6b7b168915f2; Tue, 30 May 2023 22:31:56 +0200 (CEST) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.west.internal (Postfix) with ESMTP id AB9E13200920; Tue, 30 May 2023 16:31:54 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute5.internal (MEProxy); Tue, 30 May 2023 16:31:55 -0400 Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 30 May 2023 16:31:53 -0400 (EDT) X-Outflank-Mailman: Message body and most headers restored to incoming version X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: 04d32778-ff29-11ed-b231-6b7b168915f2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= invisiblethingslab.com; h=cc:cc:content-transfer-encoding :content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to; s=fm1; t=1685478714; x=1685565114; bh=G/PZMKR5fM icHWYQT0GxcmUHbUsQVYWUtsZvrUHCJuU=; b=Fx+b8PAh3kQful+76g25YMRpI5 OX2hyNg04HQUYaN44eHLXL/rFm2SALtFR0112VOAhcyu7Q2sJwY1i66PmyE2SnTT GQ8zTaVT5pxBlG08qTCZ+FS3117iMs4IgwF+YZnRQsIjYuCt9Q9EcokEF1/w2QFv Twymq+8VxvVG/76A/z0fBAsNeDIatL11QxQzfwr7rrvR/Yezfnoab4FX2SdDIO8+ 3Z09WdrcZcDTQYfG4wiLLpb2iXwlJVjKAN2ocz+agmZSrrgePipjWehEJVX2cKu6 l8BXqqbyb62oQpkjHAq0YFkp+2SFAkTxICtIqKbr7Q7NzkLJZ8C4XaiyGSzQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1685478714; x= 1685565114; bh=G/PZMKR5fMicHWYQT0GxcmUHbUsQVYWUtsZvrUHCJuU=; b=s iYr85F64Hkrtm1e26WXimQt3qTnsJz10WHrpr4PvU8EdZ5UO6Qfqj67YBBt2BbFO PUipDPJEr6g34YqFLy8Hwa01eC3ltU7TdIquoCIcGBTWPS2otA5rs1uHUbwazjjK Ww7dGsxlG99lt+yj4HNnXShCkklC9RuO2GIiKVVdrv7njlCf5lZcH+cU8FlYF758 DVOQ5dzfsR+WfY5BDpMPF7W7vjmlnSsmy/B0Ll5i1cVddn1sbTTYzNIoUN9Ql6Ib XM5XK8sKDq8W6Feu9Fioasb6HgQXeG3lzMjwIN3YEC91VleFfGumhwAmWhtxq+H3 0DWvG8HfoOsaY0EOEduGg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedrfeekjedgudeglecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefhvfevufffkffojghfggfgsedtkeertdertddtnecuhfhrohhmpeffvghm ihcuofgrrhhivgcuqfgsvghnohhurhcuoeguvghmihesihhnvhhishhisghlvghthhhinh hgshhlrggsrdgtohhmqeenucggtffrrghtthgvrhhnpeejffejgffgueegudevvdejkefg hefghffhffejteekleeufeffteffhfdtudehteenucevlhhushhtvghrufhiiigvpedune curfgrrhgrmhepmhgrihhlfhhrohhmpeguvghmihesihhnvhhishhisghlvghthhhinhhg shhlrggsrdgtohhm X-ME-Proxy: Feedback-ID: iac594737:Fastmail From: Demi Marie Obenour To: Jens Axboe , =?UTF-8?q?Roger=20Pau=20Monn=C3=A9?= , Alasdair Kergon , Mike Snitzer , dm-devel@redhat.com Cc: Demi Marie Obenour , =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= , linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org Subject: [PATCH v2 07/16] device-mapper: Allow userspace to opt-in to strict parameter checks Date: Tue, 30 May 2023 16:31:07 -0400 Message-Id: <20230530203116.2008-8-demi@invisiblethingslab.com> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20230530203116.2008-1-demi@invisiblethingslab.com> References: <20230530203116.2008-1-demi@invisiblethingslab.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-ZM-MESSAGEID: 1685478759367100005 Content-Type: text/plain; charset="utf-8" Currently, device-mapper ioctls ignore unknown flags. This makes adding new flags to a given ioctl risky, as it could potentially break old userspace. To solve this problem, allow userspace to pass 5 as the major version to any ioctl. This causes the kernel to reject any flags that are not supported by the ioctl, as well as nonzero padding and names and UUIDs that are not NUL-terminated. New flags will only be recognized if major version 5 is used. Kernels without this patch return -EINVAL if the major version is 5, so this is backwards compatible. Signed-off-by: Demi Marie Obenour --- drivers/md/dm-ioctl.c | 301 ++++++++++++++++++++++++++-------- include/uapi/linux/dm-ioctl.h | 30 +++- 2 files changed, 260 insertions(+), 71 deletions(-) diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index 20f452b6c61c1c4d20259fd0fc5443977e4454a0..cf752e72ef6a2d8f8230e5bd6d1= a6dc817a4f597 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -64,7 +64,8 @@ struct vers_iter { static struct rb_root name_rb_tree =3D RB_ROOT; static struct rb_root uuid_rb_tree =3D RB_ROOT; =20 -static void dm_hash_remove_all(bool keep_open_devices, bool mark_deferred,= bool only_deferred); +static void dm_hash_remove_all(bool keep_open_devices, bool mark_deferred,= bool only_deferred, + struct dm_ioctl *param); =20 /* * Guards access to both hash tables. @@ -78,7 +79,7 @@ static DEFINE_MUTEX(dm_hash_cells_mutex); =20 static void dm_hash_exit(void) { - dm_hash_remove_all(false, false, false); + dm_hash_remove_all(false, false, false, NULL); } =20 /* @@ -333,7 +334,8 @@ static struct dm_table *__hash_remove(struct hash_cell = *hc) return table; } =20 -static void dm_hash_remove_all(bool keep_open_devices, bool mark_deferred,= bool only_deferred) +static void dm_hash_remove_all(bool keep_open_devices, bool mark_deferred,= bool only_deferred, + struct dm_ioctl *param) { int dev_skipped; struct rb_node *n; @@ -367,6 +369,8 @@ static void dm_hash_remove_all(bool keep_open_devices, = bool mark_deferred, bool dm_table_destroy(t); } dm_ima_measure_on_device_remove(md, true); + if (param !=3D NULL && !dm_kobject_uevent(md, KOBJ_REMOVE, param->event_= nr, false)) + param->flags |=3D DM_UEVENT_GENERATED_FLAG; dm_put(md); if (likely(keep_open_devices)) dm_destroy(md); @@ -513,7 +517,7 @@ static struct mapped_device *dm_hash_rename(struct dm_i= octl *param, =20 void dm_deferred_remove(void) { - dm_hash_remove_all(true, false, true); + dm_hash_remove_all(true, false, true, NULL); } =20 /* @@ -529,7 +533,7 @@ typedef int (*ioctl_fn)(struct file *filp, struct dm_io= ctl *param, size_t param_ =20 static int remove_all(struct file *filp, struct dm_ioctl *param, size_t pa= ram_size) { - dm_hash_remove_all(true, !!(param->flags & DM_DEFERRED_REMOVE), false); + dm_hash_remove_all(true, !!(param->flags & DM_DEFERRED_REMOVE), false, pa= ram); param->data_size =3D 0; return 0; } @@ -892,8 +896,6 @@ static int dev_create(struct file *filp, struct dm_ioct= l *param, size_t param_si return r; } =20 - param->flags &=3D ~DM_INACTIVE_PRESENT_FLAG; - __dev_status(md, param); =20 dm_put(md); @@ -947,8 +949,6 @@ static struct hash_cell *__find_device_hash_cell(struct= dm_ioctl *param) =20 if (hc->new_map) param->flags |=3D DM_INACTIVE_PRESENT_FLAG; - else - param->flags &=3D ~DM_INACTIVE_PRESENT_FLAG; =20 return hc; } @@ -1161,7 +1161,6 @@ static int do_resume(struct dm_ioctl *param) =20 new_map =3D hc->new_map; hc->new_map =3D NULL; - param->flags &=3D ~DM_INACTIVE_PRESENT_FLAG; =20 up_write(&_hash_lock); =20 @@ -1426,6 +1425,32 @@ static int next_target(struct dm_target_spec *last, = uint32_t next, const char *e return 0; } =20 +static inline bool sloppy_checks(const struct dm_ioctl *param) +{ + return param->version[0] < DM_VERSION_MAJOR_STRICT; +} + +static bool no_non_nul_after_nul(const char *untrusted_str, size_t size, + unsigned int cmd, const char *msg) +{ + const char *cursor; + const char *endp =3D untrusted_str + size; + const char *nul_terminator =3D memchr(untrusted_str, '\0', size); + + if (nul_terminator =3D=3D NULL) { + DMERR("%s not NUL-terminated, cmd(%u)", msg, cmd); + return false; + } + for (cursor =3D nul_terminator; cursor < endp; cursor++) { + if (*cursor !=3D 0) { + DMERR("%s has non-NUL byte at %zd after NUL byte at %zd, cmd(%u)", + msg, cursor - untrusted_str, nul_terminator - untrusted_str, cmd); + return false; + } + } + return true; +} + static int populate_table(struct dm_table *table, struct dm_ioctl *param, size_t param_size) { @@ -1436,12 +1461,19 @@ static int populate_table(struct dm_table *table, const char *const end =3D (const char *) param + param_size; char *target_params; size_t min_size =3D sizeof(struct dm_ioctl); + bool const strict =3D !sloppy_checks(param); =20 if (!param->target_count) { DMERR("%s: no targets specified", __func__); return -EINVAL; } =20 + if (strict && param_size % 8 !=3D 0) { + DMERR("%s: parameter size %zu not multiple of 8", + __func__, param_size); + return -EINVAL; + } + for (i =3D 0; i < param->target_count; i++) { const char *nul_terminator; =20 @@ -1466,6 +1498,18 @@ static int populate_table(struct dm_table *table, /* Add 1 for NUL terminator */ min_size =3D (nul_terminator - (const char *)spec) + 1; =20 + if (strict) { + if (!no_non_nul_after_nul(spec->target_type, sizeof(spec->target_type), + DM_TABLE_LOAD_CMD, "target type")) + return -EINVAL; + + if (spec->status) { + DMERR("%s: status in target spec must be zero, not %u", + __func__, spec->status); + return -EINVAL; + } + } + r =3D dm_table_add_target(table, spec->target_type, (sector_t) spec->sector_start, (sector_t) spec->length, @@ -1476,6 +1520,32 @@ static int populate_table(struct dm_table *table, } =20 next =3D spec->next; + + if (strict) { + uint64_t zero =3D 0; + /* + * param_size is a multiple of 8 so this is still in + * bounds (or 1 past the end). + */ + size_t expected_next =3D round_up(min_size, 8); + + if (expected_next !=3D next) { + DMERR("%s: in strict mode, expected next to be %zu but it was %u", + __func__, expected_next, next); + return -EINVAL; + } + + if (memcmp(&zero, nul_terminator, next - min_size + 1) !=3D 0) { + DMERR("%s: in strict mode, padding must be zeroed", __func__); + return -EINVAL; + } + } + } + + if (strict && next !=3D (size_t)(end - (const char *)spec)) { + DMERR("%s: last target size is %u, but %zd bytes remaining in target spe= c", + __func__, next, end - (const char *)spec); + return -EINVAL; } =20 return dm_table_complete(table); @@ -1823,48 +1893,67 @@ static int target_message(struct file *filp, struct= dm_ioctl *param, size_t para * the ioctl. */ #define IOCTL_FLAGS_NO_PARAMS 1 -#define IOCTL_FLAGS_ISSUE_GLOBAL_EVENT 2 +#define IOCTL_FLAGS_TAKES_EVENT_NR 2 +#define IOCTL_FLAGS_ISSUE_GLOBAL_EVENT (IOCTL_FLAGS_TAKES_EVENT_NR | 4) =20 /* *--------------------------------------------------------------- * Implementation of open/close/ioctl on the special char device. *--------------------------------------------------------------- */ -static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags) +static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags, uint32_t = *supported_flags) { static const struct { int cmd; int flags; ioctl_fn fn; + uint32_t supported_flags; } _ioctls[] =3D { - {DM_VERSION_CMD, 0, NULL}, /* version is dealt with elsewhere */ - {DM_REMOVE_ALL_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVE= NT, remove_all}, - {DM_LIST_DEVICES_CMD, 0, list_devices}, + /* Macro to make the structure initializers somewhat readable */ +#define I(cmd, flags, fn, supported_flags) { \ + (cmd), \ + (flags), \ + (fn), \ + /* \ + * Supported flags in sloppy mode must not include anything in DM_STRICT_= ONLY_FLAGS. \ + * Use BUILD_BUG_ON_ZERO to check for that. \ + */ \ + (supported_flags) | BUILD_BUG_ON_ZERO((supported_flags) & DM_STRICT_ONLY_= FLAGS), \ +} + I(DM_VERSION_CMD, 0, NULL, 0), /* version is dealt with elsewhere */ + I(DM_REMOVE_ALL_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EV= ENT, remove_all, + DM_DEFERRED_REMOVE), + I(DM_LIST_DEVICES_CMD, 0, list_devices, DM_UUID_FLAG), + I(DM_DEV_CREATE_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EV= ENT, dev_create, + DM_PERSISTENT_DEV_FLAG), + I(DM_DEV_REMOVE_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EV= ENT, dev_remove, + DM_DEFERRED_REMOVE), + I(DM_DEV_RENAME_CMD, IOCTL_FLAGS_ISSUE_GLOBAL_EVENT, dev_rename, + DM_QUERY_INACTIVE_TABLE_FLAG | DM_UUID_FLAG), + I(DM_DEV_SUSPEND_CMD, IOCTL_FLAGS_NO_PARAMS, dev_suspend, + DM_QUERY_INACTIVE_TABLE_FLAG | DM_SUSPEND_FLAG | DM_SKIP_LOCKFS_FLAG | = DM_NOFLUSH_FLAG), + I(DM_DEV_STATUS_CMD, IOCTL_FLAGS_NO_PARAMS, dev_status, DM_QUERY_INACTIV= E_TABLE_FLAG), + I(DM_DEV_WAIT_CMD, IOCTL_FLAGS_TAKES_EVENT_NR, dev_wait, + DM_QUERY_INACTIVE_TABLE_FLAG | DM_STATUS_TABLE_FLAG | DM_NOFLUSH_FLAG), + I(DM_TABLE_LOAD_CMD, 0, table_load, DM_QUERY_INACTIVE_TABLE_FLAG | DM_RE= ADONLY_FLAG), + I(DM_TABLE_CLEAR_CMD, IOCTL_FLAGS_NO_PARAMS, table_clear, DM_QUERY_INACT= IVE_TABLE_FLAG), + I(DM_TABLE_DEPS_CMD, 0, table_deps, DM_QUERY_INACTIVE_TABLE_FLAG), + I(DM_TABLE_STATUS_CMD, 0, table_status, + DM_QUERY_INACTIVE_TABLE_FLAG | DM_STATUS_TABLE_FLAG | DM_NOFLUSH_FLAG), =20 - {DM_DEV_CREATE_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVE= NT, dev_create}, - {DM_DEV_REMOVE_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVE= NT, dev_remove}, - {DM_DEV_RENAME_CMD, IOCTL_FLAGS_ISSUE_GLOBAL_EVENT, dev_rename}, - {DM_DEV_SUSPEND_CMD, IOCTL_FLAGS_NO_PARAMS, dev_suspend}, - {DM_DEV_STATUS_CMD, IOCTL_FLAGS_NO_PARAMS, dev_status}, - {DM_DEV_WAIT_CMD, 0, dev_wait}, + I(DM_LIST_VERSIONS_CMD, 0, list_versions, 0), =20 - {DM_TABLE_LOAD_CMD, 0, table_load}, - {DM_TABLE_CLEAR_CMD, IOCTL_FLAGS_NO_PARAMS, table_clear}, - {DM_TABLE_DEPS_CMD, 0, table_deps}, - {DM_TABLE_STATUS_CMD, 0, table_status}, - - {DM_LIST_VERSIONS_CMD, 0, list_versions}, - - {DM_TARGET_MSG_CMD, 0, target_message}, - {DM_DEV_SET_GEOMETRY_CMD, 0, dev_set_geometry}, - {DM_DEV_ARM_POLL_CMD, IOCTL_FLAGS_NO_PARAMS, dev_arm_poll}, - {DM_GET_TARGET_VERSION_CMD, 0, get_target_version}, + I(DM_TARGET_MSG_CMD, 0, target_message, DM_QUERY_INACTIVE_TABLE_FLAG), + I(DM_DEV_SET_GEOMETRY_CMD, 0, dev_set_geometry, 0), + I(DM_DEV_ARM_POLL_CMD, IOCTL_FLAGS_NO_PARAMS, dev_arm_poll, 0), + I(DM_GET_TARGET_VERSION_CMD, 0, get_target_version, 0), }; =20 if (unlikely(cmd >=3D ARRAY_SIZE(_ioctls))) return NULL; =20 cmd =3D array_index_nospec(cmd, ARRAY_SIZE(_ioctls)); + *supported_flags =3D _ioctls[cmd].supported_flags; *ioctl_flags =3D _ioctls[cmd].flags; return _ioctls[cmd].fn; } @@ -1877,27 +1966,34 @@ static int check_version(unsigned int cmd, struct d= m_ioctl __user *user, struct dm_ioctl *kernel_params) { int r =3D 0; - uint32_t *version =3D kernel_params->version; + uint32_t expected_major_version =3D DM_VERSION_MAJOR; =20 - if (copy_from_user(version, user->version, sizeof(user->version))) + if (copy_from_user(kernel_params->version, user->version, sizeof(kernel_p= arams->version))) return -EFAULT; =20 - if ((version[0] !=3D DM_VERSION_MAJOR) || - (version[1] > DM_VERSION_MINOR)) { + if (kernel_params->version[0] >=3D DM_VERSION_MAJOR_STRICT) + expected_major_version =3D DM_VERSION_MAJOR_STRICT; + + if ((kernel_params->version[0] !=3D expected_major_version) || + (kernel_params->version[1] > DM_VERSION_MINOR)) { DMERR("ioctl interface mismatch: kernel(%u.%u.%u), user(%u.%u.%u), cmd(%= d)", - DM_VERSION_MAJOR, DM_VERSION_MINOR, + expected_major_version, + DM_VERSION_MINOR, DM_VERSION_PATCHLEVEL, - version[0], version[1], version[2], cmd); + kernel_params->version[0], + kernel_params->version[1], + kernel_params->version[2], + cmd); r =3D -EINVAL; } =20 /* * Fill in the kernel version. */ - version[0] =3D DM_VERSION_MAJOR; - version[1] =3D DM_VERSION_MINOR; - version[2] =3D DM_VERSION_PATCHLEVEL; - if (copy_to_user(user->version, version, sizeof(version))) + kernel_params->version[0] =3D expected_major_version; + kernel_params->version[1] =3D DM_VERSION_MINOR; + kernel_params->version[2] =3D DM_VERSION_PATCHLEVEL; + if (copy_to_user(user->version, kernel_params->version, sizeof(kernel_par= ams->version))) return -EFAULT; =20 return r; @@ -1920,9 +2016,12 @@ static int copy_params(struct dm_ioctl __user *user,= struct dm_ioctl *param_kern { struct dm_ioctl *dmi; int secure_data; - const size_t minimum_data_size =3D offsetof(struct dm_ioctl, data); + const size_t minimum_data_size =3D sloppy_checks(param_kernel) ? + offsetof(struct dm_ioctl, data) : sizeof(struct dm_ioctl); unsigned int noio_flag; =20 + static_assert(offsetof(struct dm_ioctl, data_size) =3D=3D sizeof(param_ke= rnel->version)); + static_assert(offsetof(struct dm_ioctl, data_size) =3D=3D 12); /* Version has been copied from userspace already, avoid TOCTOU */ if (copy_from_user((char *)param_kernel + sizeof(param_kernel->version), (char __user *)user + sizeof(param_kernel->version), @@ -1930,12 +2029,13 @@ static int copy_params(struct dm_ioctl __user *user= , struct dm_ioctl *param_kern return -EFAULT; =20 if (param_kernel->data_size < minimum_data_size) { - DMERR("Invalid data size in the ioctl structure: %u", - param_kernel->data_size); + DMERR("Invalid data size in the ioctl structure: %u (minimum %zu)", + param_kernel->data_size, minimum_data_size); return -EINVAL; } =20 secure_data =3D param_kernel->flags & DM_SECURE_DATA_FLAG; + param_kernel->flags &=3D ~DM_SECURE_DATA_FLAG; =20 *param_flags =3D secure_data ? DM_WIPE_BUFFER : 0; =20 @@ -1966,7 +2066,8 @@ static int copy_params(struct dm_ioctl __user *user, = struct dm_ioctl *param_kern /* Copy from param_kernel (which was already copied from user) */ memcpy(dmi, param_kernel, minimum_data_size); =20 - if (copy_from_user(&dmi->data, (char __user *)user + minimum_data_size, + if (copy_from_user((char *)dmi + minimum_data_size, + (char __user *)user + minimum_data_size, param_kernel->data_size - minimum_data_size)) goto bad; data_copied: @@ -1983,33 +2084,86 @@ static int copy_params(struct dm_ioctl __user *user= , struct dm_ioctl *param_kern return -EFAULT; } =20 -static int validate_params(uint cmd, struct dm_ioctl *param) +static int validate_params(uint cmd, struct dm_ioctl *param, + uint32_t ioctl_flags, uint32_t supported_flags) { - /* Always clear this flag */ - param->flags &=3D ~DM_BUFFER_FULL_FLAG; - param->flags &=3D ~DM_UEVENT_GENERATED_FLAG; - param->flags &=3D ~DM_SECURE_DATA_FLAG; - param->flags &=3D ~DM_DATA_OUT_FLAG; - - /* Ignores parameters */ - if (cmd =3D=3D DM_REMOVE_ALL_CMD || - cmd =3D=3D DM_LIST_DEVICES_CMD || - cmd =3D=3D DM_LIST_VERSIONS_CMD) - return 0; + static_assert(__same_type(param->flags, supported_flags)); + u64 zero =3D 0; =20 if (cmd =3D=3D DM_DEV_CREATE_CMD) { if (!*param->name) { DMERR("name not supplied when creating device"); return -EINVAL; } - } else if (*param->uuid && *param->name) { - DMERR("only supply one of name or uuid, cmd(%u)", cmd); + } else { + if (*param->uuid && *param->name) { + DMERR("only supply one of name or uuid, cmd(%u)", cmd); + return -EINVAL; + } + } + + if (sloppy_checks(param)) { + /* Ensure strings are terminated */ + param->name[DM_NAME_LEN - 1] =3D '\0'; + param->uuid[DM_UUID_LEN - 1] =3D '\0'; + /* Mask off bits that could confuse other code */ + param->flags &=3D ~DM_STRICT_ONLY_FLAGS; + /* Skip strict checks */ + return 0; + } + + /* Check that strings are terminated */ + if (!no_non_nul_after_nul(param->name, DM_NAME_LEN, cmd, "Name") || + !no_non_nul_after_nul(param->uuid, DM_UUID_LEN, cmd, "UUID")) { return -EINVAL; } =20 - /* Ensure strings are terminated */ - param->name[DM_NAME_LEN - 1] =3D '\0'; - param->uuid[DM_UUID_LEN - 1] =3D '\0'; + if (memcmp(param->data, &zero, sizeof(param->data)) !=3D 0) { + DMERR("second padding field not zeroed in strict mode (cmd %u)", cmd); + return -EINVAL; + } + + if (param->flags & ~supported_flags) { + DMERR("unsupported flags 0x%x specified, cmd(%u)", + param->flags & ~supported_flags, cmd); + return -EINVAL; + } + + if (param->padding) { + DMERR("padding not zeroed in strict mode (got %u, cmd %u)", + param->padding, cmd); + return -EINVAL; + } + + if (param->open_count !=3D 0) { + DMERR("open_count not zeroed in strict mode (got %d, cmd %u)", + param->open_count, cmd); + return -EINVAL; + } + + if (param->event_nr !=3D 0 && (ioctl_flags & IOCTL_FLAGS_TAKES_EVENT_NR) = =3D=3D 0) { + DMERR("Event number not zeroed for command that does not take one (got %= u, cmd %u)", + param->event_nr, cmd); + return -EINVAL; + } + + if (ioctl_flags & IOCTL_FLAGS_NO_PARAMS) { + /* Ignores parameters */ + if (param->data_size !=3D sizeof(struct dm_ioctl)) { + DMERR("command %u must not have parameters", cmd); + return -EINVAL; + } + + if (param->target_count !=3D 0) { + DMERR("command %u must have zero target_count", cmd); + return -EINVAL; + } + + if (param->data_start) { + DMERR("command %u must have zero data_start", cmd); + return -EINVAL; + } + } =20 return 0; } @@ -2024,6 +2178,7 @@ static int ctl_ioctl(struct file *file, uint command,= struct dm_ioctl __user *us ioctl_fn fn =3D NULL; size_t input_param_size; struct dm_ioctl param_kernel; + uint32_t supported_flags, old_flags; =20 /* only root can play with this */ if (!capable(CAP_SYS_ADMIN)) @@ -2039,7 +2194,7 @@ static int ctl_ioctl(struct file *file, uint command,= struct dm_ioctl __user *us * writes out the kernel's interface version. */ r =3D check_version(cmd, user, ¶m_kernel); - if (r) + if (r !=3D 0) return r; =20 /* @@ -2048,7 +2203,7 @@ static int ctl_ioctl(struct file *file, uint command,= struct dm_ioctl __user *us if (cmd =3D=3D DM_VERSION_CMD) return 0; =20 - fn =3D lookup_ioctl(cmd, &ioctl_flags); + fn =3D lookup_ioctl(cmd, &ioctl_flags, &supported_flags); if (!fn) { DMERR("dm_ctl_ioctl: unknown command 0x%x", command); return -ENOTTY; @@ -2063,11 +2218,20 @@ static int ctl_ioctl(struct file *file, uint comman= d, struct dm_ioctl __user *us return r; =20 input_param_size =3D param->data_size; - r =3D validate_params(cmd, param); + + /* + * In sloppy mode, validate_params will clear some + * flags to ensure other code does not get confused. + * Save the original flags here. + */ + old_flags =3D param->flags; + r =3D validate_params(cmd, param, ioctl_flags, supported_flags); if (r) goto out; + /* This XOR keeps only the flags validate_params has changed. */ + old_flags ^=3D param->flags; =20 - param->data_size =3D offsetof(struct dm_ioctl, data); + param->data_size =3D sloppy_checks(param) ? offsetof(struct dm_ioctl, dat= a) : sizeof(struct dm_ioctl); r =3D fn(file, param, input_param_size); =20 if (unlikely(param->flags & DM_BUFFER_FULL_FLAG) && @@ -2077,6 +2241,9 @@ static int ctl_ioctl(struct file *file, uint command,= struct dm_ioctl __user *us if (!r && ioctl_flags & IOCTL_FLAGS_ISSUE_GLOBAL_EVENT) dm_issue_global_event(); =20 + /* Resture the flags that validate_params cleared */ + param->flags |=3D old_flags; + /* * Copy the results back to userland. */ diff --git a/include/uapi/linux/dm-ioctl.h b/include/uapi/linux/dm-ioctl.h index 1990b5700f6948243def314cec22f380926aca2e..81103e1dcdac3015204e9c05d73= 037191e965d59 100644 --- a/include/uapi/linux/dm-ioctl.h +++ b/include/uapi/linux/dm-ioctl.h @@ -171,8 +171,11 @@ struct dm_target_spec { =20 /* * Parameter string starts immediately after this object. - * Be careful to add padding after string to ensure correct - * alignment of subsequent dm_target_spec. + * Be careful to add padding after string to ensure 8-byte + * alignment of subsequent dm_target_spec. If the major version + * is DM_VERSION_MAJOR_STRICT, the padding must be at most 7 bytes, + * (not including the terminating NULt that ends the string) and + * must be zeroed. */ }; =20 @@ -285,14 +288,25 @@ enum { #define DM_TARGET_MSG _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, struct dm_ioctl) #define DM_DEV_SET_GEOMETRY _IOWR(DM_IOCTL, DM_DEV_SET_GEOMETRY_CMD, struc= t dm_ioctl) =20 +/* Legacy major version */ #define DM_VERSION_MAJOR 4 -#define DM_VERSION_MINOR 48 +/* + * New major version. Enforces strict parameter checks and is required for + * using some new features, such as new flags. Should be used by all new = code. + * + * If one uses DM_VERSION_MAJOR_STRICT, it is possible for the behavior of + * ioctls to depend on the minor version passed by userspace. Userspace m= ust + * not pass a minor version greater than the version it was designed for. + */ +#define DM_VERSION_MAJOR_STRICT 5 +#define DM_VERSION_MINOR 49 #define DM_VERSION_PATCHLEVEL 0 #define DM_VERSION_EXTRA "-ioctl (2023-03-01)" =20 /* Status bits */ #define DM_READONLY_FLAG (1 << 0) /* In/Out */ #define DM_SUSPEND_FLAG (1 << 1) /* In/Out */ +#define DM_EXISTS_FLAG (1 << 2) /* Not used by kernel, reserved for libde= vmapper in userland */ #define DM_PERSISTENT_DEV_FLAG (1 << 3) /* In */ =20 /* @@ -315,7 +329,8 @@ enum { #define DM_BUFFER_FULL_FLAG (1 << 8) /* Out */ =20 /* - * This flag is now ignored. + * This flag is now ignored if DM_VERSION_MAJOR is used, and causes + * -EINVAL if DM_VERSION_MAJOR_STRICT is used. */ #define DM_SKIP_BDGET_FLAG (1 << 9) /* In */ =20 @@ -382,4 +397,11 @@ enum { */ #define DM_IMA_MEASUREMENT_FLAG (1 << 19) /* In */ =20 +/* + * If DM_VERSION_MAJOR is used, these flags are ignored by the kernel. + * If DM_VERSION_MAJOR_STRICT is used, these flags are reserved and + * must be zeroed. + */ +#define DM_STRICT_ONLY_FLAGS ((__u32)0xFFF00004) + #endif /* _LINUX_DM_IOCTL_H */ --=20 Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab