[GIT PULL] Revert "dlm: check the maximum size of a request from user"

[GIT PULL] Revert "dlm: check the maximum size of a request from user"

[David Teigland]

Hi Linus,

Please pull the following fix from branch:

git://git.kernel.org/pub/scm/linux/kernel/git/teigland/linux-dlm.git for-linus

This reverts commit 2b75bc9121e54e22537207b47b71373bcb0be41c.

There is something wrong with the CONFIG_COMPAT max size
check in ioctl write.  There is a report of a case where
this breaks userland (clvmd) when maximum resource name
lengths are used.  I am still sorting out exactly which
combinations of kernel and userland libs are a problem.

Reported-by: Jana Saout <jana@saout.de>
CC: Sasha Levin <levinsasha928@gmail.com>
Signed-off-by: David Teigland <teigland@redhat.com>
---
 fs/dlm/user.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/fs/dlm/user.c b/fs/dlm/user.c
index 7ff4985..eb4ed9b 100644
--- a/fs/dlm/user.c
+++ b/fs/dlm/user.c
@@ -503,13 +503,6 @@ static ssize_t device_write(struct file *file, const char __user *buf,
 #endif
 		return -EINVAL;
 
-#ifdef CONFIG_COMPAT
-	if (count > sizeof(struct dlm_write_request32) + DLM_RESNAME_MAXLEN)
-#else
-	if (count > sizeof(struct dlm_write_request) + DLM_RESNAME_MAXLEN)
-#endif
-		return -EINVAL;
-
 	kbuf = kzalloc(count + 1, GFP_NOFS);
 	if (!kbuf)
 		return -ENOMEM;
-- 
1.8.1.rc1.5.g7e0651a

Re: [GIT PULL] Revert "dlm: check the maximum size of a request from user"

[Sasha Levin]

Hi David,

This opens up a hole for userspace to force the kernel to allocate
huge chunks of memory, triggering oom killing spree and such.

It should probably be fixed instead of just reverted.

I'll look into it.


Thanks,
Sasha

On Mon, Feb 4, 2013 at 12:06 PM, David Teigland <teigland@redhat.com> wrote:
> Hi Linus,
>
> Please pull the following fix from branch:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/teigland/linux-dlm.git for-linus
>
> This reverts commit 2b75bc9121e54e22537207b47b71373bcb0be41c.
>
> There is something wrong with the CONFIG_COMPAT max size
> check in ioctl write.  There is a report of a case where
> this breaks userland (clvmd) when maximum resource name
> lengths are used.  I am still sorting out exactly which
> combinations of kernel and userland libs are a problem.
>
> Reported-by: Jana Saout <jana@saout.de>
> CC: Sasha Levin <levinsasha928@gmail.com>
> Signed-off-by: David Teigland <teigland@redhat.com>
> ---
>  fs/dlm/user.c | 7 -------
>  1 file changed, 7 deletions(-)
>
> diff --git a/fs/dlm/user.c b/fs/dlm/user.c
> index 7ff4985..eb4ed9b 100644
> --- a/fs/dlm/user.c
> +++ b/fs/dlm/user.c
> @@ -503,13 +503,6 @@ static ssize_t device_write(struct file *file, const char __user *buf,
>  #endif
>                 return -EINVAL;
>
> -#ifdef CONFIG_COMPAT
> -       if (count > sizeof(struct dlm_write_request32) + DLM_RESNAME_MAXLEN)
> -#else
> -       if (count > sizeof(struct dlm_write_request) + DLM_RESNAME_MAXLEN)
> -#endif
> -               return -EINVAL;
> -
>         kbuf = kzalloc(count + 1, GFP_NOFS);
>         if (!kbuf)
>                 return -ENOMEM;
> --
> 1.8.1.rc1.5.g7e0651a
>

Re: [GIT PULL] Revert "dlm: check the maximum size of a request from user"

[David Teigland]

On Mon, Feb 04, 2013 at 03:19:44PM -0500, Sasha Levin wrote:
> Hi David,
> 
> This opens up a hole for userspace to force the kernel to allocate
> huge chunks of memory, triggering oom killing spree and such.
> 
> It should probably be fixed instead of just reverted.
> 
> I'll look into it.

Here is the patch I'm planning to put in the queue for the next
merge window, once it's been tested.

Subject: [PATCH] dlm: check the write size from user

Return EINVAL from write if the size is larger than
allowed.  Do this before allocating kernel memory for
the bogus size, which could lead to OOM.

Reported-by: Sasha Levin <levinsasha928@gmail.com>
Signed-off-by: David Teigland <teigland@redhat.com>
---
 fs/dlm/user.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/dlm/user.c b/fs/dlm/user.c
index eb4ed9b..911649a 100644
--- a/fs/dlm/user.c
+++ b/fs/dlm/user.c
@@ -503,6 +503,13 @@ static ssize_t device_write(struct file *file, const char __user *buf,
 #endif
 		return -EINVAL;
 
+	/*
+	 * can't compare against COMPAT/dlm_write_request32 because
+	 * we don't yet know if is64bit is zero
+	 */
+	if (count > sizeof(struct dlm_write_request) + DLM_RESNAME_MAXLEN)
+		return -EINVAL;
+
 	kbuf = kzalloc(count + 1, GFP_NOFS);
 	if (!kbuf)
 		return -ENOMEM;
-- 
1.8.1.rc1.5.g7e0651a

Re: [GIT PULL] Revert "dlm: check the maximum size of a request from user"

[David Teigland]

On Mon, Feb 04, 2013 at 12:06:55PM -0500, David Teigland wrote:
> Please pull the following fix from branch:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/teigland/linux-dlm.git for-linus
> 
> This reverts commit 2b75bc9121e54e22537207b47b71373bcb0be41c.

Hi Linus,

You can choose to pull that revert, or you can alternatively pull this fix
to the original patch from this branch:

git://git.kernel.org/pub/scm/linux/kernel/git/teigland/linux-dlm.git fix-max-write

Thanks to Jana who reported the problem and was able to test this fix so
quickly.

Subject: [PATCH] dlm: check the write size from user

Return EINVAL from write if the size is larger than
allowed.  Do this before allocating kernel memory for
the bogus size, which could lead to OOM.

Reported-by: Sasha Levin <levinsasha928@gmail.com>
Tested-by: Jana Saout <jana@saout.de>
Signed-off-by: David Teigland <teigland@redhat.com>
---
 fs/dlm/user.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/dlm/user.c b/fs/dlm/user.c
index 7ff4985..911649a 100644
--- a/fs/dlm/user.c
+++ b/fs/dlm/user.c
@@ -503,11 +503,11 @@ static ssize_t device_write(struct file *file, const char __user *buf,
 #endif
 		return -EINVAL;
 
-#ifdef CONFIG_COMPAT
-	if (count > sizeof(struct dlm_write_request32) + DLM_RESNAME_MAXLEN)
-#else
+	/*
+	 * can't compare against COMPAT/dlm_write_request32 because
+	 * we don't yet know if is64bit is zero
+	 */
 	if (count > sizeof(struct dlm_write_request) + DLM_RESNAME_MAXLEN)
-#endif
 		return -EINVAL;
 
 	kbuf = kzalloc(count + 1, GFP_NOFS);
-- 
1.8.1.rc1.5.g7e0651a