* [PATCH v2 1/6] device-mapper: Check that target specs are sufficiently aligned
[not found] ` <20230603145244.1538-1-demi@invisiblethingslab.com>
@ 2023-06-03 14:52 ` Demi Marie Obenour
2023-06-22 17:29 ` [dm-devel] " Mikulas Patocka
2023-06-03 14:52 ` [PATCH v2 2/6] device-mapper: Avoid pointer arithmetic overflow Demi Marie Obenour
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Demi Marie Obenour @ 2023-06-03 14:52 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, dm-devel
Cc: Demi Marie Obenour, linux-kernel, stable
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [dm-devel] [PATCH v2 1/6] device-mapper: Check that target specs are sufficiently aligned
2023-06-03 14:52 ` [PATCH v2 1/6] device-mapper: Check that target specs are sufficiently aligned Demi Marie Obenour
@ 2023-06-22 17:29 ` Mikulas Patocka
2023-06-22 20:27 ` Demi Marie Obenour
0 siblings, 1 reply; 15+ messages in thread
From: Mikulas Patocka @ 2023-06-22 17:29 UTC (permalink / raw)
To: Demi Marie Obenour
Cc: Alasdair Kergon, Mike Snitzer, dm-devel, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dm-devel] [PATCH v2 1/6] device-mapper: Check that target specs are sufficiently aligned
2023-06-22 17:29 ` [dm-devel] " Mikulas Patocka
@ 2023-06-22 20:27 ` Demi Marie Obenour
0 siblings, 0 replies; 15+ messages in thread
From: Demi Marie Obenour @ 2023-06-22 20:27 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Alasdair Kergon, Mike Snitzer, dm-devel, linux-kernel, stable
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/6] device-mapper: Avoid pointer arithmetic overflow
[not found] ` <20230603145244.1538-1-demi@invisiblethingslab.com>
2023-06-03 14:52 ` [PATCH v2 1/6] device-mapper: Check that target specs are sufficiently aligned Demi Marie Obenour
@ 2023-06-03 14:52 ` Demi Marie Obenour
2023-06-22 17:30 ` [dm-devel] " Mikulas Patocka
2023-06-22 22:50 ` Mike Snitzer
2023-06-03 14:52 ` [PATCH v2 3/6] device-mapper: structs and parameter strings must not overlap Demi Marie Obenour
2023-06-03 14:52 ` [PATCH v2 4/6] device-mapper: Avoid double-fetch of version Demi Marie Obenour
3 siblings, 2 replies; 15+ messages in thread
From: Demi Marie Obenour @ 2023-06-03 14:52 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, dm-devel
Cc: Demi Marie Obenour, linux-kernel, stable
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [dm-devel] [PATCH v2 2/6] device-mapper: Avoid pointer arithmetic overflow
2023-06-03 14:52 ` [PATCH v2 2/6] device-mapper: Avoid pointer arithmetic overflow Demi Marie Obenour
@ 2023-06-22 17:30 ` Mikulas Patocka
2023-06-22 22:50 ` Mike Snitzer
1 sibling, 0 replies; 15+ messages in thread
From: Mikulas Patocka @ 2023-06-22 17:30 UTC (permalink / raw)
To: Demi Marie Obenour
Cc: Alasdair Kergon, Mike Snitzer, dm-devel, linux-kernel, stable
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 2/6] device-mapper: Avoid pointer arithmetic overflow
2023-06-03 14:52 ` [PATCH v2 2/6] device-mapper: Avoid pointer arithmetic overflow Demi Marie Obenour
2023-06-22 17:30 ` [dm-devel] " Mikulas Patocka
@ 2023-06-22 22:50 ` Mike Snitzer
1 sibling, 0 replies; 15+ messages in thread
From: Mike Snitzer @ 2023-06-22 22:50 UTC (permalink / raw)
To: Demi Marie Obenour; +Cc: Alasdair Kergon, dm-devel, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/6] device-mapper: structs and parameter strings must not overlap
[not found] ` <20230603145244.1538-1-demi@invisiblethingslab.com>
2023-06-03 14:52 ` [PATCH v2 1/6] device-mapper: Check that target specs are sufficiently aligned Demi Marie Obenour
2023-06-03 14:52 ` [PATCH v2 2/6] device-mapper: Avoid pointer arithmetic overflow Demi Marie Obenour
@ 2023-06-03 14:52 ` Demi Marie Obenour
2023-06-22 17:31 ` [dm-devel] " Mikulas Patocka
2023-06-03 14:52 ` [PATCH v2 4/6] device-mapper: Avoid double-fetch of version Demi Marie Obenour
3 siblings, 1 reply; 15+ messages in thread
From: Demi Marie Obenour @ 2023-06-03 14:52 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, dm-devel
Cc: Demi Marie Obenour, linux-kernel, stable
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [dm-devel] [PATCH v2 3/6] device-mapper: structs and parameter strings must not overlap
2023-06-03 14:52 ` [PATCH v2 3/6] device-mapper: structs and parameter strings must not overlap Demi Marie Obenour
@ 2023-06-22 17:31 ` Mikulas Patocka
0 siblings, 0 replies; 15+ messages in thread
From: Mikulas Patocka @ 2023-06-22 17:31 UTC (permalink / raw)
To: Demi Marie Obenour
Cc: Alasdair Kergon, Mike Snitzer, dm-devel, linux-kernel, stable
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 4/6] device-mapper: Avoid double-fetch of version
[not found] ` <20230603145244.1538-1-demi@invisiblethingslab.com>
` (2 preceding siblings ...)
2023-06-03 14:52 ` [PATCH v2 3/6] device-mapper: structs and parameter strings must not overlap Demi Marie Obenour
@ 2023-06-03 14:52 ` Demi Marie Obenour
3 siblings, 0 replies; 15+ messages in thread
From: Demi Marie Obenour @ 2023-06-03 14:52 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, dm-devel
Cc: Demi Marie Obenour, linux-kernel, stable
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 Lab
^ permalink raw reply related [flat|nested] 15+ messages in thread