[PATCH 0/6] Several device-mapper fixes

[PATCH 0/6] Several device-mapper fixes

[Demi Marie Obenour]

This series contains several miscellaneous fixes to input validation in
device-mapper.  The only potentially controversial commits should be the
last two, which prevent creating devices named ".", "..", or "control".
The other patches fix input validation problems that have existed since
at least the beginning of git history.

Demi Marie Obenour (6):
  device-mapper: Check that target specs are sufficiently aligned
  device-mapper: Avoid pointer arithmetic overflow
  device-mapper: structs and parameter strings must not overlap
  device-mapper: Avoid double-fetch of version
  device-mapper: Refuse to create device named "control"
  device-mapper: "." and ".." are not valid symlink names

 drivers/md/dm-ioctl.c | 75 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 63 insertions(+), 12 deletions(-)

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 1/6] device-mapper: Check that target specs are sufficiently aligned

[Demi Marie Obenour]

Otherwise subsequent code will dereference a misaligned
`struct dm_target_spec *`, which is undefined behavior.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
---
 drivers/md/dm-ioctl.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index cc77cf3d410921432eb0c62cdede7d55b9aa674a..34fa74c6a70db8aa67aaba3f6a2fc4f38ef736bc 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1394,6 +1394,13 @@ static inline fmode_t get_mode(struct dm_ioctl *param)
 static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
 		       struct dm_target_spec **spec, char **target_params)
 {
+	static_assert(_Alignof(struct dm_target_spec) <= 8,
+		      "struct dm_target_spec has excessive alignment requirements");
+	if (next % 8) {
+		DMERR("Next target spec (offset %u) is not 8-byte aligned", next);
+		return -EINVAL;
+	}
+
 	*spec = (struct dm_target_spec *) ((unsigned char *) last + next);
 	*target_params = (char *) (*spec + 1);
 
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 2/6] device-mapper: Avoid pointer arithmetic overflow

[Demi Marie Obenour]

Especially on 32-bit systems, it is possible for the pointer arithmetic
to overflow and cause a userspace pointer to be dereferenced in the
kernel.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
---
 drivers/md/dm-ioctl.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 34fa74c6a70db8aa67aaba3f6a2fc4f38ef736bc..64e8f16d344c47057de5e2d29e3d63202197dca0 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1396,6 +1396,25 @@ static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
 {
 	static_assert(_Alignof(struct dm_target_spec) <= 8,
 		      "struct dm_target_spec has excessive alignment requirements");
+	static_assert(offsetof(struct dm_ioctl, data) >= sizeof(struct dm_target_spec),
+		      "struct dm_target_spec too big");
+
+	/*
+	 * Number of bytes remaining, starting with last. This is always
+	 * sizeof(struct dm_target_spec) or more, as otherwise *last was
+	 * out of bounds already.
+	 */
+	size_t remaining = (char *)end - (char *)last;
+
+	/*
+	 * There must be room for both the next target spec and the
+	 * NUL-terminator of the target itself.
+	 */
+	if (remaining - sizeof(struct dm_target_spec) <= next) {
+		DMERR("Target spec extends beyond end of parameters");
+		return -EINVAL;
+	}
+
 	if (next % 8) {
 		DMERR("Next target spec (offset %u) is not 8-byte aligned", next);
 		return -EINVAL;
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 3/6] device-mapper: structs and parameter strings must not overlap

[Demi Marie Obenour]

The NUL terminator for each target parameter string must precede the
following 'struct dm_target_spec'.  Otherwise, dm_split_args() might
corrupt this struct.  Furthermore, the first 'struct dm_target_spec'
must come after the 'struct dm_ioctl', as if it overlaps too much
dm_split_args() could corrupt the 'struct dm_ioctl'.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
---
 drivers/md/dm-ioctl.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 64e8f16d344c47057de5e2d29e3d63202197dca0..da6ca26b51d0953df380582bb3a51c2ec22c27cb 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1391,7 +1391,7 @@ static inline fmode_t get_mode(struct dm_ioctl *param)
 	return mode;
 }
 
-static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
+static int next_target(struct dm_target_spec *last, uint32_t next, const char *end,
 		       struct dm_target_spec **spec, char **target_params)
 {
 	static_assert(_Alignof(struct dm_target_spec) <= 8,
@@ -1404,7 +1404,7 @@ static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
 	 * sizeof(struct dm_target_spec) or more, as otherwise *last was
 	 * out of bounds already.
 	 */
-	size_t remaining = (char *)end - (char *)last;
+	size_t remaining = end - (char *)last;
 
 	/*
 	 * There must be room for both the next target spec and the
@@ -1423,10 +1423,7 @@ static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
 	*spec = (struct dm_target_spec *) ((unsigned char *) last + next);
 	*target_params = (char *) (*spec + 1);
 
-	if (*spec < (last + 1))
-		return -EINVAL;
-
-	return invalid_str(*target_params, end);
+	return 0;
 }
 
 static int populate_table(struct dm_table *table,
@@ -1436,8 +1433,9 @@ static int populate_table(struct dm_table *table,
 	unsigned int i = 0;
 	struct dm_target_spec *spec = (struct dm_target_spec *) param;
 	uint32_t next = param->data_start;
-	void *end = (void *) param + param_size;
+	const char *const end = (const char *) param + param_size;
 	char *target_params;
+	size_t min_size = sizeof(struct dm_ioctl);
 
 	if (!param->target_count) {
 		DMERR("%s: no targets specified", __func__);
@@ -1445,6 +1443,13 @@ static int populate_table(struct dm_table *table,
 	}
 
 	for (i = 0; i < param->target_count; i++) {
+		const char *nul_terminator;
+
+		if (next < min_size) {
+			DMERR("%s: next target spec (offset %u) overlaps %s",
+			      __func__, next, i ? "previous target" : "'struct dm_ioctl'");
+			return -EINVAL;
+		}
 
 		r = next_target(spec, next, end, &spec, &target_params);
 		if (r) {
@@ -1452,6 +1457,15 @@ static int populate_table(struct dm_table *table,
 			return r;
 		}
 
+		nul_terminator = memchr(target_params, 0, (size_t)(end - target_params));
+		if (nul_terminator == NULL) {
+			DMERR("%s: target parameters not NUL-terminated", __func__);
+			return -EINVAL;
+		}
+
+		/* Add 1 for NUL terminator */
+		min_size = (size_t)(nul_terminator - (const char *)spec) + 1;
+
 		r = dm_table_add_target(table, spec->target_type,
 					(sector_t) spec->sector_start,
 					(sector_t) spec->length,
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 4/6] device-mapper: Avoid double-fetch of version

[Demi Marie Obenour]

The version is fetched once in check_version(), which then does some
validation and then overwrites the version in userspace with the API
version supported by the kernel.  copy_params() then fetches the version
from userspace *again*, and this time no validation is done.  The result
is that the kernel's version number is completely controllable by
userspace, provided that userspace can win a race condition.

Fix this flaw by not copying the version back to the kernel the second
time.  This is not exploitable as the version is not further used in the
kernel.  However, it could become a problem if future patches start
relying on the version field.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/md/dm-ioctl.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index da6ca26b51d0953df380582bb3a51c2ec22c27cb..fd46b249f6f856c49752063fc49d720e95df0525 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1873,12 +1873,13 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags)
  * As well as checking the version compatibility this always
  * copies the kernel interface version out.
  */
-static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
+static int check_version(unsigned int cmd, struct dm_ioctl __user *user,
+			 struct dm_ioctl *kernel_params)
 {
-	uint32_t version[3];
 	int r = 0;
+	uint32_t *version = kernel_params->version;
 
-	if (copy_from_user(version, user->version, sizeof(version)))
+	if (copy_from_user(version, user->version, sizeof(user->version)))
 		return -EFAULT;
 
 	if ((version[0] != DM_VERSION_MAJOR) ||
@@ -1922,7 +1923,10 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
 	const size_t minimum_data_size = offsetof(struct dm_ioctl, data);
 	unsigned int noio_flag;
 
-	if (copy_from_user(param_kernel, user, minimum_data_size))
+	/* 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),
+			   minimum_data_size - sizeof(param_kernel->version)))
 		return -EFAULT;
 
 	if (param_kernel->data_size < minimum_data_size) {
@@ -2034,7 +2038,7 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
 	 * Check the interface version passed in.  This also
 	 * writes out the kernel's interface version.
 	 */
-	r = check_version(cmd, user);
+	r = check_version(cmd, user, &param_kernel);
 	if (r)
 		return r;
 
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [PATCH 4/6] device-mapper: Avoid double-fetch of version

[kernel test robot]

Hi Demi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on device-mapper-dm/for-next]
[also build test WARNING on linus/master v6.4-rc4 next-20230602]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Demi-Marie-Obenour/device-mapper-Check-that-target-specs-are-sufficiently-aligned/20230602-052741
base:   https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git for-next
patch link:    https://lore.kernel.org/r/20230601212456.1533-5-demi%40invisiblethingslab.com
patch subject: [PATCH 4/6] device-mapper: Avoid double-fetch of version
config: x86_64-randconfig-c032-20230531 (https://download.01.org/0day-ci/archive/20230603/202306031511.xIeQ4BQz-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306031511.xIeQ4BQz-lkp@intel.com/

cocci warnings: (new ones prefixed by >>)
>> drivers/md/dm-ioctl.c:1900:42-48: ERROR: application of sizeof to pointer

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/6] device-mapper: Avoid double-fetch of version

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

On Sat, Jun 03, 2023 at 03:40:09PM +0800, kernel test robot wrote:
> Hi Demi,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on device-mapper-dm/for-next]
> [also build test WARNING on linus/master v6.4-rc4 next-20230602]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Demi-Marie-Obenour/device-mapper-Check-that-target-specs-are-sufficiently-aligned/20230602-052741
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git for-next
> patch link:    https://lore.kernel.org/r/20230601212456.1533-5-demi%40invisiblethingslab.com
> patch subject: [PATCH 4/6] device-mapper: Avoid double-fetch of version
> config: x86_64-randconfig-c032-20230531 (https://download.01.org/0day-ci/archive/20230603/202306031511.xIeQ4BQz-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202306031511.xIeQ4BQz-lkp@intel.com/
> 
> cocci warnings: (new ones prefixed by >>)
> >> drivers/md/dm-ioctl.c:1900:42-48: ERROR: application of sizeof to pointer

Ugh, silly mistake: I changed an array to a pointer but did not change
the sizeof.  Will send a v2 with the fix.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 5/6] device-mapper: Refuse to create device named "control"

[Demi Marie Obenour]

Typical userspace setups create a symlink under /dev/mapper with the
name of the device, but /dev/mapper/control is reserved for the control
device.  Therefore, trying to create such a device is almost certain to
be a userspace bug.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/md/dm-ioctl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index fd46b249f6f856c49752063fc49d720e95df0525..b12592bcb4b2b8513f5da6208fb545203534d7ff 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -767,7 +767,12 @@ static int get_target_version(struct file *filp, struct dm_ioctl *param, size_t
 static int check_name(const char *name)
 {
 	if (strchr(name, '/')) {
-		DMERR("invalid device name");
+		DMERR("device name cannot contain '/'");
+		return -EINVAL;
+	}
+
+	if (strcmp(name, DM_CONTROL_NODE) == 0) {
+		DMERR("device name cannot be \"%s\"", DM_CONTROL_NODE);
 		return -EINVAL;
 	}
 
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 6/6] device-mapper: "." and ".." are not valid symlink names

[Demi Marie Obenour]

Using either of these is going to greatly confuse userspace, as they are
not valid symlink names and so creating the usual /dev/mapper/NAME
symlink will not be possible.  As creating a device with either of these
names is almost certainly a userspace bug, just error out.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/md/dm-ioctl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index b12592bcb4b2b8513f5da6208fb545203534d7ff..adf0c4becc743e4ad59e1d6b0ef108ddd56f207d 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -771,8 +771,10 @@ static int check_name(const char *name)
 		return -EINVAL;
 	}
 
-	if (strcmp(name, DM_CONTROL_NODE) == 0) {
-		DMERR("device name cannot be \"%s\"", DM_CONTROL_NODE);
+	if (strcmp(name, DM_CONTROL_NODE) == 0 ||
+	    strcmp(name, ".") == 0 ||
+	    strcmp(name, "..") == 0) {
+		DMERR("device name cannot be \"%s\", \".\", or \"..\"", DM_CONTROL_NODE);
 		return -EINVAL;
 	}
 
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH v2 0/6] Several device-mapper fixes

[Demi Marie Obenour]

This series contains several miscellaneous fixes to input validation in
device-mapper.  The only potentially controversial commits should be the
last two, which prevent creating devices named ".", "..", or "control".
The other patches fix input validation problems that have existed since
at least the beginning of git history.

Changes since v1:

- Fix silly mistake (using sizeof() on a pointer) caught by 0day bot.

Demi Marie Obenour (6):
  device-mapper: Check that target specs are sufficiently aligned
  device-mapper: Avoid pointer arithmetic overflow
  device-mapper: structs and parameter strings must not overlap
  device-mapper: Avoid double-fetch of version
  device-mapper: Refuse to create device named "control"
  device-mapper: "." and ".." are not valid symlink names

 drivers/md/dm-ioctl.c | 91 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 72 insertions(+), 19 deletions(-)

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH v2 1/6] device-mapper: Check that target specs are sufficiently aligned

[Demi Marie Obenour]

Otherwise subsequent code will dereference a misaligned
`struct dm_target_spec *`, which is undefined behavior.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
---
 drivers/md/dm-ioctl.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index cc77cf3d410921432eb0c62cdede7d55b9aa674a..34fa74c6a70db8aa67aaba3f6a2fc4f38ef736bc 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1394,6 +1394,13 @@ static inline fmode_t get_mode(struct dm_ioctl *param)
 static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
 		       struct dm_target_spec **spec, char **target_params)
 {
+	static_assert(_Alignof(struct dm_target_spec) <= 8,
+		      "struct dm_target_spec has excessive alignment requirements");
+	if (next % 8) {
+		DMERR("Next target spec (offset %u) is not 8-byte aligned", next);
+		return -EINVAL;
+	}
+
 	*spec = (struct dm_target_spec *) ((unsigned char *) last + next);
 	*target_params = (char *) (*spec + 1);
 
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [PATCH v2 1/6] device-mapper: Check that target specs are sufficiently aligned

[Mike Snitzer]

On Sat, Jun 03 2023 at 10:52P -0400,
Demi Marie Obenour <demi@invisiblethingslab.com> wrote:

> Otherwise subsequent code will dereference a misaligned
> `struct dm_target_spec *`, which is undefined behavior.
> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> ---
>  drivers/md/dm-ioctl.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index cc77cf3d410921432eb0c62cdede7d55b9aa674a..34fa74c6a70db8aa67aaba3f6a2fc4f38ef736bc 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1394,6 +1394,13 @@ static inline fmode_t get_mode(struct dm_ioctl *param)
>  static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
>  		       struct dm_target_spec **spec, char **target_params)
>  {
> +	static_assert(_Alignof(struct dm_target_spec) <= 8,
> +		      "struct dm_target_spec has excessive alignment requirements");

Really not sure what you mean by "has excessive alignment requirements"...

> +	if (next % 8) {
> +		DMERR("Next target spec (offset %u) is not 8-byte aligned", next);
> +		return -EINVAL;
> +	}
> +
>  	*spec = (struct dm_target_spec *) ((unsigned char *) last + next);
>  	*target_params = (char *) (*spec + 1);
>  

But this patch and patches 2 and 3 need more review. I'd like Mikulas to review.

I did pick up patches 4-6 for the upcoming 6.5 merge window.

Note, please prefix with "dm ioctl" instead of "device-mapper".

(I just switched my "dm" prefix to "dm ioctl" and forced update on the
dm-6.5 branch, so the commit I referenced earlier for your version
copy patch is now here:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.5&id=a5a3de762b3ae8959347928843c12502b1b23163
)

Mike

Re: [PATCH v2 1/6] device-mapper: Check that target specs are sufficiently aligned

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 2393 bytes --]

On Thu, Jun 22, 2023 at 12:28:28PM -0400, Mike Snitzer wrote:
> On Sat, Jun 03 2023 at 10:52P -0400,
> Demi Marie Obenour <demi@invisiblethingslab.com> wrote:
> 
> > Otherwise subsequent code will dereference a misaligned
> > `struct dm_target_spec *`, which is undefined behavior.
> > 
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/md/dm-ioctl.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> > index cc77cf3d410921432eb0c62cdede7d55b9aa674a..34fa74c6a70db8aa67aaba3f6a2fc4f38ef736bc 100644
> > --- a/drivers/md/dm-ioctl.c
> > +++ b/drivers/md/dm-ioctl.c
> > @@ -1394,6 +1394,13 @@ static inline fmode_t get_mode(struct dm_ioctl *param)
> >  static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
> >  		       struct dm_target_spec **spec, char **target_params)
> >  {
> > +	static_assert(_Alignof(struct dm_target_spec) <= 8,
> > +		      "struct dm_target_spec has excessive alignment requirements");
> 
> Really not sure what you mean by "has excessive alignment requirements"...

This patch checks that struct dm_target_spec is 8-byte aligned.  That is
okay if its alignment is 8 or less, but not if is 16 or more, so I added
a static assert to check that struct dm_target_spec indeed requires at
most 8-byte alignment.  That said, “excessive alignment requirements” is
(as shown by you having to ask this question) a bad error message.
Would “must not require more than 8-byte alignment” be better?

> > +	if (next % 8) {
> > +		DMERR("Next target spec (offset %u) is not 8-byte aligned", next);
> > +		return -EINVAL;
> > +	}
> > +
> >  	*spec = (struct dm_target_spec *) ((unsigned char *) last + next);
> >  	*target_params = (char *) (*spec + 1);
> >  
> 
> But this patch and patches 2 and 3 need more review. I'd like Mikulas to review.
> 
> I did pick up patches 4-6 for the upcoming 6.5 merge window.

Thanks!

> Note, please prefix with "dm ioctl" instead of "device-mapper".

Good to know, thanks!  I have several additional patches written that
require patch 4.  Should I send patches 1 through 3 in the same series
as well?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/6] device-mapper: Check that target specs are sufficiently aligned

[Mike Snitzer]

On Thu, Jun 22 2023 at  3:51P -0400,
Demi Marie Obenour <demi@invisiblethingslab.com> wrote:

> On Thu, Jun 22, 2023 at 12:28:28PM -0400, Mike Snitzer wrote:
> > On Sat, Jun 03 2023 at 10:52P -0400,
> > Demi Marie Obenour <demi@invisiblethingslab.com> wrote:
> > 
> > > Otherwise subsequent code will dereference a misaligned
> > > `struct dm_target_spec *`, which is undefined behavior.
> > > 
> > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/md/dm-ioctl.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> > > index cc77cf3d410921432eb0c62cdede7d55b9aa674a..34fa74c6a70db8aa67aaba3f6a2fc4f38ef736bc 100644
> > > --- a/drivers/md/dm-ioctl.c
> > > +++ b/drivers/md/dm-ioctl.c
> > > @@ -1394,6 +1394,13 @@ static inline fmode_t get_mode(struct dm_ioctl *param)
> > >  static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
> > >  		       struct dm_target_spec **spec, char **target_params)
> > >  {
> > > +	static_assert(_Alignof(struct dm_target_spec) <= 8,
> > > +		      "struct dm_target_spec has excessive alignment requirements");
> > 
> > Really not sure what you mean by "has excessive alignment requirements"...
> 
> This patch checks that struct dm_target_spec is 8-byte aligned.  That is
> okay if its alignment is 8 or less, but not if is 16 or more, so I added
> a static assert to check that struct dm_target_spec indeed requires at
> most 8-byte alignment.  That said, “excessive alignment requirements” is
> (as shown by you having to ask this question) a bad error message.
> Would “must not require more than 8-byte alignment” be better?

Yes, that's better, I've updated it to use that.
 
> > > +	if (next % 8) {
> > > +		DMERR("Next target spec (offset %u) is not 8-byte aligned", next);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > >  	*spec = (struct dm_target_spec *) ((unsigned char *) last + next);
> > >  	*target_params = (char *) (*spec + 1);
> > >  
> > 
> > But this patch and patches 2 and 3 need more review. I'd like Mikulas to review.
> > 
> > I did pick up patches 4-6 for the upcoming 6.5 merge window.
> 
> Thanks!
> 
> > Note, please prefix with "dm ioctl" instead of "device-mapper".
> 
> Good to know, thanks!  I have several additional patches written that
> require patch 4.  Should I send patches 1 through 3 in the same series
> as well?

I did end up picking up patches 1-3 and rebased so they are in front
of your patches 4-6 like you intended.

But I agree with Mikulas, I'm not seeing the point in tagging any of
these for stable@.

All commits are currently at the tip of dm-6.5, see:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=dm-6.5

Mike

Re: [dm-devel] [PATCH v2 1/6] device-mapper: Check that target specs are sufficiently aligned

[Mikulas Patocka]

On Sat, 3 Jun 2023, Demi Marie Obenour wrote:

> Otherwise subsequent code will dereference a misaligned
> `struct dm_target_spec *`, which is undefined behavior.
> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> ---
>  drivers/md/dm-ioctl.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index cc77cf3d410921432eb0c62cdede7d55b9aa674a..34fa74c6a70db8aa67aaba3f6a2fc4f38ef736bc 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1394,6 +1394,13 @@ static inline fmode_t get_mode(struct dm_ioctl *param)
>  static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
>  		       struct dm_target_spec **spec, char **target_params)
>  {
> +	static_assert(_Alignof(struct dm_target_spec) <= 8,
> +		      "struct dm_target_spec has excessive alignment requirements");
> +	if (next % 8) {
> +		DMERR("Next target spec (offset %u) is not 8-byte aligned", next);
> +		return -EINVAL;
> +	}
> +
>  	*spec = (struct dm_target_spec *) ((unsigned char *) last + next);
>  	*target_params = (char *) (*spec + 1);
>  
> -- 
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab

Hi

Some architectures (such as 32-bit x86) specify that the alignment of 
64-bit integers is only 4-byte. This could in theory break old userspace 
code that only uses 4-byte alignment. I would change "next % 8" to "next % 
__alignof__(struct dm_target_spec)".

I think that there is no need to backport this patch series to the stable 
kernels because the bugs that it fixes may only be exploited by the user 
with CAP_SYS_ADMIN privilege. So, there is no security or reliability 
problem being fixed.

Mikulas

Re: [dm-devel] [PATCH v2 1/6] device-mapper: Check that target specs are sufficiently aligned

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 2343 bytes --]

On Thu, Jun 22, 2023 at 07:29:52PM +0200, Mikulas Patocka wrote:
> 
> 
> On Sat, 3 Jun 2023, Demi Marie Obenour wrote:
> 
> > Otherwise subsequent code will dereference a misaligned
> > `struct dm_target_spec *`, which is undefined behavior.
> > 
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/md/dm-ioctl.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> > index cc77cf3d410921432eb0c62cdede7d55b9aa674a..34fa74c6a70db8aa67aaba3f6a2fc4f38ef736bc 100644
> > --- a/drivers/md/dm-ioctl.c
> > +++ b/drivers/md/dm-ioctl.c
> > @@ -1394,6 +1394,13 @@ static inline fmode_t get_mode(struct dm_ioctl *param)
> >  static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
> >  		       struct dm_target_spec **spec, char **target_params)
> >  {
> > +	static_assert(_Alignof(struct dm_target_spec) <= 8,
> > +		      "struct dm_target_spec has excessive alignment requirements");
> > +	if (next % 8) {
> > +		DMERR("Next target spec (offset %u) is not 8-byte aligned", next);
> > +		return -EINVAL;
> > +	}
> > +
> >  	*spec = (struct dm_target_spec *) ((unsigned char *) last + next);
> >  	*target_params = (char *) (*spec + 1);
> >  
> > -- 
> > Sincerely,
> > Demi Marie Obenour (she/her/hers)
> > Invisible Things Lab
> 
> Hi
> 
> Some architectures (such as 32-bit x86) specify that the alignment of 
> 64-bit integers is only 4-byte. This could in theory break old userspace 
> code that only uses 4-byte alignment. I would change "next % 8" to "next % 
> __alignof__(struct dm_target_spec)".

That’s fine, provided that the rest of the code is okay with 4-byte
alignment.

> I think that there is no need to backport this patch series to the stable 
> kernels because the bugs that it fixes may only be exploited by the user 
> with CAP_SYS_ADMIN privilege. So, there is no security or reliability 
> problem being fixed.

I agree that there is no reliability problem, but with kernel lockdown
root → kernel is a security boundary, so fixes for memory unsafety
problems should still be backported IMO.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/6] device-mapper: Avoid pointer arithmetic overflow

[Demi Marie Obenour]

Especially on 32-bit systems, it is possible for the pointer arithmetic
to overflow and cause a userspace pointer to be dereferenced in the
kernel.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
---
 drivers/md/dm-ioctl.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 34fa74c6a70db8aa67aaba3f6a2fc4f38ef736bc..64e8f16d344c47057de5e2d29e3d63202197dca0 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1396,6 +1396,25 @@ static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
 {
 	static_assert(_Alignof(struct dm_target_spec) <= 8,
 		      "struct dm_target_spec has excessive alignment requirements");
+	static_assert(offsetof(struct dm_ioctl, data) >= sizeof(struct dm_target_spec),
+		      "struct dm_target_spec too big");
+
+	/*
+	 * Number of bytes remaining, starting with last. This is always
+	 * sizeof(struct dm_target_spec) or more, as otherwise *last was
+	 * out of bounds already.
+	 */
+	size_t remaining = (char *)end - (char *)last;
+
+	/*
+	 * There must be room for both the next target spec and the
+	 * NUL-terminator of the target itself.
+	 */
+	if (remaining - sizeof(struct dm_target_spec) <= next) {
+		DMERR("Target spec extends beyond end of parameters");
+		return -EINVAL;
+	}
+
 	if (next % 8) {
 		DMERR("Next target spec (offset %u) is not 8-byte aligned", next);
 		return -EINVAL;
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [dm-devel] [PATCH v2 2/6] device-mapper: Avoid pointer arithmetic overflow

[Mikulas Patocka]

On Sat, 3 Jun 2023, Demi Marie Obenour wrote:

> Especially on 32-bit systems, it is possible for the pointer arithmetic
> to overflow and cause a userspace pointer to be dereferenced in the
> kernel.
> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org

Reviewed-by: Mikulas Patocka <mpatocka@redhat.com>

> ---
>  drivers/md/dm-ioctl.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 34fa74c6a70db8aa67aaba3f6a2fc4f38ef736bc..64e8f16d344c47057de5e2d29e3d63202197dca0 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1396,6 +1396,25 @@ static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
>  {
>  	static_assert(_Alignof(struct dm_target_spec) <= 8,
>  		      "struct dm_target_spec has excessive alignment requirements");
> +	static_assert(offsetof(struct dm_ioctl, data) >= sizeof(struct dm_target_spec),
> +		      "struct dm_target_spec too big");
> +
> +	/*
> +	 * Number of bytes remaining, starting with last. This is always
> +	 * sizeof(struct dm_target_spec) or more, as otherwise *last was
> +	 * out of bounds already.
> +	 */
> +	size_t remaining = (char *)end - (char *)last;
> +
> +	/*
> +	 * There must be room for both the next target spec and the
> +	 * NUL-terminator of the target itself.
> +	 */
> +	if (remaining - sizeof(struct dm_target_spec) <= next) {
> +		DMERR("Target spec extends beyond end of parameters");
> +		return -EINVAL;
> +	}
> +
>  	if (next % 8) {
>  		DMERR("Next target spec (offset %u) is not 8-byte aligned", next);
>  		return -EINVAL;
> -- 
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://listman.redhat.com/mailman/listinfo/dm-devel
> 

Re: [PATCH v2 2/6] device-mapper: Avoid pointer arithmetic overflow

[Mike Snitzer]

On Sat, Jun 03 2023 at 10:52P -0400,
Demi Marie Obenour <demi@invisiblethingslab.com> wrote:

> Especially on 32-bit systems, it is possible for the pointer arithmetic
> to overflow and cause a userspace pointer to be dereferenced in the
> kernel.
> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> ---
>  drivers/md/dm-ioctl.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 34fa74c6a70db8aa67aaba3f6a2fc4f38ef736bc..64e8f16d344c47057de5e2d29e3d63202197dca0 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1396,6 +1396,25 @@ static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
>  {
>  	static_assert(_Alignof(struct dm_target_spec) <= 8,
>  		      "struct dm_target_spec has excessive alignment requirements");
> +	static_assert(offsetof(struct dm_ioctl, data) >= sizeof(struct dm_target_spec),
> +		      "struct dm_target_spec too big");

I'm struggling to see the point for this compile-time check?
Especially when you consider (on x86_64):

sizeof(struct dm_target_spec) = 40
offsetof(struct dm_ioctl, data) = 305

Just feels like there is no utility offered by adding this check.

SO I've dropped it.  But if you feel there is some inherent value
please let me know.

Thanks,
Mike

[PATCH v2 3/6] device-mapper: structs and parameter strings must not overlap

[Demi Marie Obenour]

The NUL terminator for each target parameter string must precede the
following 'struct dm_target_spec'.  Otherwise, dm_split_args() might
corrupt this struct.  Furthermore, the first 'struct dm_target_spec'
must come after the 'struct dm_ioctl', as if it overlaps too much
dm_split_args() could corrupt the 'struct dm_ioctl'.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
---
 drivers/md/dm-ioctl.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 64e8f16d344c47057de5e2d29e3d63202197dca0..da6ca26b51d0953df380582bb3a51c2ec22c27cb 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1391,7 +1391,7 @@ static inline fmode_t get_mode(struct dm_ioctl *param)
 	return mode;
 }
 
-static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
+static int next_target(struct dm_target_spec *last, uint32_t next, const char *end,
 		       struct dm_target_spec **spec, char **target_params)
 {
 	static_assert(_Alignof(struct dm_target_spec) <= 8,
@@ -1404,7 +1404,7 @@ static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
 	 * sizeof(struct dm_target_spec) or more, as otherwise *last was
 	 * out of bounds already.
 	 */
-	size_t remaining = (char *)end - (char *)last;
+	size_t remaining = end - (char *)last;
 
 	/*
 	 * There must be room for both the next target spec and the
@@ -1423,10 +1423,7 @@ static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
 	*spec = (struct dm_target_spec *) ((unsigned char *) last + next);
 	*target_params = (char *) (*spec + 1);
 
-	if (*spec < (last + 1))
-		return -EINVAL;
-
-	return invalid_str(*target_params, end);
+	return 0;
 }
 
 static int populate_table(struct dm_table *table,
@@ -1436,8 +1433,9 @@ static int populate_table(struct dm_table *table,
 	unsigned int i = 0;
 	struct dm_target_spec *spec = (struct dm_target_spec *) param;
 	uint32_t next = param->data_start;
-	void *end = (void *) param + param_size;
+	const char *const end = (const char *) param + param_size;
 	char *target_params;
+	size_t min_size = sizeof(struct dm_ioctl);
 
 	if (!param->target_count) {
 		DMERR("%s: no targets specified", __func__);
@@ -1445,6 +1443,13 @@ static int populate_table(struct dm_table *table,
 	}
 
 	for (i = 0; i < param->target_count; i++) {
+		const char *nul_terminator;
+
+		if (next < min_size) {
+			DMERR("%s: next target spec (offset %u) overlaps %s",
+			      __func__, next, i ? "previous target" : "'struct dm_ioctl'");
+			return -EINVAL;
+		}
 
 		r = next_target(spec, next, end, &spec, &target_params);
 		if (r) {
@@ -1452,6 +1457,15 @@ static int populate_table(struct dm_table *table,
 			return r;
 		}
 
+		nul_terminator = memchr(target_params, 0, (size_t)(end - target_params));
+		if (nul_terminator == NULL) {
+			DMERR("%s: target parameters not NUL-terminated", __func__);
+			return -EINVAL;
+		}
+
+		/* Add 1 for NUL terminator */
+		min_size = (size_t)(nul_terminator - (const char *)spec) + 1;
+
 		r = dm_table_add_target(table, spec->target_type,
 					(sector_t) spec->sector_start,
 					(sector_t) spec->length,
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [dm-devel] [PATCH v2 3/6] device-mapper: structs and parameter strings must not overlap

[Mikulas Patocka]

On Sat, 3 Jun 2023, Demi Marie Obenour wrote:

> The NUL terminator for each target parameter string must precede the
> following 'struct dm_target_spec'.  Otherwise, dm_split_args() might
> corrupt this struct.  Furthermore, the first 'struct dm_target_spec'
> must come after the 'struct dm_ioctl', as if it overlaps too much
> dm_split_args() could corrupt the 'struct dm_ioctl'.
> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org

Reviewed-by: Mikulas Patocka <mpatocka@redhat.com>

> ---
>  drivers/md/dm-ioctl.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 64e8f16d344c47057de5e2d29e3d63202197dca0..da6ca26b51d0953df380582bb3a51c2ec22c27cb 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1391,7 +1391,7 @@ static inline fmode_t get_mode(struct dm_ioctl *param)
>  	return mode;
>  }
>  
> -static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
> +static int next_target(struct dm_target_spec *last, uint32_t next, const char *end,
>  		       struct dm_target_spec **spec, char **target_params)
>  {
>  	static_assert(_Alignof(struct dm_target_spec) <= 8,
> @@ -1404,7 +1404,7 @@ static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
>  	 * sizeof(struct dm_target_spec) or more, as otherwise *last was
>  	 * out of bounds already.
>  	 */
> -	size_t remaining = (char *)end - (char *)last;
> +	size_t remaining = end - (char *)last;
>  
>  	/*
>  	 * There must be room for both the next target spec and the
> @@ -1423,10 +1423,7 @@ static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
>  	*spec = (struct dm_target_spec *) ((unsigned char *) last + next);
>  	*target_params = (char *) (*spec + 1);
>  
> -	if (*spec < (last + 1))
> -		return -EINVAL;
> -
> -	return invalid_str(*target_params, end);
> +	return 0;
>  }
>  
>  static int populate_table(struct dm_table *table,
> @@ -1436,8 +1433,9 @@ static int populate_table(struct dm_table *table,
>  	unsigned int i = 0;
>  	struct dm_target_spec *spec = (struct dm_target_spec *) param;
>  	uint32_t next = param->data_start;
> -	void *end = (void *) param + param_size;
> +	const char *const end = (const char *) param + param_size;
>  	char *target_params;
> +	size_t min_size = sizeof(struct dm_ioctl);
>  
>  	if (!param->target_count) {
>  		DMERR("%s: no targets specified", __func__);
> @@ -1445,6 +1443,13 @@ static int populate_table(struct dm_table *table,
>  	}
>  
>  	for (i = 0; i < param->target_count; i++) {
> +		const char *nul_terminator;
> +
> +		if (next < min_size) {
> +			DMERR("%s: next target spec (offset %u) overlaps %s",
> +			      __func__, next, i ? "previous target" : "'struct dm_ioctl'");
> +			return -EINVAL;
> +		}
>  
>  		r = next_target(spec, next, end, &spec, &target_params);
>  		if (r) {
> @@ -1452,6 +1457,15 @@ static int populate_table(struct dm_table *table,
>  			return r;
>  		}
>  
> +		nul_terminator = memchr(target_params, 0, (size_t)(end - target_params));
> +		if (nul_terminator == NULL) {
> +			DMERR("%s: target parameters not NUL-terminated", __func__);
> +			return -EINVAL;
> +		}
> +
> +		/* Add 1 for NUL terminator */
> +		min_size = (size_t)(nul_terminator - (const char *)spec) + 1;
> +
>  		r = dm_table_add_target(table, spec->target_type,
>  					(sector_t) spec->sector_start,
>  					(sector_t) spec->length,
> -- 
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://listman.redhat.com/mailman/listinfo/dm-devel
> 

[PATCH v2 4/6] device-mapper: Avoid double-fetch of version

[Demi Marie Obenour]

The version is fetched once in check_version(), which then does some
validation and then overwrites the version in userspace with the API
version supported by the kernel.  copy_params() then fetches the version
from userspace *again*, and this time no validation is done.  The result
is that the kernel's version number is completely controllable by
userspace, provided that userspace can win a race condition.

Fix this flaw by not copying the version back to the kernel the second
time.  This is not exploitable as the version is not further used in the
kernel.  However, it could become a problem if future patches start
relying on the version field.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/md/dm-ioctl.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index da6ca26b51d0953df380582bb3a51c2ec22c27cb..7510afe237d979a5ee71afe87a20d49f631de1aa 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1873,30 +1873,33 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags)
  * As well as checking the version compatibility this always
  * copies the kernel interface version out.
  */
-static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
+static int check_version(unsigned int cmd, struct dm_ioctl __user *user,
+			 struct dm_ioctl *kernel_params)
 {
-	uint32_t version[3];
 	int r = 0;
 
-	if (copy_from_user(version, user->version, sizeof(version)))
+	if (copy_from_user(kernel_params->version, user->version, sizeof(kernel_params->version)))
 		return -EFAULT;
 
-	if ((version[0] != DM_VERSION_MAJOR) ||
-	    (version[1] > DM_VERSION_MINOR)) {
+	if ((kernel_params->version[0] != DM_VERSION_MAJOR) ||
+	    (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,
 		      DM_VERSION_PATCHLEVEL,
-		      version[0], version[1], version[2], cmd);
+		      kernel_params->version[0],
+		      kernel_params->version[1],
+		      kernel_params->version[2],
+		      cmd);
 		r = -EINVAL;
 	}
 
 	/*
 	 * Fill in the kernel version.
 	 */
-	version[0] = DM_VERSION_MAJOR;
-	version[1] = DM_VERSION_MINOR;
-	version[2] = DM_VERSION_PATCHLEVEL;
-	if (copy_to_user(user->version, version, sizeof(version)))
+	kernel_params->version[0] = DM_VERSION_MAJOR;
+	kernel_params->version[1] = DM_VERSION_MINOR;
+	kernel_params->version[2] = DM_VERSION_PATCHLEVEL;
+	if (copy_to_user(user->version, kernel_params->version, sizeof(kernel_params->version)))
 		return -EFAULT;
 
 	return r;
@@ -1922,7 +1925,10 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
 	const size_t minimum_data_size = offsetof(struct dm_ioctl, data);
 	unsigned int noio_flag;
 
-	if (copy_from_user(param_kernel, user, minimum_data_size))
+	/* 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),
+			   minimum_data_size - sizeof(param_kernel->version)))
 		return -EFAULT;
 
 	if (param_kernel->data_size < minimum_data_size) {
@@ -2034,7 +2040,7 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
 	 * Check the interface version passed in.  This also
 	 * writes out the kernel's interface version.
 	 */
-	r = check_version(cmd, user);
+	r = check_version(cmd, user, &param_kernel);
 	if (r)
 		return r;
 
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [PATCH v2 4/6] device-mapper: Avoid double-fetch of version

[Mike Snitzer]

On Sat, Jun 03 2023 at 10:52P -0400,
Demi Marie Obenour <demi@invisiblethingslab.com> wrote:

> The version is fetched once in check_version(), which then does some
> validation and then overwrites the version in userspace with the API
> version supported by the kernel.  copy_params() then fetches the version
> from userspace *again*, and this time no validation is done.  The result
> is that the kernel's version number is completely controllable by
> userspace, provided that userspace can win a race condition.
> 
> Fix this flaw by not copying the version back to the kernel the second
> time.  This is not exploitable as the version is not further used in the
> kernel.  However, it could become a problem if future patches start
> relying on the version field.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> ---
>  drivers/md/dm-ioctl.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index da6ca26b51d0953df380582bb3a51c2ec22c27cb..7510afe237d979a5ee71afe87a20d49f631de1aa 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1873,30 +1873,33 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags)
>   * As well as checking the version compatibility this always
>   * copies the kernel interface version out.
>   */
> -static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
> +static int check_version(unsigned int cmd, struct dm_ioctl __user *user,
> +			 struct dm_ioctl *kernel_params)
>  {
> -	uint32_t version[3];
>  	int r = 0;
>  
> -	if (copy_from_user(version, user->version, sizeof(version)))
> +	if (copy_from_user(kernel_params->version, user->version, sizeof(kernel_params->version)))
>  		return -EFAULT;
>  
> -	if ((version[0] != DM_VERSION_MAJOR) ||
> -	    (version[1] > DM_VERSION_MINOR)) {
> +	if ((kernel_params->version[0] != DM_VERSION_MAJOR) ||
> +	    (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,
>  		      DM_VERSION_PATCHLEVEL,
> -		      version[0], version[1], version[2], cmd);
> +		      kernel_params->version[0],
> +		      kernel_params->version[1],
> +		      kernel_params->version[2],
> +		      cmd);
>  		r = -EINVAL;
>  	}
>  
>  	/*
>  	 * Fill in the kernel version.
>  	 */
> -	version[0] = DM_VERSION_MAJOR;
> -	version[1] = DM_VERSION_MINOR;
> -	version[2] = DM_VERSION_PATCHLEVEL;
> -	if (copy_to_user(user->version, version, sizeof(version)))
> +	kernel_params->version[0] = DM_VERSION_MAJOR;
> +	kernel_params->version[1] = DM_VERSION_MINOR;
> +	kernel_params->version[2] = DM_VERSION_PATCHLEVEL;
> +	if (copy_to_user(user->version, kernel_params->version, sizeof(kernel_params->version)))
>  		return -EFAULT;
>  
>  	return r;
> @@ -1922,7 +1925,10 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
>  	const size_t minimum_data_size = offsetof(struct dm_ioctl, data);
>  	unsigned int noio_flag;
>  
> -	if (copy_from_user(param_kernel, user, minimum_data_size))
> +	/* 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),
> +			   minimum_data_size - sizeof(param_kernel->version)))
>  		return -EFAULT;
>  
>  	if (param_kernel->data_size < minimum_data_size) {
> @@ -2034,7 +2040,7 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
>  	 * Check the interface version passed in.  This also
>  	 * writes out the kernel's interface version.
>  	 */
> -	r = check_version(cmd, user);
> +	r = check_version(cmd, user, &param_kernel);
>  	if (r)
>  		return r;
>  

I picked this patch up for 6.5, please see:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.5&id=c71878e9982075eab2e9f6dc5a09ba7b60ac1e24

But FYI, I folded these changes in:

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 526464904fc1..b58a15e212a3 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1838,6 +1838,9 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user,
 {
 	int r = 0;
 
+	/* Make certain version is first member of dm_ioctl struct */
+	BUILD_BUG_ON(offsetof(struct dm_ioctl, version) != 0);
+
 	if (copy_from_user(kernel_params->version, user->version, sizeof(kernel_params->version)))
 		return -EFAULT;
 
@@ -1885,7 +1888,7 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
 	const size_t minimum_data_size = offsetof(struct dm_ioctl, data);
 	unsigned int noio_flag;
 
-	/* Version has been copied from userspace already, avoid TOCTOU */
+	/* check_version() already copied version from userspace, avoid TOCTOU */
 	if (copy_from_user((char *)param_kernel + sizeof(param_kernel->version),
 			   (char __user *)user + sizeof(param_kernel->version),
 			   minimum_data_size - sizeof(param_kernel->version)))

Re: [PATCH v2 4/6] device-mapper: Avoid double-fetch of version

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 5788 bytes --]

On Thu, Jun 22, 2023 at 12:20:40PM -0400, Mike Snitzer wrote:
> On Sat, Jun 03 2023 at 10:52P -0400,
> Demi Marie Obenour <demi@invisiblethingslab.com> wrote:
> 
> > The version is fetched once in check_version(), which then does some
> > validation and then overwrites the version in userspace with the API
> > version supported by the kernel.  copy_params() then fetches the version
> > from userspace *again*, and this time no validation is done.  The result
> > is that the kernel's version number is completely controllable by
> > userspace, provided that userspace can win a race condition.
> > 
> > Fix this flaw by not copying the version back to the kernel the second
> > time.  This is not exploitable as the version is not further used in the
> > kernel.  However, it could become a problem if future patches start
> > relying on the version field.
> > 
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > ---
> >  drivers/md/dm-ioctl.c | 30 ++++++++++++++++++------------
> >  1 file changed, 18 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> > index da6ca26b51d0953df380582bb3a51c2ec22c27cb..7510afe237d979a5ee71afe87a20d49f631de1aa 100644
> > --- a/drivers/md/dm-ioctl.c
> > +++ b/drivers/md/dm-ioctl.c
> > @@ -1873,30 +1873,33 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags)
> >   * As well as checking the version compatibility this always
> >   * copies the kernel interface version out.
> >   */
> > -static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
> > +static int check_version(unsigned int cmd, struct dm_ioctl __user *user,
> > +			 struct dm_ioctl *kernel_params)
> >  {
> > -	uint32_t version[3];
> >  	int r = 0;
> >  
> > -	if (copy_from_user(version, user->version, sizeof(version)))
> > +	if (copy_from_user(kernel_params->version, user->version, sizeof(kernel_params->version)))
> >  		return -EFAULT;
> >  
> > -	if ((version[0] != DM_VERSION_MAJOR) ||
> > -	    (version[1] > DM_VERSION_MINOR)) {
> > +	if ((kernel_params->version[0] != DM_VERSION_MAJOR) ||
> > +	    (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,
> >  		      DM_VERSION_PATCHLEVEL,
> > -		      version[0], version[1], version[2], cmd);
> > +		      kernel_params->version[0],
> > +		      kernel_params->version[1],
> > +		      kernel_params->version[2],
> > +		      cmd);
> >  		r = -EINVAL;
> >  	}
> >  
> >  	/*
> >  	 * Fill in the kernel version.
> >  	 */
> > -	version[0] = DM_VERSION_MAJOR;
> > -	version[1] = DM_VERSION_MINOR;
> > -	version[2] = DM_VERSION_PATCHLEVEL;
> > -	if (copy_to_user(user->version, version, sizeof(version)))
> > +	kernel_params->version[0] = DM_VERSION_MAJOR;
> > +	kernel_params->version[1] = DM_VERSION_MINOR;
> > +	kernel_params->version[2] = DM_VERSION_PATCHLEVEL;
> > +	if (copy_to_user(user->version, kernel_params->version, sizeof(kernel_params->version)))
> >  		return -EFAULT;
> >  
> >  	return r;
> > @@ -1922,7 +1925,10 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
> >  	const size_t minimum_data_size = offsetof(struct dm_ioctl, data);
> >  	unsigned int noio_flag;
> >  
> > -	if (copy_from_user(param_kernel, user, minimum_data_size))
> > +	/* 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),
> > +			   minimum_data_size - sizeof(param_kernel->version)))
> >  		return -EFAULT;
> >  
> >  	if (param_kernel->data_size < minimum_data_size) {
> > @@ -2034,7 +2040,7 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
> >  	 * Check the interface version passed in.  This also
> >  	 * writes out the kernel's interface version.
> >  	 */
> > -	r = check_version(cmd, user);
> > +	r = check_version(cmd, user, &param_kernel);
> >  	if (r)
> >  		return r;
> >  
> 
> I picked this patch up for 6.5, please see:
> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.5&id=c71878e9982075eab2e9f6dc5a09ba7b60ac1e24

That’s great, thanks!

> But FYI, I folded these changes in:
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 526464904fc1..b58a15e212a3 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1838,6 +1838,9 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user,
>  {
>  	int r = 0;
>  
> +	/* Make certain version is first member of dm_ioctl struct */
> +	BUILD_BUG_ON(offsetof(struct dm_ioctl, version) != 0);
> +
>  	if (copy_from_user(kernel_params->version, user->version, sizeof(kernel_params->version)))
>  		return -EFAULT;
>  
> @@ -1885,7 +1888,7 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
>  	const size_t minimum_data_size = offsetof(struct dm_ioctl, data);
>  	unsigned int noio_flag;
>  
> -	/* Version has been copied from userspace already, avoid TOCTOU */
> +	/* check_version() already copied version from userspace, avoid TOCTOU */
>  	if (copy_from_user((char *)param_kernel + sizeof(param_kernel->version),
>  			   (char __user *)user + sizeof(param_kernel->version),
>  			   minimum_data_size - sizeof(param_kernel->version)))

Those changes indeed make the code better, thanks for including them!
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 5/6] device-mapper: Refuse to create device named "control"

[Demi Marie Obenour]

Typical userspace setups create a symlink under /dev/mapper with the
name of the device, but /dev/mapper/control is reserved for the control
device.  Therefore, trying to create such a device is almost certain to
be a userspace bug.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/md/dm-ioctl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 7510afe237d979a5ee71afe87a20d49f631de1aa..5b647ab044e44b0c9d0961b5a336b41f50408f88 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -767,7 +767,12 @@ static int get_target_version(struct file *filp, struct dm_ioctl *param, size_t
 static int check_name(const char *name)
 {
 	if (strchr(name, '/')) {
-		DMERR("invalid device name");
+		DMERR("device name cannot contain '/'");
+		return -EINVAL;
+	}
+
+	if (strcmp(name, DM_CONTROL_NODE) == 0) {
+		DMERR("device name cannot be \"%s\"", DM_CONTROL_NODE);
 		return -EINVAL;
 	}
 
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH v2 6/6] device-mapper: "." and ".." are not valid symlink names

[Demi Marie Obenour]

Using either of these is going to greatly confuse userspace, as they are
not valid symlink names and so creating the usual /dev/mapper/NAME
symlink will not be possible.  As creating a device with either of these
names is almost certainly a userspace bug, just error out.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/md/dm-ioctl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 5b647ab044e44b0c9d0961b5a336b41f50408f88..12be95ee20778b9acd3ea0d98f160a7409028afc 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -771,8 +771,10 @@ static int check_name(const char *name)
 		return -EINVAL;
 	}
 
-	if (strcmp(name, DM_CONTROL_NODE) == 0) {
-		DMERR("device name cannot be \"%s\"", DM_CONTROL_NODE);
+	if (strcmp(name, DM_CONTROL_NODE) == 0 ||
+	    strcmp(name, ".") == 0 ||
+	    strcmp(name, "..") == 0) {
+		DMERR("device name cannot be \"%s\", \".\", or \"..\"", DM_CONTROL_NODE);
 		return -EINVAL;
 	}
 
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab