drivers/md/dm-ioctl.c | 75 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 63 insertions(+), 12 deletions(-)
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
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
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
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
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
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
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
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
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 LabOn 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
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
>
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
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
>
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, ¶m_kernel);
if (r)
return r;
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things LabOn 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, ¶m_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)))
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, ¶m_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
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 LabUsing 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© 2016 - 2026 Red Hat, Inc.