| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <200710130016.29937.arnd@arndb.de>
Date: Sat, 13 Oct 2007 00:16:29 +0200
From: Arnd Bergmann <arnd@...db.de>
To: Alasdair G Kergon <agk@...hat.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
dm-devel@...hat.com, linux-kernel@...r.kernel.org,
Milan Broz <mbroz@...hat.com>,
Guido Guenther <agx@...xcpu.org>,
Kevin Corry <kevcorry@...ibm.com>, stable@...nel.org
Subject: Re: [2.6.24 PATCH 02/25] dm io:ctl use constant struct size
On Friday 12 October 2007, Alasdair G Kergon wrote:
> Make size of dm_ioctl struct always 312 bytes on all supported
> architectures.
>
> This change retains compatibility with already-compiled code because
> it uses an embedded offset to locate the payload that follows the
> structure.
>
> On 64-bit architectures there is no change at all; on 32-bit
> we are increasing the size of dm-ioctl from 308 to 312 bytes.
>
> Currently with 32-bit userspace / 64-bit kernel on x86_64
> some ioctls (including rename, message) are incorrectly rejected
> by the comparison against 'param + 1'. This breaks userspace
> lvrename and multipath 'fail_if_no_path' changes, for example.
>
> (BTW Device-mapper uses its own versioning and ignores the ioctl
> size bits. Only the generic ioctl compat code on mixed arches
> checks them, and that will continue to accept both sizes for now,
> but we intend to list 308 as deprecated and eventually remove it.)
>
> Signed-off-by: Milan Broz <mbroz@...hat.com>
> Signed-off-by: Alasdair G Kergon <agk@...hat.com>
> Cc: Guido Guenther <agx@...xcpu.org>
> Cc: Kevin Corry <kevcorry@...ibm.com>
> Cc: stable@...nel.org
>
This change seems rather bogus, you're changing the ABI just to work
around a bug in the compat_ioctl layer. Why not just do the compat
code the right way, like the patch below?
Arnd <><
---
dm: move compat_ioctl handling to dm-ioctl.c
Device mapper ioctl numbers use a variable size field
Signed-off-by: Arnd Bergmann <arnd@...db.de>
---
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index b441d82..a662279 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -700,7 +700,7 @@ static int dev_rename(struct dm_ioctl *param, size_t param_size)
int r;
char *new_name = (char *) param + param->data_start;
- if (new_name < (char *) (param + 1) ||
+ if (new_name < (char *)param + param->data_size ||
invalid_str(new_name, (void *) param + param_size)) {
DMWARN("Invalid new logical volume name supplied.");
return -EINVAL;
@@ -726,7 +726,7 @@ static int dev_set_geometry(struct dm_ioctl *param, size_t param_size)
if (!md)
return -ENXIO;
- if (geostr < (char *) (param + 1) ||
+ if (geostr < (char *)param + param->data_size ||
invalid_str(geostr, (void *) param + param_size)) {
DMWARN("Invalid geometry supplied.");
goto out;
@@ -1233,7 +1233,7 @@ static int target_message(struct dm_ioctl *param, size_t param_size)
if (r)
goto out;
- if (tmsg < (struct dm_target_msg *) (param + 1) ||
+ if (tmsg < (struct dm_target_msg *) ((char *)param + param->data_size) ||
invalid_str(tmsg->message, (void *) param + param_size)) {
DMWARN("Invalid target message parameters.");
r = -EINVAL;
@@ -1348,11 +1348,11 @@ static void free_params(struct dm_ioctl *param)
vfree(param);
}
-static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl **param)
+static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl **param, uint ulen)
{
struct dm_ioctl tmp, *dmi;
- if (copy_from_user(&tmp, user, sizeof(tmp)))
+ if (copy_from_user(&tmp, user, ulen))
return -EFAULT;
if (tmp.data_size < sizeof(tmp))
@@ -1399,13 +1399,11 @@ static int validate_params(uint cmd, struct dm_ioctl *param)
return 0;
}
-static int ctl_ioctl(struct inode *inode, struct file *file,
- uint command, ulong u)
+static int ctl_ioctl(uint command, struct dm_ioctl __user *user, uint ulen)
{
int r = 0;
unsigned int cmd;
struct dm_ioctl *param;
- struct dm_ioctl __user *user = (struct dm_ioctl __user *) u;
ioctl_fn fn = NULL;
size_t param_size;
@@ -1447,7 +1445,7 @@ static int ctl_ioctl(struct inode *inode, struct file *file,
/*
* Copy the parameters into kernel space.
*/
- r = copy_params(user, ¶m);
+ r = copy_params(user, ¶m, ulen);
current->flags &= ~PF_MEMALLOC;
@@ -1459,7 +1457,7 @@ static int ctl_ioctl(struct inode *inode, struct file *file,
goto out;
param_size = param->data_size;
- param->data_size = sizeof(*param);
+ param->data_size = ulen;
r = fn(param, param_size);
/*
@@ -1473,8 +1471,64 @@ static int ctl_ioctl(struct inode *inode, struct file *file,
return r;
}
+static int ctl_ioctl(struct inode *inode, struct file *file,
+ uint command, ulong u)
+{
+ return ctl_do_ioctl(command, (void __user *)u, sizeof (struct dm_ioctl));
+}
+
+#ifdef CONFIG_COMPAT
+struct compat_dm_ioctl {
+ /*
+ * The version number is made up of three parts:
+ * major - no backward or forward compatibility,
+ * minor - only backwards compatible,
+ * patch - both backwards and forwards compatible.
+ *
+ * All clients of the ioctl interface should fill in the
+ * version number of the interface that they were
+ * compiled with.
+ *
+ * All recognised ioctl commands (ie. those that don't
+ * return -ENOTTY) fill out this field, even if the
+ * command failed.
+ */
+ uint32_t version[3]; /* in/out */
+ uint32_t data_size; /* total size of data passed in
+ * including this struct */
+
+ uint32_t data_start; /* offset to start of data
+ * relative to start of this struct */
+
+ uint32_t target_count; /* in/out */
+ int32_t open_count; /* out */
+ uint32_t flags; /* in/out */
+ uint32_t event_nr; /* in/out */
+ uint32_t padding;
+
+ compat_u64 dev; /* in/out */
+
+ char name[DM_NAME_LEN]; /* device name */
+ char uuid[DM_UUID_LEN]; /* unique identifier for
+ * the block device */
+};
+
+static int compat_ctl_ioctl(struct inode *inode, struct file *file,
+ uint command, ulong u)
+{
+ int ret = 0;
+ lock_kernel();
+ ret = ctl_do_ioctl(command, compat_ptr(u), sizeof(struct compat_dm_ioctl));
+ unlock_kernel();
+ return ret;
+}
+#else
+#define compat_ctl_ioctl NULL
+#endif
+
static const struct file_operations _ctl_fops = {
.ioctl = ctl_ioctl,
+ .compat_ioctl = compat_ctl_ioctl,
.owner = THIS_MODULE,
};
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -76,7 +76,6 @@
#include <linux/mii.h>
#include <linux/if_bonding.h>
#include <linux/watchdog.h>
-#include <linux/dm-ioctl.h>
#include <linux/soundcard.h>
#include <linux/lp.h>
@@ -1986,39 +1985,6 @@ COMPATIBLE_IOCTL(STOP_ARRAY_RO)
COMPATIBLE_IOCTL(RESTART_ARRAY_RW)
COMPATIBLE_IOCTL(GET_BITMAP_FILE)
ULONG_IOCTL(SET_BITMAP_FILE)
-/* DM */
-COMPATIBLE_IOCTL(DM_VERSION_32)
-COMPATIBLE_IOCTL(DM_REMOVE_ALL_32)
-COMPATIBLE_IOCTL(DM_LIST_DEVICES_32)
-COMPATIBLE_IOCTL(DM_DEV_CREATE_32)
-COMPATIBLE_IOCTL(DM_DEV_REMOVE_32)
-COMPATIBLE_IOCTL(DM_DEV_RENAME_32)
-COMPATIBLE_IOCTL(DM_DEV_SUSPEND_32)
-COMPATIBLE_IOCTL(DM_DEV_STATUS_32)
-COMPATIBLE_IOCTL(DM_DEV_WAIT_32)
-COMPATIBLE_IOCTL(DM_TABLE_LOAD_32)
-COMPATIBLE_IOCTL(DM_TABLE_CLEAR_32)
-COMPATIBLE_IOCTL(DM_TABLE_DEPS_32)
-COMPATIBLE_IOCTL(DM_TABLE_STATUS_32)
-COMPATIBLE_IOCTL(DM_LIST_VERSIONS_32)
-COMPATIBLE_IOCTL(DM_TARGET_MSG_32)
-COMPATIBLE_IOCTL(DM_DEV_SET_GEOMETRY_32)
-COMPATIBLE_IOCTL(DM_VERSION)
-COMPATIBLE_IOCTL(DM_REMOVE_ALL)
-COMPATIBLE_IOCTL(DM_LIST_DEVICES)
-COMPATIBLE_IOCTL(DM_DEV_CREATE)
-COMPATIBLE_IOCTL(DM_DEV_REMOVE)
-COMPATIBLE_IOCTL(DM_DEV_RENAME)
-COMPATIBLE_IOCTL(DM_DEV_SUSPEND)
-COMPATIBLE_IOCTL(DM_DEV_STATUS)
-COMPATIBLE_IOCTL(DM_DEV_WAIT)
-COMPATIBLE_IOCTL(DM_TABLE_LOAD)
-COMPATIBLE_IOCTL(DM_TABLE_CLEAR)
-COMPATIBLE_IOCTL(DM_TABLE_DEPS)
-COMPATIBLE_IOCTL(DM_TABLE_STATUS)
-COMPATIBLE_IOCTL(DM_LIST_VERSIONS)
-COMPATIBLE_IOCTL(DM_TARGET_MSG)
-COMPATIBLE_IOCTL(DM_DEV_SET_GEOMETRY)
/* Big K */
COMPATIBLE_IOCTL(PIO_FONT)
COMPATIBLE_IOCTL(GIO_FONT)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists